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
