Re: RFR: 8258117: jar tool sets the time stamp of module-info.class entries to the current time [v5]
On Mon, 22 Nov 2021 09:09:37 GMT, Jaikiran Pai wrote:
>> 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`.
Test continues to pass with all these new changes.
-
PR: https://git.openjdk.java.net/jdk/pull/5486
Re: RFR: 8258117: jar tool sets the time stamp of module-info.class entries to the current time [v5]
On Mon, 22 Nov 2021 08:25:38 GMT, Alan Bateman 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
Re: RFR: 8258117: jar tool sets the time stamp of module-info.class entries to the current time [v5]
On Sat, 20 Nov 2021 01:58:54 GMT, Jaikiran Pai 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
Re: RFR: 8258117: jar tool sets the time stamp of module-info.class entries to the current time [v5]
> 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 - Changes: - all: https://git.openjdk.java.net/jdk/pull/5486/files - new: https://git.openjdk.java.net/jdk/pull/5486/files/2c0246f9..945fde6f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5486=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5486=03-04 Stats: 48509 lines in 1041 files changed: 34004 ins; 7147 del; 7358 mod Patch: https://git.openjdk.java.net/jdk/pull/5486.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5486/head:pull/5486 PR: https://git.openjdk.java.net/jdk/pull/5486
