RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v13]

[Severin Gehwolf]

On Tue, 19 Dec 2023 19:14:42 GMT, Mandy Chung <mchung at openjdk.org> wrote:

>> Severin Gehwolf has updated the pull request incrementally with two additional commits since the last revision:
>> 
>>  - Disallow packaged modules and run-time image link
>>  - Only check for existing path when not a scratch task
>>    
>>    When using a run-time image link and the initial build was produced with
>>    the --keep-packaged-modules option, we don't need to check existing
>>    paths to the location where packaged modules need to be copied. This
>>    breaks the --verbose output validation.
>
> FWIW.   [f623930](https://github.com/mlchung/jdk/commit/f623930cd529085ddb730a60b7facf106ea01955) for your reference.   I pulled your branch and refactored and made suggestions to the code while I was walking through the code.   Some observations:
> 
> The constants such as the pathname of the timestamp file and the internal file listing
> per-module non-class non-resource files are part of jlink.  I move the constants to
> `JlinkTask`to follow where `OPTIONS_RESOURCE` is defined.
> 
> `JRTArchive` scans the class and resource files of a given module from the runtime image.
> It should also read `fs_$MODULE_files` to find the list of non-class and non-resource files.
> 
> The current implementation checks if a file is modified lazily when `Entry::stream`
> is called and so it has to remember if file modification has been checked and
> warning has been emitted.   I think doing the file modification check eagerly
> when `collectFiles` is called would simplify the code.
> 
> For maintainability, better to move the reading of and writing to `fs_$MODULE_files`
> together in a single class rather than separated in `JRTArchive` and `JlinkResourcesListPlugin`.
> I move them to `JRTArchive.ResourceFileEntry` for now.   There may be a better place.

@mlchung Thanks for your review and code-cleanup!

> `--unlock-run-image` is changed to be an internal option. It is ok while we should revisit this per the customer feedback.
> If not needed, we should take this out in another release.

OK.

> The help output for this option in `jlink.properties` is no longer needed and should be removed.

OK, removed locally.

> Maybe this option should be `--ignore-modified-runtime` or something explicit.

Sure. I've renamed it to `--ignore-modified-runtime`.

> If I read the code correctly, the image created with this option will enable multi-hops unconditionally? i.e. no timestamp file created and disable the check completely. I think the .stamp file should be present in any image created without packaged modules.

The option is currently used in tests (and can also be used to verify binary equivalence of jlinking Java SE with and without packaged modules), which is a nice property to have. If the stamp file is present in one, but not the other this is sufficient to fail the equivalence test.

> I agree that there is no difference to the plugins of SORTER, PROCESSOR, COMPRESSOR categories if the input to linking is packaged modules vs runtime image.
> 
> I also agree that the transformation done by filtering plugins will persist in the new image if linking from the runtime image.

Good.

> Currently, filtering plugins can be of FILTER and TRANSFORMER category.

Which filtering plugins do you see with category TRANSFORMER? `strip-java-debug-attributes` and `strip-native-debug-symbols` perhaps? Happy to move them to the FILTER category.

> For the remaining plugins (ADDER and TRANSFORMER category) such as `--add-options`, `--system-modules` etc, it sounds right not to persist the transformed data. But a few exceptions:
> 
> ```
>   --vendor-bug-url
>   --vendor-version
>   --vendor-vm-bug-url
> ```
> 
> These plugins change the value of `java.vendor.*` system properties defined in `VersionProps.class`. This behavioral difference when linking with packaged modules and runtime image is hard to notice as those would need the internal knowledge of these plugins (note consider the future plugins too).
>
> I also agree with Alan that --verbose output is a bit confusing (my initial feedback). The image has already been created. The users can't tell which plugins are implicitly run. To list the plugin options, it has to delete the already-created image and re-do the jlink command with `--verbose`.

Are you suggesting to remove it?

> In addition, this change would consider that the default module path now becomes: :$JAVA_HOME/jmods:

I cannot parse this. Do you mean the default module path becoming `$JAVA_HOME` or `$JAVA_HOME/jmods`?

> I wonder if the warning about "$JAVA_HOME/jmods" not present is strictly needed.

I don't think it's needed. We could just mention `Performing a runtime-image-based jlink.` or something like that.

> I am inclined to consider that non-filtering plugins are responsible to restore the data if transformed to the original form. It seems to be simpler model for users to understand. They can expect that the image produced is the same when linking with packaged modules or from the runtime image except filtered data.

Sure.

> The use of `--exclude-resources` plugin to exclude transformed data to restore the data back to original form is clever and works for plugins that add new resources. But it does not work for plugins that modifying existing data (`--vendor-bug-url` etc plugins). The change in `TaskHelper::getPluginsConfig` raises the question that it isn't the right way to support this feature. I think we need to explore a better way to add this support. One possibility I consider is to run non-filtering plugins of ADDER and TRANSFORMER categories always (similar to auto-enabled). It has to determine if it's requested by the user or to restore the data to original form via `Plugin::configure` time. `Plugin::transform` will handle if the data is from packaged-modules and from runtime image which may contain the data transformed by this plugin. I haven't explored this fully yet. More discussion is needed and Alan may have opinions.

So would it be acceptable if I changed those plugins to restore the old behaviour if they were not requested by the user with the respective cli option? Basically, my thinking would be that `Plugin::configure` would gain extra argument so as to determined whether this is a runtime-image based link and triggered by the current jlink run with its CLI option. This should be sufficient to configure for `transform` appropriately.

> A side note: some filtering plugins are of TRANSFORMER plugin. Maybe those plugins should be FILTER category so that plugins not of FILTER category are non-filtering plugins and expected to restore the data to original form if linking from the runtime image.

Could you enumerate the plugins that you think are currently `TRANSFORMER` and should become `FILTER`, please?

-------------

PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-1898495676