On 2018-10-24 19:18, Erik Joelsson wrote:
Hello,
Nice to see this finally happening!
Are we actually adding a new demo? I thought we were working towards
getting rid of the demos completely.
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?
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.
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.
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.
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.
On top of Erik's comments, I also have the following to add from a build
perspective:
* 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.
* 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.
* 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?
* 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.
/Magnus
/Erik
On 2018-10-24 07:22, Alan Bateman wrote:
On 23/10/2018 16:49, Andy Herrick wrote:
This patch implements the Java Packager Tool (jpackager) as
described in JEP 343: Packaging Tool
<https://bugs.openjdk.java.net/browse/JDK-8200758>
jpackager is a simple packaging tool, based on the JavaFX
|javapackager| tool, that:
* Supports native packaging formats to give the end user a natural
installation experience. These formats include |msi| and |exe| on
Windows, |pkg| and |dmg| on MacOS, and |deb| and |rpm| on Linux.
* Allows launch-time parameters to be specified at packaging time.
* Can be invoked directly, from the command line, or programmatically,
via the |ToolProvider| API.
Webrev:
http://cr.openjdk.java.net/~herrick/8212780/webrev.01/
cc'ing build-dev as it's important to get it reviewed there.
What is the plan for tests to go with this tool? I see there is one
test in the webrev to do some argument validation but I don't see
anything else right now.
What is the status of the JNLPConverter tool? I see it is included as
a "demo" but maybe it would be better to host somewhere else as this
is for developers migrating Java Web Start applications.
Would it be possible to update the JEP with all the CLI options? That
would be useful for review and also useful for those invoking it with
the ToolProvider API.
If I read the webrev correctly then it adds two modules, one with the
jpackager tool and the other with an API. It would be useful to get a
bit more information on the split. Also I think the name of the API
module and the package that it exports needs discussion to make sure
that the right names are chosen.
-Alan
