RFR: 8368333: [lworld] Add preview mode to ImageReader and JRT file-system [v3]

David Beaumont duke at openjdk.org
Wed Oct 8 12:44:01 UTC 2025


On Wed, 24 Sep 2025 21:14:34 GMT, Roger Riggs <rriggs at openjdk.org> wrote:

>> David Beaumont has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Manually deleting ImageReaderFactory (it returned somehow)
>
> src/java.base/share/classes/jdk/internal/jrtfs/JrtFileSystemProvider.java line 125:
> 
>> 123:         } else if (envValue instanceof String && Boolean.parseBoolean((String) envValue)) {
>> 124:             return PreviewMode.ENABLED;
>> 125:         }
> 
> This kind of ambiguous type for the environment variable should not be supported. 
> Pick a type and stick to it.  
> Whereever the jrt: file system is specified will need a description for enablePreview to be added to the spec.

Other file-system flags already do this, so I was just copying that approach. Happy to not do this though.
Which do you suggest, Boolean or String?

> src/java.base/share/classes/jdk/internal/jrtfs/SystemImage.java line 79:
> 
>> 77: 
>> 78:         // TODO: Maybe throw if enablePreview attempted for exploded image?
>> 79: 
> 
> Exploded image is useful for debugging, it could support preview mode using the same META-INF directory structure.

Indeed. I can support it later, I'm just wondering if, in this partially complete state, I should complain or not.
I've got most of the code for that done somewhere or other.

> test/jdk/jdk/internal/jimage/ModuleReferenceTest.java line 228:
> 
>> 226: 
>> 227:     // Encodes strings sequentially starting from index 100.
>> 228:     private static Function<String, Integer> testEncoder() {
> 
> The method name `testEncoder` gives the impression of being a test, but it is just a utility function used by a test.

Goood point. It took me years to stop naming all my unit test methods testXxxx() so I should have spotted this. Thanks.

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

PR Review Comment: https://git.openjdk.org/valhalla/pull/1619#discussion_r2413721287
PR Review Comment: https://git.openjdk.org/valhalla/pull/1619#discussion_r2413724197
PR Review Comment: https://git.openjdk.org/valhalla/pull/1619#discussion_r2413726546


More information about the valhalla-dev mailing list