On Fri, 29 Aug 2025 15:27:21 GMT, David Beaumont <[email protected]> wrote:
>> Reuses the previously unused package flags in the /packages/xxx entries of
>> jimage files to support preview mode.
>>
>> With these flags it becomes simple to determine things like:
>> 1. Which modules are children of /packages/xxx
>> - This can differ between preview and non-preview mode.
>>
>> 2. Which modules/packages have any preview content
>> - Useful in preview mode for faster rejection testing to avoid
>> double-lookup on all resources.
>>
>> If there are no preview resources built into the jimage file, then the
>> difference between output is that the "isEmpty" flag (old version) has
>> become a "hasContent" flag (but since these flags are not read by anyone
>> right now, this should not be an issue).
>>
>> Note that bumping the minor version number was done to ensure that, during
>> testing, no undiscovered code is reading the new file with old logic (this
>> would work since there's no structural change, but the semantics are
>> different).
>>
>> This does mean that on systems where a 'jimage' binary is installed for the
>> JDK (e.g. /usr/bin/jimage), that tool will stop working on files generate by
>> this code, but the version of that binary in the built JDK will work. The
>> alternative is to just not bump the version number (if nobody is reading the
>> flags now, it shouldn't matter what's in them).
>
> David Beaumont has updated the pull request incrementally with one additional
> commit since the last revision:
>
> More tests and test tweaking for clarity
Looks pretty thorough.
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/ImageResourcesTree.java
line 154:
> 152: * entry to determine if that module contains resources for the
> package.
> 153: *
> 154: * <p>If all entries are marked with {@code IS_PREVIEW_ONLY}
Is there more to this comment?
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/ImageResourcesTree.java
line 197:
> 195: @Override
> 196: public String toString() {
> 197: return String.format("%s [has_normal_content=%s,
> has_preview_content=%s, is_preview_only=%s]",
A bit verbose for output, very few will see or use.
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/ImageResourcesTree.java
line 230:
> 228: // It is possible (but not worth checking for) that these
> flags are the same
> 229: // as the existing reference (e.g. updating with an empty
> preview package).
> 230: unsortedReferences.put(moduleName, new
> PackageReference(moduleName, flags));
is the any cleaner using `compute` or `computeIfPresent`, etc.
test/jdk/tools/jlink/whitebox/jdk.jlink/jdk/tools/jlink/internal/ImageResourcesTreeTest.java
line 61:
> 59: "/java.base/java/util/resource.txt",
> 60: "/java.logging/java/util/logging/LoggingClass.class",
> 61:
> "/java.logging/java/util/logging/OtherLoggingClass.class");
Would these work well using @ParameterizedTest? And other tests below that use
the same lists.
Or should these lists be shared.
-------------
PR Review:
https://git.openjdk.org/valhalla/pull/1537#pullrequestreview-3186990961
PR Review Comment:
https://git.openjdk.org/valhalla/pull/1537#discussion_r2323366727
PR Review Comment:
https://git.openjdk.org/valhalla/pull/1537#discussion_r2323376999
PR Review Comment:
https://git.openjdk.org/valhalla/pull/1537#discussion_r2323389797
PR Review Comment:
https://git.openjdk.org/valhalla/pull/1537#discussion_r2323483229
