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

[Andy Herrick]

On 5/7/2019 4:39 PM, Roger Riggs wrote:
> 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*.
I quite agree. Perhaps this can be done in conjunction with JDK-8223322 
<https://bugs.openjdk.java.net/browse/JDK-8223322> which will revise the 
flow of Main and ToolProvider.
(adding comment in JDK-8223322) 
<https://bugs.openjdk.java.net/browse/JDK-8223322>
>
> BundleParams:
>
>  - 116: Why type-variable "C" instead of the conventional T?
I don't know the history of this, but it seems reasonable to change to 
"T" like other classes.
(added as a line item 19 in JDK-8223241 
<https://bugs.openjdk.java.net/browse/JDK-8223241>)
>
> StandardBundlerParam:
>
>  - Many unused imports (according to IntelliJ) including some in the 
> same jdk.jpackage.internal that are not needed.
already listed as item 1 in JDK-8223241 
<https://bugs.openjdk.java.net/browse/JDK-8223241>
>
> 138: TODO and typo in comment.
remove todo - this has been tested
(added line item 20 in JDK-8223241 
<https://bugs.openjdk.java.net/browse/JDK-8223241>)

> 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?
need to look into this - (added to JDK-8223334)
>
> 747: System.getProperty can require permissions to get properties if 
> run under a security manager.
Many of the operations performed by jpackage too would require 
all-permissions if run under security manager.
>
> CLIHelp:
>  - 58, 65, 72, 80: Indentation of pLaunchOptions does not line up.
I don't see what you mean here.  Looks lined up to me
>
> IOUtils:
>  - 262: why the mix of ProcessBuilder and Runtime.exec  - stick to 
> ProcessBuilder
added to this case to JDK-8223334
>
> Log:
>    "JPACKAGE_DEBUG" environment variable - ? uppercase, documented?
implemented as strictly upper case, what do we have to do to document ?
>
> ValidOptions: 46: typo in comment:  "in the a"
several problems with this comment - added to JDK-8223241 
<https://bugs.openjdk.java.net/browse/JDK-8223241>
>
> 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.
new JBS issue to remove dead code. (JDK-8223586 
<https://bugs.openjdk.java.net/browse/JDK-8223586>)

>
> 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.
>
All above added to JDK-8223586 
<https://bugs.openjdk.java.net/browse/JDK-8223586>

/Andy

> 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.
>>
>