RFR: JDK-8218055: Use ToolProvider instead of AppRuntimeImageBuilder.

[Andy Herrick]

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.
>>
>> [1] https://bugs.openjdk.java.net/browse/JDK-8218055
>>
>> [2] http://cr.openjdk.java.net/~herrick/8218055/webrev.02
>
> 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