RFR: JDK-8212780: JEP 343: Packaging Tool Implementation

[Andy Herrick]

I filed JDK-8223241 <https://bugs.openjdk.java.net/browse/JDK-8223241> 
to address each of these issues, and included import statement wildcard 
usage as suggested (removing it from JDK-8223189 
<https://bugs.openjdk.java.net/browse/JDK-8223189>)

/Andy


On 5/1/2019 7:17 PM, Kevin Rushforth wrote:
> 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
>