Feedback from build team on EA2:
Just a few minor nits:
* In Launcher-jdk.jpackage.gmk, please indent the else clause ("$(eval $(call SetupBuildLauncher, jpackage ....") two spaces.
* In Lib-jdk.jpackage.gmk, I think there's still room to prune some more unnecessary compiler flags and parameters to SetupJdkExecutable:
65 CFLAGS_linux := -fPIC, \
66 CFLAGS_solaris := -KPIC, \
67 CFLAGS_macosx := -fPIC, \
I wonder if these really are needed. Normally, only shared libraries neeed PIC code. (And for those we set it automatically.)
69 LDFLAGS_windows := -nologo, \
This should not be needed, it's incorporated in CXXFLAGS_JDKEXE. (Also in the second block, for jpackageapplauncherw).
72 LIBS_solaris := -lc, \
Same here; this should not be needed. It's in GLOBAL_LIBS on Solaris, and is always included.
75 VERSIONINFO_RESOURCE := $(GLOBAL_VERSION_INFO_RESOURCE), \
This is setup by default by SetupJdkExecutable, so you can remove it. (Also in the second block, for jpackageapplauncherw).
Finally, I do believe that the setups of jpackageapplauncher and jpackageapplauncherw should be done in Launcher-jdk.jpackage.gmk, not Lib-jdk.jpackage.gmk. Since they are to be shipped as resources, they are not "really" launchers in our normal sense, but they are at least more launchers than they are libraries.
As we've talked about privately, in the future I'd like to see Windows too use the SetupBuildLauncher method for creating the launcher, but this is clearly good enough for inclusion in the mainline.
Just a few minor nits:
* In Launcher-jdk.jpackage.gmk, please indent the else clause ("$(eval $(call SetupBuildLauncher, jpackage ....") two spaces.
* In Lib-jdk.jpackage.gmk, I think there's still room to prune some more unnecessary compiler flags and parameters to SetupJdkExecutable:
65 CFLAGS_linux := -fPIC, \
66 CFLAGS_solaris := -KPIC, \
67 CFLAGS_macosx := -fPIC, \
I wonder if these really are needed. Normally, only shared libraries neeed PIC code. (And for those we set it automatically.)
69 LDFLAGS_windows := -nologo, \
This should not be needed, it's incorporated in CXXFLAGS_JDKEXE. (Also in the second block, for jpackageapplauncherw).
72 LIBS_solaris := -lc, \
Same here; this should not be needed. It's in GLOBAL_LIBS on Solaris, and is always included.
75 VERSIONINFO_RESOURCE := $(GLOBAL_VERSION_INFO_RESOURCE), \
This is setup by default by SetupJdkExecutable, so you can remove it. (Also in the second block, for jpackageapplauncherw).
Finally, I do believe that the setups of jpackageapplauncher and jpackageapplauncherw should be done in Launcher-jdk.jpackage.gmk, not Lib-jdk.jpackage.gmk. Since they are to be shipped as resources, they are not "really" launchers in our normal sense, but they are at least more launchers than they are libraries.
As we've talked about privately, in the future I'd like to see Windows too use the SetupBuildLauncher method for creating the launcher, but this is clearly good enough for inclusion in the mainline.
- duplicates
-
JDK-8216521 Fix white space errors in jpackage source files
- Closed
- relates to
-
JDK-8217317 Create jpackage native library for windows
- Resolved
-
JDK-8212780 Packaging Tool Implementation
- Resolved