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

[Mandy Chung]

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