RFR: 8258117: jar tool sets the time stamp of module-info.class entries to the current time [v5]

[Alan Bateman]

On Sat, 20 Nov 2021 01:58:54 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

>> The commit here is a potential fix for the issue noted in https://bugs.openjdk.java.net/browse/JDK-8258117.
>> 
>> The change here repurposes an existing internal interface `ModuleInfoEntry` to keep track of the last modified timestamp of a `module-info.class` descriptor.
>> 
>> This commit uses the timestamp of the `module-info.class` on the filesystem to set the time on the `JarEntry`. There are a couple of cases to consider here:
>> 
>> 1. When creating a jar  (using `--create`), we use the source `module-info.class` from the filesystem and then add extended info to it (attributes like packages, module version etc...). In such cases, this patch will use the lastmodified timestamp from the filesystem of `module-info.class` even though we might end up updating/extending/modifying (for example by adding a module version) its content while storing it as a `JarEntry`. 
>> 
>> 2. When updating a jar (using `--update`), this patch will use the lastmodified timestamp of `module-info.class` either from the filesystem or from the source jar's entry (depending on whether a new `module-info.class` is being passed to the command). Here too, it's possible that we might end up changing/modifying/extending the `module-info.class` (for example, changing the module version to a new version) that gets written into the updated jar file, but this patch _won't_ use `System.currentTimeMillis()` even in such cases.
>> 
>> If we do have to track actual changes that might happen to `module-info.class` while extending its info (in `extendedInfoBytes()`) and then decide whether to use current system time as last modified time, then this will require a bigger change and also a discussion on what kind of extending of module-info.class content will require a change to the lastmodifiedtime of that entry.
>
> Jaikiran Pai has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains seven additional commits since the last revision:
> 
>  - Merge latest from master branch
>  - use testng asserts
>  - Remove "final" usage from test
>  - convert test to testng
>  - Merge latest from master branch
>  - Merge latest from master branch
>  - 8258117: jar tool sets the time stamp of module-info.class entries to the current time

src/jdk.jartool/share/classes/sun/tools/jar/Main.java line 806:

> 804:                     Long lastModified = f.lastModified() == 0 ? null : f.lastModified();
> 805:                     moduleInfos.putIfAbsent(name,
> 806:                             new StreamedModuleInfoEntry(name, Files.readAllBytes(f.toPath()), lastModified));

I'd prefer to see this split into two two statements as there is just too much going on one the one line.

src/jdk.jartool/share/classes/sun/tools/jar/Main.java line 1785:

> 1783:         private final String name;
> 1784:         private final byte[] bytes;
> 1785:         private final Long lastModifiedTime;

It might be better to use a FileTime here, otherwise we risk a NPE when unboxing.

src/jdk.jartool/share/classes/sun/tools/jar/Main.java line 2108:

> 2106:             byte[] extended = extendedInfoBytes(md, bytes, packages);
> 2107:             // replace the entry value with the extended bytes
> 2108:             e.setValue(new StreamedModuleInfoEntry(e.getValue().name(), extended, e.getValue().getLastModifiedTime()));

There are 3 calls to getValue and each one I need to remember that e.getValue is a ModuleInfoEntry. If you add ModuleInfo entry = e.getValue() then it might help the readability.

test/jdk/tools/jar/modularJar/JarToolModuleDescriptorReproducibilityTest.java line 60:

> 58:     private static final String UPDATED_MODULE_VERSION = "1.2.4";
> 59:     private static final String MAIN_CLASS = "jdk.test.foo.Foo";
> 60:     private static final Path MODULE_CLASSES_DIR = Paths.get("8258117-module-classes", MODULE_NAME).toAbsolutePath();

You can use Path.of here.

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

PR: https://git.openjdk.java.net/jdk/pull/5486