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

[Jaikiran Pai]

On Fri, 17 Sep 2021 12:54:07 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 two additional 
> commits since the last revision:
> 
>  - Merge latest from master branch
>  - 8258117: jar tool sets the time stamp of module-info.class entries to the 
> current time

Keep alive.

-

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 [v2]

[Jaikiran Pai]

On 25/09/21 4:17 pm, Lance Andersen wrote:


Hi Jaikiran

This is on my todo list, sorry for the delay.

Hoping we can get a couple additional eyes on this as well.


Thank you Lance. Please take your time.


-Jaikiran

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

[Lance Andersen]

On Sep 23, 2021, at 2:28 AM, Jaikiran Pai 
mailto:j...@openjdk.java.net>> wrote:

On Fri, 17 Sep 2021 12:54:07 GMT, Jaikiran Pai 
mailto:j...@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 two additional commits since the 
last revision:

- Merge latest from master branch
- 8258117: jar tool sets the time stamp of module-info.class entries to the 
current time

Any reviews for this one, please?

Hi Jaikiran

This is on my todo list, sorry for the delay.

Hoping we can get a couple additional eyes on this as well.

Best
Lance

-

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

[cid:E1C4E2F0-ECD0-4C9D-ADB4-B16CA7BCB7FC@home]



Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com

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

[Jaikiran Pai]

On Fri, 17 Sep 2021 12:54:07 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 two additional 
> commits since the last revision:
> 
>  - Merge latest from master branch
>  - 8258117: jar tool sets the time stamp of module-info.class entries to the 
> current time

Any reviews for this one, please?

-

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 [v2]

[Jaikiran Pai]

> 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 two additional commits since the 
last revision:

 - 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/e2193081..d91f02a3

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=5486=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5486=00-01

  Stats: 5483 lines in 274 files changed: 3475 ins; 1249 del; 759 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