RFR: JDK-8218055: Use ToolProvider instead of AppRuntimeImageBuilder.
Please review the jpackage fix for bug [1] at [2]. This is a fix for the JDK-8200758-branch branch of the open sandbox repository (jpackage). This enhancement removes the use of jdk.tools.jlink.internal.packager.AppRuntimeImageBuilder in favor of invoking jlink directly thru the ToolProvider interface, with associated code cleanup. [1] https://bugs.openjdk.java.net/browse/JDK-8218055 [2] http://cr.openjdk.java.net/~herrick/8218055/webrev.02 /Andy
On 2/21/19 4:49 PM, Andy Herrick wrote:
Please review the jpackage fix for bug [1] at [2].
This is a fix for the JDK-8200758-branch branch of the open sandbox repository (jpackage).
This enhancement removes the use of jdk.tools.jlink.internal.packager.AppRuntimeImageBuilder in favor of invoking jlink directly thru the ToolProvider interface, with associated code cleanup.
I only skimmed on the patch. A couple of comments: 73 () -> new RuntimeException("link tool not found")); 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. 453 args,add("--bind-services"); s/,/. (does this compile?) jdk.tools.jlink.internal.packager.AppRuntimeImageBuilder class is no longer needed. This should be removed. Mandy
On 2/21/2019 8:54 PM, Mandy Chung wrote:
On 2/21/19 4:49 PM, Andy Herrick wrote:
Please review the jpackage fix for bug [1] at [2].
This is a fix for the JDK-8200758-branch branch of the open sandbox repository (jpackage).
This enhancement removes the use of jdk.tools.jlink.internal.packager.AppRuntimeImageBuilder in favor of invoking jlink directly thru the ToolProvider interface, with associated code cleanup.
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();
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 ?
453 args,add("--bind-services");
sorry - I fixed this between generating and posting the webrev (and forgot to re-generate) - You will see this fix in rev 03 with the other changes.
s/,/. (does this compile?)
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 ? /Andy
Mandy
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
On 2/22/2019 8:06 AM, Mandy Chung wrote:
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.
OK, if you prefer it to go in as part of the jpackage work, then that's fine. -- Kevin
Mandy
revised webrev t address issues brought up by Mandy: [2] http://cr.openjdk.java.net/~herrick/8218055/webrev.03 /Andy On 2/21/2019 8:54 PM, Mandy Chung wrote:
On 2/21/19 4:49 PM, Andy Herrick wrote:
Please review the jpackage fix for bug [1] at [2].
This is a fix for the JDK-8200758-branch branch of the open sandbox repository (jpackage).
This enhancement removes the use of jdk.tools.jlink.internal.packager.AppRuntimeImageBuilder in favor of invoking jlink directly thru the ToolProvider interface, with associated code cleanup.
I only skimmed on the patch. A couple of comments:
73 () -> new RuntimeException("link tool not found"));
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.
453 args,add("--bind-services");
s/,/. (does this compile?)
jdk.tools.jlink.internal.packager.AppRuntimeImageBuilder class is no longer needed. This should be removed.
Mandy
On 2/22/19 9:50 AM, Andy Herrick wrote:
revised webrev t address issues brought up by Mandy:
I only looked at JLinkBundlerHelper.java and the removed files that look okay. Nit: can you use "jlink" lower case in the log/exception message (that's the tool's name). Someone else who knows this area should review. Mandy
I uploaded webrev version 04 (http://cr.openjdk.java.net/~herrick/8218055/webrev.04) which is same as version 03 except "JLink" is changed to "jlink" in several messages. /ANdy On 2/22/2019 2:37 PM, Mandy Chung wrote:
On 2/22/19 9:50 AM, Andy Herrick wrote:
revised webrev t address issues brought up by Mandy:
I only looked at JLinkBundlerHelper.java and the removed files that look okay. Nit: can you use "jlink" lower case in the log/exception message (that's the tool's name).
Someone else who knows this area should review.
Mandy
Hi Andy, Updated version looks fine as well. Thank, Alexander On 2/25/2019 5:19 AM, Andy Herrick wrote:
I uploaded webrev version 04 (http://cr.openjdk.java.net/~herrick/8218055/webrev.04) which is same as version 03 except "JLink" is changed to "jlink" in several messages.
/ANdy
On 2/22/2019 2:37 PM, Mandy Chung wrote:
On 2/22/19 9:50 AM, Andy Herrick wrote:
revised webrev t address issues brought up by Mandy:
I only looked at JLinkBundlerHelper.java and the removed files that look okay. Nit: can you use "jlink" lower case in the log/exception message (that's the tool's name).
Someone else who knows this area should review.
Mandy
Hi Andy, Updated changes looks fine. Thanks, Alexander On 2/22/2019 9:50 AM, Andy Herrick wrote:
revised webrev t address issues brought up by Mandy:
[2] http://cr.openjdk.java.net/~herrick/8218055/webrev.03
/Andy
On 2/21/2019 8:54 PM, Mandy Chung wrote:
On 2/21/19 4:49 PM, Andy Herrick wrote:
Please review the jpackage fix for bug [1] at [2].
This is a fix for the JDK-8200758-branch branch of the open sandbox repository (jpackage).
This enhancement removes the use of jdk.tools.jlink.internal.packager.AppRuntimeImageBuilder in favor of invoking jlink directly thru the ToolProvider interface, with associated code cleanup.
I only skimmed on the patch. A couple of comments:
73 () -> new RuntimeException("link tool not found"));
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.
453 args,add("--bind-services");
s/,/. (does this compile?)
jdk.tools.jlink.internal.packager.AppRuntimeImageBuilder class is no longer needed. This should be removed.
Mandy
participants (4)
-
Alexander Matveev
-
Andy Herrick
-
Kevin Rushforth
-
Mandy Chung