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
