JDK-8212936 : Makefile and other improvements for jpackager
  • Type: Bug
  • Component: tools
  • Sub-Component: jpackage
  • Affected Version: internal
  • Priority: P2
  • Status: Resolved
  • Resolution: Fixed
  • Submitted: 2018-10-24
  • Updated: 2019-03-22
  • Resolved: 2018-11-19
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
Other
internalFixed
Related Reports
Relates :  
Relates :  
Relates :  
Relates :  
Description
Feedback on initial RFR for  "JDK-8212780: JEP 343: Packaging Tool Implementation" That needs to be addressed:

1.) CompileJavaModules.gmk:

The jdk.packager_CLEAN_FILES could be replaced with a simple "jdk.packager_CLEAN := .properties" unless you actually need to add all the platform specific files to builds for all platforms (which I doubt?). To clarify, the current patch will add all linux, windows and mac properties to builds of all OSes. Is that intended?

2.) Modules.gmk:

I would rather have the filter be conditional on an inclusive list of platforms. Other OpenJDK contributors are building for other OSes like AIX and we don't want to have to keep track of all those. The list of OSes that jpackager supports is well defined, so better check for those explicitly.

3.) src/jdk.jlink/share/classes/module-info.java:

I believe the qualified export needs to be put in a module-info.java.extra file since we filter out the target module on unsupported platforms.

4.) Launcher-jdk.packager.gmk:

* Please use $$(call FindExecutableDirForModule, $$(MODULE)) for the DEST dir in the copy.
* Please adjust the indentation of the big windows/else blocks. We indent everything with 2 spaces in conditional blocks.
* A minor style nit on the copy-and-chmod macro, since it's meant to be used as a recipe, we usually indent those with tabs to indicate that they are recipe lines, in this case, one tab is enough even though the surrounding define should be indented with 2 spaces (don't combine tab and spaces here).
* The (new) SetupJdkExecutable macro handles VERSIONINFO_RESOURCE automatically for you unless you have specific needs. This looks like the defaults should be fine.
* Since this is just for windows, the TOOLCHAIN_LINK_CXX should not make any difference. (It's only needed for GCC to force linking with g++ instead of gcc) So please remove.
* You could consider using FindSrcDirsForComponent for the src dir.

5.) Lib-jdk.packager.gmk:

* The native source files are not organized according to the standards introduced with JEP 201. If they were, most of these SetupJdkLibrary calls would automatically find the correct sources. I realize there is a special case for the windows papplauncherc executable as it's built from the same sources as papplauncher, so that will need a special case. Building of the executables should be moved to Launcher-jdk.packager.gmk however.
* Why are you changing the build output dir and copying debuginfo files around? We have a standard way of building and places where files are put. If that is not working for you, we need to fix it on a global level. Having each native library being built differently is not good for maintainability.
* VERSIONINFO_RESOURCE is handled automatically so does not need to be specified.
* Please indent the full contents of logic blocks with 2 spaces. Not having proper indents makes it much harder to read the code.
* Several lines are too long for future side by side comparisons, please break them up. You can use the # lines as a soft guidance. 

This may be broken up into separate sub-tasks - but initially tracking all with this bug.
Comments
I have implemented #1, #2, #3, #4, some of #5, #7, and #9. #6 has previously been addressed. that leaves the rest of #5, and #8 to be addressed separately #5 is split off as JDK-8213747 #8 is split off as JDK-8213748
12-11-2018

#6 above has been split off as JDK-8213244
01-11-2018

Further feedback - also likely to be broken into separate bugs, but including here: (continuing numbering from description above) 6.) In make/CompileJavaModules.gmk: Do you really need to use GENERATE_JDKBYTECODE_NOWARNINGS? When you add new code to OpenJDK, it really *should* be able to build without warnings. This has previously only been used for legacy code that has not yet been brought up to OpenJDK standards. Introducing new code with warnings silenced feels like a step in the wrong direction. 7.) In make/launcher/Launcher-jdk.packager.gmk: The setup of BUILD_JPACKAGEREXE is only done for windows, but you have "DISABLED_WARNINGS_gcc := unused-result implicit-fallthrough". This does not make sense. The CFLAGS listed look redundant. At the very least I know that -nologo is already present in CXXFLAGS_JDKEXE; I believe the same goes for the rest (or most of the rest) of the flags. Please only add flags if you really need them, since all special configuration/combinations is a potential cause for trouble in the future. This same applies to LDFLAGS. 8.) In src/jdk.packager/unix/scripts/jpackager: This file resides in a non-standard directory. We have a small list of directories that are supposed to come after the $MODULE/share|$OS/ part of the path, and "scripts" is not one of them. While there is no rule "forbidding" new kinds of directories, I strongly recommend against this. Looking more closely at the file, I wonder if you really need it? It's sole purpose seems to be to launch java -m jdk.packager/jdk.packager.main.Main. For that, we have a much better solution. Just change make/launcher/Launcher-jdk.packager.gmk to include the following contents: $(eval $(call SetupBuildLauncher, jpackager, \ MAIN_CLASS := jdk.packager.main.Main, \ )) It will create a "jpackager" binary. Which works for Windows too, so maybe you won't even need that Windows executable, if that too is just a launcher? 9.) In make/lib/Lib-jdk.packager.gmk: The same reservations as for BUILD_JPACKAGEREXE that the CFLAGS and LDFLAGS look redundant. Is there a difference between PACKAGERAPPLAUNCHEREXE_SRC and PACKAGERAPPLAUNCHERCEXE_SRC? I fail to spot it, if so. Are you just compiling the same source twice? Also, the names are *confusingly* similar. Please rename so that the difference is clearer! You don't have to have the PACKAGER or BUILD_PACKAGER prefix. It's just convenience to make sure we don't get name collisions. APP_LAUNCHER and APP_LAUNCHER_CLIENT would work fine.
01-11-2018