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

[Kevin Rushforth]

Thanks for your feedback. I filed two issues [1][2] for the thread 
concurrency issue. The first one needs to be solved for JDK 13, which is 
to either document the existing limitation (which is probably what we'll 
do) or serialize access by synchronizing on the JPackageToolProvider 
class (or, equivalently, making the Main::run methods synchronized). The 
second one is the improvement to allow concurrent execution of two 
instances of the tool, which can be done for a future version.

The rest of the comments can be added as cleanup items (either a new 
issue or to one of the existing issues).

-- Kevin


On 5/2/2019 8:16 AM, Roger Riggs wrote:
> Hi,
>
> Some comments, a initial batch...
>
> Having support for the ToolProvider is great.
> However, there is no indication about whether it is valid to use it 
> from multiple threads.
> The implementation is structured to be deliberately single thread use 
> only
> with the invocation being via a static method and the logging being 
> via static methods.
> There will need to be a disclaimer and perhaps an exception should be 
> thrown.
>
> A future improvement:
> The implementation should be methods on the instance created by the 
> ToolProvider
> and the logging relative to that instance/delegated where needed.
> Main can then be a simple lookup of the tool provider and invoke.
>
> jpackage.main.Main:
>
>  - Main.run returns -1, which then is used as exit status, -1 is not 
> the usual exit status for a C/Unix main.
>
> 65,  the run() method is usually not static and should be re-named to 
> avoid confusion
>
> 92: Implies something should be logged on a failure.
>
> 65: Run (Pw, Pw, args...) doesn't use try ... finally to ensure log is 
> flushed.
>
> 65: 'throws Exception' implies it can throw an exception but is 
> ambiguous as to whether
> it returns an error number or throws on what kinds of errors?
>
> 91: Ambiguous as to whether processArguments() throws or return false!
>
> CommandLine:
> 59: Would be more flexible and powerful to use List<String> 
> consistently...
>
> 67: use arg.startsWith() for cleaner code.
>
> 102: Seems wasteful to parse all the arguments twice.
>
> jdk.jpackage.internal classes:
>
> AbstractAppImageBuilder:
>  57: The constructor does not need to throw IOException
>
> 60:  are .diz files common enough to preemptively exclude (w/o 
> documentation)
>
> 89: Can the mix of old File API and new Path/Files APIs be avoided?
>
> Adding javadoc to the abstract builder methods will help future 
> maintainers.
>
> 203: is a bit more generous than most CLASSPATH parsing and might lead 
> to non-obvious bugs.
>    For example, a path component with a space in it!
>
> 229:  There is no mention of debugging support in JEP 343.
>   Where is this functionality defined or is it to be identified as 
> undocumented internal implementation
>
> Regards, Roger
>