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

[Kevin Rushforth]

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