Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8258117: jar tool sets the time stamp of module-info.class entries to the current time #5486

Closed
wants to merge 16 commits into from

Conversation

jaikiran
Copy link
Member

@jaikiran jaikiran commented Sep 13, 2021

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.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8258117: jar tool sets the time stamp of module-info.class entries to the current time

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/5486/head:pull/5486
$ git checkout pull/5486

Update a local copy of the PR:
$ git checkout pull/5486
$ git pull https://git.openjdk.java.net/jdk pull/5486/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5486

View PR using the GUI difftool:
$ git pr show -t 5486

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/5486.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Sep 13, 2021

👋 Welcome back jpai! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr label Sep 13, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Sep 13, 2021

@jaikiran The following labels will be automatically applied to this pull request:

  • compiler
  • core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added core-libs compiler labels Sep 13, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Sep 13, 2021

@mlbridge
Copy link

@mlbridge mlbridge bot commented Sep 14, 2021

Mailing list message from Bernd Eckenfels on core-libs-dev:

Is there support for repeatable builds planned? Using the source file might be acceptable, but the class file timestamp could be changing more likely for repeated builds?

--
http://bernd.eckenfels.net
________________________________
Von: core-libs-dev <core-libs-dev-retn at openjdk.java.net> im Auftrag von Jaikiran Pai <jpai at openjdk.java.net>
Gesendet: Monday, September 13, 2021 7:44:22 AM
An: compiler-dev at openjdk.java.net <compiler-dev at openjdk.java.net>; core-libs-dev at openjdk.java.net <core-libs-dev at openjdk.java.net>
Betreff: RFR: 8258117: jar tool sets the time stamp of module-info.class entries to the current time

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.

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

Commit messages:
- 8258117: jar tool sets the time stamp of module-info.class entries to the current time

Changes: https://git.openjdk.java.net/jdk/pull/5486/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=5486&range=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8258117
Stats: 317 lines in 2 files changed: 298 ins; 0 del; 19 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

@mlbridge
Copy link

@mlbridge mlbridge bot commented Sep 14, 2021

Mailing list message from Jaikiran Pai on core-libs-dev:

Hello Bernd,

On 14/09/21 6:10 am, Bernd Eckenfels wrote:

Is there support for repeatable builds planned? Using the source file might be acceptable, but the class file timestamp could be changing more likely for repeated builds?

To clarify the description of my PR, this change uses the timestamp of
the (compiled) module-info.class file that is being added/updated to the
jar.

-Jaikiran

@jaikiran
Copy link
Member Author

@jaikiran jaikiran commented Sep 23, 2021

Any reviews for this one, please?

@mlbridge
Copy link

@mlbridge mlbridge bot commented Sep 25, 2021

Mailing list message from Lance Andersen on core-libs-dev:

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

On Fri, 17 Sep 2021 12:54:07 GMT, Jaikiran Pai <jpai at openjdk.org<mailto: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 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 at home]

Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
Lance.Andersen at oracle.com<mailto:Lance.Andersen at oracle.com>

@mlbridge
Copy link

@mlbridge mlbridge bot commented Sep 25, 2021

Mailing list message from Jaikiran Pai on core-libs-dev:

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Oct 23, 2021

@jaikiran This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@jaikiran
Copy link
Member Author

@jaikiran jaikiran commented Oct 24, 2021

Keep alive.

Copy link
Contributor

@LanceAndersen LanceAndersen left a comment

Hi Jaikiran,

Thank you for your efforts on this. I think the changes look good overall. I want to make one more pass this week, but nothing obvious has jumped out as a concern.

I have also made a couple of mach5 runs without any regressions.

Best
Lance

@jaikiran
Copy link
Member Author

@jaikiran jaikiran commented Nov 17, 2021

Thank you for running the tests Lance. I'll wait for you to complete the review.

Copy link
Contributor

@LanceAndersen LanceAndersen left a comment

Hi Jaikiran,

I am OK with moving forward. You might give it a couple more days before you push in the event we get additional feedback (realizing the PR has been open for a while)

Thank you for your efforts and patience on this.

@openjdk
Copy link

@openjdk openjdk bot commented Nov 19, 2021

@jaikiran This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

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

Reviewed-by: lancea, ihse, alanb

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 1 new commit pushed to the master branch:

  • 0384739: 8276665: ObjectInputStream.GetField.get(name, object) should throw ClassNotFoundException

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Nov 19, 2021
@jaikiran
Copy link
Member Author

@jaikiran jaikiran commented Nov 20, 2021

Hello Lance,

Hi Jaikiran,

I am OK with moving forward.

Thank you for the review.

You might give it a couple more days before you push in the event we get additional feedback (realizing the PR has been open for a while)

Thank you for your efforts and patience on this.

Certainly. I won't integrate this until this next Tuesday. In the meantime, given that this has been open for a while now and new commits have made it to master, I will go ahead and merge the latest master changes just to be sure nothing surprisingly breaks.

