On 2/22/19 6:17 AM, Andy Herrick wrote:
On 2/21/2019 8:54 PM, Mandy Chung wrote:
I only skimmed on the patch. A couple of comments:
73 () -> new RuntimeException("link tool not found"));
yes jlink should always exist in the JDK that jpackage is run from - I
just copied this code from jpackage jtreg code, replacing jpackage with
jlink. The orElseThrow arg is unnecessary, the default
NoSuchElementException is as good as this one, will change to:
static final ToolProvider JLINK_TOOL =
ToolProvider.findFirst("jlink").orElseThrow();
OK. I check that jdk.jpackage requires jdk.jlink
s/link/jlink/
Instead of RuntimeException, should this error be localized?
Does jpackage require jdk.jlink? Then this would never reach.
424 Files.deleteIfExists(output); // jlink will re-create
This would fail if output directory is not empty.
yes - windows and linux always pass in an empty (but already created)
directory. Mac (because of the odd layout of an app image:
".../Plugins/Java.runtime/Contents/Home") will pass in a non-existant
directory .
AppRuntimeImageBuilder was tolerant of an empty directory, but jlink
itself isn't.
I had looked into not creating this dir on windows and linux, but that
turned into a mess, since jlink might or might not be invoked, and the
outputDir passed can be one of 3 places (this is linux or windows):
<output>/<name>/runtime - (simple create-image case)
<build-root>/images/<platform>-<installer-type>/<name>/runtime - (simple
create-installer case)
<build-root>/images/<platform>-<installer-type>/<name> -
(create-installer --runtime-installer case)
Do you think I should do something else here or try to clarify this with
a better comment ?
I don't know this code. From what you describe, looks like some lurking
issue.
I think the code path should ensure that the dir does not exist when
you call jlink Or you can jlink with a temporary output directory
and rename it to the destination one.
jdk.tools.jlink.internal.packager.AppRuntimeImageBuilder class is no
longer needed. This should be removed.
I had discussed this with Kevin, because I wasn't sure of the protocol
for removing existing non-exported classes from the runtime, and he
suggested we remove this as a follow-on cleanup bug.
Do you think I should remove with this change ?
You should remove this with your patch as this is an internal API.
Mandy
