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

[Kevin Rushforth]

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