I got most of the way through the platform-independent Java code today.
Here is my feedback so far.
NOTE: I don't think any of these need to be addressed prior to
integration, so you can either fix them in a follow-on webrev or file a
bug for fixing after integration.
GENERAL:
* Several files (e.g., Arguments.java) have unused imports that can be
removed. Maybe this can be done along with expanding wild-card imports.
src/jdk.jpackage/share/classes/module-info.java:
* I see that jdk.jpackage requires java.desktop. It looks like this is
only needed by the Linux installers to get the size of application
icons. If so, then perhaps this dependency could be moved to
`linux/classes/module-info.java.extra`?
src/jdk.jpackage/share/classes/jdk/jpackage/internal/JPackageToolProvider.java:
* Should JPackageToolProvider::run log the exception it catches?
Although normal error handling shouldn't throw exceptions, it might be
useful.
src/jdk.jpackage/share/classes/jdk/jpackage/main/Main.java:
* The following will prefix the version message with "jpackage version "
private static final String version = bundle.getString("MSG_Version")
+ " " + System.getProperty("java.version");
Similar tools such as jlink and jmod don't prefix their output, but
instead simply print the value of the "java.version" property.
src/jdk.jpackage/share/classes/jdk/jpackage/internal/resources/ResourceLocator.java:
* It would be helpful to add a comment to highlight its usage. Also, for
classes such as this where it is not expected that anyone should create
an instance, a private constructor might be useful?
src/jdk.jpackage/share/classes/jdk/jpackage/internal/Arguments.java:
* The following two constants can be (default) package-scope, since it
isn't used outside the package (and if it were, the BundlerParamInfo
class would need to be public in order for it to be useful) :
public static final BundlerParamInfo<Boolean> CREATE_APP_IMAGE =
public static final BundlerParamInfo<Boolean> CREATE_INSTALLER =
* The "hasAppImage" field is set but not used (which might be OK, unless
you meant to use it for argument validation)
src/jdk.jpackage/share/classes/jdk/jpackage/internal/AbstractAppImageBuilder.java:
* This file could use a class comment
* What is the purpose of the following?
excludeFileList.add(".*\\.diz");
If it really is needed, a comment would be helpful.
* The following on line 195 could use try-with-resources:
PrintStream out = new PrintStream(cfgFileName);
src/jdk.jpackage/share/classes/jdk/jpackage/internal/AbstractBundler.java:
* IMAGES_ROOT can be package-scope
src/jdk.jpackage/share/classes/jdk/jpackage/internal/AbstractImageBundler.java:
* Spelling error on line 36
> or as an intermeadiate step in "create-installer" mode.
should be "intermediate"
* In extractFlagsFromVersion, other than throwing an exception, is there
a need to pattern match older JDK versions? The minimum that jpackage
needs to support is JDK 11, so we should be able to assume JEP 322
compliant version numbers. Might this allow for a simplification?
src/jdk.jpackage/share/classes/jdk/jpackage/internal/BundleParams.java:
* I think the following comment is obsolete, since the support for the
"JavaFX-Application-Class" jar manifest entry is no longer in jpackage:
// Note we look for both JavaFX executable jars and regular
executable jars
// As long as main "application" entry point is the same it is main
class
// (i.e. for FX jar we will use JavaFX manifest entry ...)
-- Kevin
