On Fri, 29 Aug 2025 13:33:11 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:
>
> Minor tidy
Some pre-loaded comments for reviewers.
src/java.base/share/classes/jdk/internal/jimage/ImageHeader.java line 33:
> 31:
> 32: /**
> 33: * Defines the header and version information for jimage files.
Technically optional even though this is a semantic change (it's a change in
data nobody should be reading at the moment).
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/ImageResourcesTree.java
line 48:
> 46: public final class ImageResourcesTree {
> 47: public static boolean isTreeInfoResource(String path) {
> 48: return path.startsWith("/packages/") ||
> path.startsWith("/modules/");
Fixed in passing. Previously this would silently hide/ignore packages with
names starting "packages..." or "modules..." in the jimage tool.
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/ImageResourcesTree.java
line 178:
> 176: private final int flags;
> 177:
> 178: PackageReference(String name, int flags) {
I really don't like the name "PackageReference", since this does not reference
a package, it references a module in which the package exists. I'd be happy to
just refactor the name to something else but didn't want to do too many
"preferential" changes.
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/ImageResourcesTree.java
line 183:
> 181: }
> 182:
> 183: String name() {return name;}
Methods here make the comparator easier to construct.
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]",
I assume nobody was expected to be relying on the `toString()` output.
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/ImageResourcesTree.java
line 206:
> 204:
> 205: // Outside this class, callers should access via modules() /
> moduleCount().
> 206: private final Map<String, PackageReference> unsortedReferences =
> new HashMap<>();
Changing the name to make it clear this is not the preferred way to read
references now. If these classes were in separate sources it would be easier to
encapsulate.
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/ImageResourcesTree.java
line 243:
> 241: private void validate() {
> 242: // If there's a module for which this package has content,
> it should be first and unique.
> 243: if (modules().skip(1).anyMatch(ref -> !ref.isEmpty())) {
With module references sorted, the validation becomes easier.
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/ImageResourcesTree.java
line 498:
> 496: buff.order(writer.getByteOrder());
> 497: pkgNode.modules().forEach(mod -> {
> 498: buff.putInt(mod.flags);
This is the whole point of this change, to write the new flags into the package
entry.
-------------
PR Review:
https://git.openjdk.org/valhalla/pull/1537#pullrequestreview-3168722127
PR Review Comment:
https://git.openjdk.org/valhalla/pull/1537#discussion_r2310152805
PR Review Comment:
https://git.openjdk.org/valhalla/pull/1537#discussion_r2310168105
PR Review Comment:
https://git.openjdk.org/valhalla/pull/1537#discussion_r2310172648
PR Review Comment:
https://git.openjdk.org/valhalla/pull/1537#discussion_r2310177352
PR Review Comment:
https://git.openjdk.org/valhalla/pull/1537#discussion_r2310178660
PR Review Comment:
https://git.openjdk.org/valhalla/pull/1537#discussion_r2310181114
PR Review Comment:
https://git.openjdk.org/valhalla/pull/1537#discussion_r2310183290
PR Review Comment:
https://git.openjdk.org/valhalla/pull/1537#discussion_r2310189310
