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

[Jaikiran Pai]

On Mon, 22 Nov 2021 08:25:38 GMT, Alan Bateman <alanb at openjdk.org> wrote:

>> 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.

Done. I have updated the PR to split this line into more than 1 statement.

> 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.

Sounds good. I've updated the PR to replace this to use `FileTime`.

> 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.

That makes sense. The updated PR now has this change.

> 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.

Done. Replaced this and one other place in this test to use `Path.of`.

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

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