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

[Andy Herrick]

looks good.

/Andy


On 5/3/2019 2:08 PM, Kevin Rushforth wrote:
> 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
>>>
>>
>