On Tue, 28 Nov 2023 23:16:07 GMT, Mandy Chung <mch...@openjdk.org> wrote:
>> Severin Gehwolf has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Tighten ModifiedFilesExitTest
>>
>> Ensure the error message is reasonable and doesn't include
>> Exceptions presented to the user.
>
> src/jdk.jlink/share/classes/jdk/tools/jlink/internal/TaskHelper.java line 452:
>
>> 450: String addOptionsGlob = "glob:" +
>> AddOptionsPlugin.OPTS_FILE;
>> 451: String saveJlinkOptsGlob = "glob:/jdk.jlink/" +
>> JlinkTask.OPTIONS_RESOURCE;
>> 452: String additionalPatterns = systemModulesPattern + "," +
>
> Suggest to have each plugin to define `Plugin::excludeResourcesPattern` to
> return a non-empty string if linking from the run-time image. Construct this
> pattern using the API.
OK. Each plugin returns the pattern it needs to exclude now.
> src/jdk.jlink/share/classes/jdk/tools/jlink/internal/TaskHelper.java line 475:
>
>> 473: // SystemModulesMap class isn't guaranteed to be
>> correct for the
>> 474: // current module set.
>> 475: if (systemModulesPlugin == null) {
>
> Suggest to fail if system modules plugin does not exist which should not
> happen. The `disableFastPath` system property was added for testing only.
OK. `--disable-plugin system-modules` no longer allowed for run-time image
links.
> src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/AddRunImageResourcesPlugin.java
> line 56:
>
>> 54: * resources. Needed for the the run-time image based jlink.
>> 55: */
>> 56: public final class AddRunImageResourcesPlugin extends AbstractPlugin {
>
> This resource file is generated if jdk.jlink is linked in the resulting
> image. Maybe this plugin can be named `GenerateJlinkResourcesListPlugin` or
> `JlinkResourcesListPlugin`???
I went with `JlinkResourcesListPlugin`.
> src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/AddRunImageResourcesPlugin.java
> line 184:
>
>> 182: } catch (RunImageLinkException e) {
>> 183: // RunImageArchive::RunImageFile.content() may throw this
>> when
>> 184: // getting the content(). Propagate this specific exeption.
>
> Suggestion:
>
> // getting the content(). Propagate this specific exception.
Should be fixed.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1414364516
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1414365118
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1414363725
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1414363091
