Re: RFR: JDK-8242302 : Refactor jpackage native code
Thank you for the review! Whitespace issues will be fixed in the final patch. - Alexey On 4/23/2020 2:51 PM, Erik Joelsson wrote: 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
Re: RFR: JDK-8242302 : Refactor jpackage native code
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
Re: RFR: JDK-8242302 : Refactor jpackage native code
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
Re: RFR: JDK-8242302 : Refactor jpackage native code
(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
