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

[Kevin Rushforth]

OK, thanks.

-- Kevin

On 5/3/2019 11:31 AM, Roger Riggs wrote:
> Hi Kevin,
>
> I'm fine with the disclaimer; undefined behavior is fine.
>
> Thanks, Roger
>
> p.s. synchronizing the run method would prevent filing of issues when
> someone does it anyway, keeping the noise to a minimum.
>
>
> On 05/03/2019 02: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
>>>>
>>>
>>
>