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

[Kevin Rushforth]

Here is the webrev to document the threading limitation:

https://bugs.openjdk.java.net/browse/JDK-8223321
http://cr.openjdk.java.net/~kcr/8223321/webrev.00/

Once reviewed, Andy can include this in the next version of the webrev 
(and we'll update the CSR).

-- Kevin


On 5/3/2019 10:23 AM, Kevin Rushforth wrote:
> 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
>
> [1] https://bugs.openjdk.java.net/browse/JDK-8223321
> [2] https://bugs.openjdk.java.net/browse/JDK-8223322
>
>
> 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
>>
>