RFR: JDK-8242302 : Refactor jpackage native code

[Erik Joelsson]

Thanks, that looks good!

Only two minor whitespace nits.

Line 32-33, 102-103: continuation indent 4 spaces

Line 101: extra space

No need for new webrev.

/Erik

On 2020-04-23 11:38, Alexey Semenyuk wrote:
> Erik,
>
> Please review updated patch with the suggested improvements at [1]. 
> The webrev includes only incremental changes to 
> Lib-jdk.incubator.jpackage.gmk for clarity.
>
> [1] 
> http://cr.openjdk.java.net/~asemenyuk/8242302/webrev.01/webrev.01/index.html
>
> - Alexey
>
> On 4/20/2020 1:17 PM, Erik Joelsson wrote:
>> (adding build-dev)
>>
>> Hello Alexey,
>>
>> The SetupJdkExecutable and SetupJdkLibrary macros are designed to 
>> find the correct source dirs automatically as long as they follow the 
>> standard naming. In this case you are adding some extra with the 
>> "common" dir as well as reusing some src dirs between multiple calls 
>> to the macros. There are constructs prepared for handling these 
>> situations too. We introduced these specialized macros to avoid 
>> repeated setting up of almost identical source dirs like in your 
>> patch, and to help enforce a standard in directory naming.
>>
>> Upon closer inspection, I see now that what's said above only applies 
>> to SetupJdkLibrary while SetupJdkExecutable is lacking most of these 
>> features. Adding them should be pretty straight forward though and 
>> should be fixed. Not asking you to do it though. So for those cases, 
>> I recommend using the FindSrcDirsForComponent macro to find the 
>> source dirs needed for now. It's defined in 
>> make/common/JdkNativeCompilation.gmk. You can also extract the exact 
>> resolved SRC dirs from a call to either macro using for example 
>> BUILD_JPACKAGE_APPLAUNCHEREXE_SRC (after the macro has been evaled). 
>> This would be recommended for BUILD_JPACKAGE_APPLAUNCHERWEXE.
>>
>> For the ones where the NAME field matches the source sub dir (which 
>> should be almost all unless there is a good reason), you don't need 
>> to specify SRC at all. If you need to add the "common" subdir, then 
>> you can use EXTRA_SRC and the special designation 
>> "jdk.incubator.jpackage:common" which will get parsed into all 
>> existing common directories for your current build target. In the 
>> case of jpackageapplauncherw, you can similarly use 
>> "jdk.incubator.jpackage:applauncher" in the SRC field.
>>
>> For SetupJdkLibrary, there is no need to explicitly add source dirs 
>> to -I paths. All source dirs are by default added unless 
>> HEADERS_FROM_SRC is set to false. Again this should be fixed for 
>> SetupJdkExecutable.
>>
>> For macros that we intend to call with parameters, our style 
>> convention is to declare them with camel case and always on a new 
>> line. This is to make them stand out clearly from variables which we 
>> assign with :=, and make them look a little bit more like function 
>> declarations. See make/common/Utils.gmk for examples of how this 
>> looks. So something like:
>>
>> JpackageWithStaticCrt = \
>>     $(filter-out -MD, $1) -MT
>>
>> /Erik
>>
>>
>> On 2020-04-15 13:13, Alexey Semenyuk wrote:
>>> Please review fix [2] for jpackage bug [1].
>>>
>>> Refactor jpackage native code.
>>>
>>> - Improve code reuse between different platforms.
>>> - Replace custom string classes with the standard std::basic_string.
>>> - Merge functionality of libapplauncher.dll(.so) library with 
>>> jpackageapplauncher(.exe) binary. There is no point to keep two 
>>> different executables.
>>> - Link jpackageapplauncher.exe with MSVC Run-Time library statically 
>>> to avoid copying of MSVC Run-Time dlls to app's bin folder.
>>> - Remove unused code.
>>>
>>> - Alexey
>>>
>>> [1] https://bugs.openjdk.java.net/browse/JDK-8242302
>>>
>>> [2] http://cr.openjdk.java.net/~asemenyuk/8242302/webrev.00
>>>
>