Uploaded image for project: 'JDK'
  1. JDK
  2. JDK-8223241

jpackage cleanup from code review



    • Bug
    • Resolution: Fixed
    • P3
    • internal
    • internal
    • tools


      There are several issues that should be addressed to clean up the jpackage code:

      1.) import statements:
      Several files (e.g., Arguments.java) have unused imports that can be removed.
      Except for wildcard static imports, we should replace the wild-card imports with explicit imports.
      for example:
      src/jdk.jpackage/linux/classes/jdk/jpackage/internal/LinuxDebBundler.java (java.util.* and java.io.*)

      2.) In 'share/classes/module-info.java', 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`?

      3.) Should JPackageToolProvider::run log the exception it catches? Although normal error handling shouldn't throw exceptions, it might be useful.

      4.) for jpackage --version, we should output only the version (like jlink) not preceded by "jpackage version: " (like java).

       5.) Add a private constructor to ResourceLocator, and a comment explaining why this empty class is needed or how it is used.

      6.) In Arguments.java: CREATE_APP_IMAGE, and CREATE_INSTALLER should be (default) package-scope instead of public.

      7.) in Arguments.java, the class variable hasAppImage is no longer used and can be removed

      8.) Add class comment to AbstractAppImageBuilder.java

      9.) In AbstractAppImageBuilder, remove 'excludeFileList.add(".*\\.diz");' line. Note then getExcludedFiles() needs to be changed to return null if excludeFileList is an empty ArrayList (instead of empty String).
      We could alternatively remove the excluded file list alltogether since it is now always empty.

      10.) in In AbstractAppImageBuilder.writeCfgFile(), consider using try-with-resources for the 'out" PrintStream.

      11.) in AbstractBundler: IMAGES_ROOT can be (default) package-scope .

      12.) in AbstractImageBundler: Spelling error in comment on line 46, 'intermeadiate" should be "intermediate".

      13.) AbstractImageBundler.extractFlagsFromVersion(): The minimum that jpackage needs to support is JDK 11, so we should be able to assume JEP 322 compliant version numbers. This code could be greatly simplified.

      14.) in BundleParams: remove the comment at line 291 that references FX.

      15.) JLinkBundlerHelper.java: JRE_MODULES_FILENAME and SERVER_JRE_MODULES_FILENAME are unused (obsolete) and should be removed.

      16.) LinuxAppBundler.java: Several places where non-public (package-scope) API is exported publicly; these should all be package-scope itself or else BundlerParamInfo should be public

      17.) LinuxAppImageBuilder.java:createUtf8File is unused (I went looking because I was curious how and why we would use such a method). I see similarly-unused methods of the same name in the Mac and Windows AppImageBuilder classes.

      18.) MacAppImageBuilder.java:Line 818: is the following still needed?
                              || p.toString().contains(
      19.) change type-variable "C" of the conventional "T" in BundleParams.fetchParams()

      20.) remove comment with TODO in StandardBundlerParams

      21.) fix comment at line 32-46 of ValidOptions.java

      22.) update help text for mac-signing-keychain and mac-signing-key-user-name as per javadoc.


        Issue Links



              herrick Andy Herrick (Inactive)
              herrick Andy Herrick (Inactive)
              0 Vote for this issue
              1 Start watching this issue