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

[Roger Riggs]

Hi,

Additional comments/observations on the jpackage webrev.
Cleanup opportunities for more maintainable code.


When using the ToolProvider API in an application that has a security 
manager,
what permissions must be available?  Properties, files, ...?


Main:

  91: "processArguments" seems misnamed for the method that does 
*everything*.

BundleParams:

  - 116: Why type-variable "C" instead of the conventional T?

StandardBundlerParam:

  - Many unused imports (according to IntelliJ) including some in the 
same jdk.jpackage.internal that are not needed.

138: TODO and typo in comment.
148:  when does the pathSep get replaced with spaces?  Will that cause 
an issue in re-parsing the string?

661-662: escaped is never set to true,  so escaping is not supported?

747: System.getProperty can require permissions to get properties if run 
under a security manager.

CLIHelp:
  - 58, 65, 72, 80: Indentation of pLaunchOptions does not line up.

IOUtils:
  - 262: why the mix of ProcessBuilder and Runtime.exec  - stick to 
ProcessBuilder

Log:
    "JPACKAGE_DEBUG" environment variable - ? uppercase, documented?

ValidOptions: 46: typo in comment:  "in the a"

VersionExtractor.java seems to be dead code.

Dead code in RelativeFileset:  upshift(), contains(), copy constructor().

Platform.java:
  - should use System Runtime.Version class.

Params.java: Dead code  does not seem to be used in DeployParams
   (and would probably be better as a map than an individual object).
    setParams is unused.

DeployParams:

  - If there are accessor methods, use them!

  - Lots of unused methods. (According to IntelliJ)

  - setSystemWide should use primitive boolean, not Boolean; conversions 
will be done as needed.

  - Ditto installDirChooser

  - Ditto SignBundle flag

Many .java files that pass a map of parameters use the argument name "p" 
but using "params"
would communicate better and be more consistent across the different files.

LinuxDebBundler and LinuxRpmBundler should share more code.

MacAppBundler.java:
  - list of categories will need maintenance to stay in sync with 
Apple.  Can the list be derived from the installed Mac tool chain?
  - or don't validate the argument until it is built and let the mac app 
builder do the checking.

Regards, Roger


On 04/29/2019 05:58 PM, Andy Herrick wrote:
> jpackage reviewers:
>
> We hope to move JEP 343 to "Proposed to Target" next week, so we would 
> like expedite the review process as much as possible.
>