Looks good to me. We can live with this limitation for a little while, since
there aren't exactly thousands of users who will need to run this
concurrently.
And concurrent use of the command line tool - ie using separate VMs
works fine - per Kevin.
But we do need to get to it.
-phil.
On 5/3/19 11:08 AM, 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