moduleInfos.putIfAbsent(name, Files.readAllBytes(f.toPath()));
Long lastModified = f.lastModified() == 0 ? null : f.lastModified();
moduleInfos.putIfAbsent(name,
new StreamedModuleInfoEntry(name, Files.readAllBytes(f.toPath()), lastModified));
Copy link
Contributor

@AlanBateman AlanBateman Nov 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@jaikiran jaikiran Nov 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -1761,14 +1782,28 @@ private File createTemporaryFile(String tmpbase, String suffix) {
static class StreamedModuleInfoEntry implements ModuleInfoEntry {
private final String name;
private final byte[] bytes;
StreamedModuleInfoEntry(String name, byte[] bytes) {
private final Long lastModifiedTime;
Copy link
Contributor

@AlanBateman AlanBateman Nov 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@jaikiran jaikiran Nov 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

ModuleDescriptor md = ModuleDescriptor.read(ByteBuffer.wrap(bytes));
byte[] extended = extendedInfoBytes(md, bytes, packages);
// replace the entry value with the extended bytes
e.setValue(new StreamedModuleInfoEntry(e.getValue().name(), extended, e.getValue().getLastModifiedTime()));
Copy link
Contributor

@AlanBateman AlanBateman Nov 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@jaikiran jaikiran Nov 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

private static final String MODULE_VERSION = "1.2.3";
private static final String UPDATED_MODULE_VERSION = "1.2.4";
private static final String MAIN_CLASS = "jdk.test.foo.Foo";
private static final Path MODULE_CLASSES_DIR = Paths.get("8258117-module-classes", MODULE_NAME).toAbsolutePath();
Copy link
Contributor

@AlanBateman AlanBateman Nov 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use Path.of here.

Copy link
Member Author

@jaikiran jaikiran Nov 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@jaikiran jaikiran Nov 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test continues to pass with all these new changes.

Copy link
Contributor

@LanceAndersen LanceAndersen left a comment

I ran your latest run through Mach5 tier1 - tier3 without any failures.

Your latest updates seem OK to me.

@openjdk
Copy link

@openjdk openjdk bot commented Nov 24, 2021

@jaikiran this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout 8258117
git fetch https://git.openjdk.java.net/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

@openjdk openjdk bot added merge-conflict and removed ready labels Nov 24, 2021
Copy link
Member

@magicus magicus left a comment

Looks good to me.

@openjdk openjdk bot added ready and removed merge-conflict labels Nov 24, 2021
@jaikiran
Copy link
Member Author

@jaikiran jaikiran commented Nov 24, 2021

Thank you Lance and Magnus for the latest reviews and the testing. A recent commit to master in this area meant that there was a merge conflict in this PR. I have now resolved that and pushed the merge to this PR (no other changes). The test continues to pass.
Alan, do these latest round of changes around FileTime usage that you requested, look fine?

Copy link
Contributor

@AlanBateman AlanBateman left a comment

A few minor cleanups but otherwise I think this is good.

ZipEntry e = new ZipEntry(name);
e.setTime(System.currentTimeMillis());
FileTime lastModified = mie.getLastModifiedTime();
e.setLastModifiedTime(lastModified != null ? lastModified : FileTime.fromMillis(System.currentTimeMillis()));
Copy link
Contributor

@AlanBateman AlanBateman Nov 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this would be a bit more readable if it were if-then-else.

try (InputStream is = bytes()) {
return is.readAllBytes();
}
}
Copy link
Contributor

@AlanBateman AlanBateman Nov 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the comment would be clear if it says "the last modified time of the module-info.class".
Also would you mind changing ModuleInfoEntry to use 4-space rather than 2-space indentation?

@jaikiran
Copy link
Member Author

@jaikiran jaikiran commented Nov 25, 2021

Thank you everyone for the reviews.

@jaikiran
Copy link
Member Author

@jaikiran jaikiran commented Nov 25, 2021

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Nov 25, 2021

Going to push as commit a81e4fc.
Since your change was applied there have been 8 commits pushed to the master branch:

  • 26472bd: 8277811: ProblemList vmTestbase/nsk/jdi/TypeComponent/isSynthetic/issynthetic001/TestDescription.java
  • b5841ba: 8277806: 4 tools/jar failures per platform after JDK-8272728
  • e785f69: 8276124: Provide snippet support for properties files
  • 96fe1d0: 8264605: vmTestbase/nsk/jvmti/SuspendThread/suspendthrd003/TestDescription.java failed with "agent_tools.cpp, 471: (foundThread = (jthread) jni_env->NewGlobalRef(foundThread)) != NULL"
  • 077b2de: 8274161: Cleanup redundant casts in jdk.compiler
  • 951247c: 8235876: Misleading warning message in java source-file mode
  • 663e33d: 8272728: javac ignores any -J option in @argfiles silently
  • 0384739: 8276665: ObjectInputStream.GetField.get(name, object) should throw ClassNotFoundException

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Nov 25, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Nov 25, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Nov 25, 2021

@jaikiran Pushed as commit a81e4fc.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler core-libs integrated
4 participants