RFR: JDK-8212780: JEP 343: Packaging Tool Implementation
kevin.rushforth at oracle.com
Fri May 3 18:08:29 UTC 2019
Here is the webrev to document the threading limitation:
Once reviewed, Andy can include this in the next version of the webrev
(and we'll update the CSR).
On 5/3/2019 10:23 AM, Kevin Rushforth wrote:
> Thanks for your feedback. I filed two issues  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
>  https://bugs.openjdk.java.net/browse/JDK-8223321
>  https://bugs.openjdk.java.net/browse/JDK-8223322
> On 5/2/2019 8:16 AM, Roger Riggs wrote:
>> 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
>> 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
>> A future improvement:
>> The implementation should be methods on the instance created by the
>> and the logging relative to that instance/delegated where needed.
>> Main can then be a simple lookup of the tool provider and invoke.
>> - 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!
>> 59: Would be more flexible and powerful to use List<String>
>> 67: use arg.startsWith() for cleaner code.
>> 102: Seems wasteful to parse all the arguments twice.
>> jdk.jpackage.internal classes:
>> 57: The constructor does not need to throw IOException
>> 60: are .diz files common enough to preemptively exclude (w/o
>> 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
>> 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
More information about the core-libs-dev