-
Notifications
You must be signed in to change notification settings - Fork 233
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
Make DOS timestamp local time zone independent #738
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the PR! This is really helpful. A couple of comments, but I hope it's not too much work.
private/tools/java/rules/jvm/external/jar/AddJarManifestEntry.java
Outdated
Show resolved
Hide resolved
.atZone(ZoneId.systemDefault()) | ||
.toInstant() | ||
.toEpochMilli(); | ||
private static final LocalDateTime DEFAULT_TIMESTAMP = LocalDateTime.of(2010, 1, 1, 0, 0, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really, what we want is the timestamp of a well-known date in UTC. I think the fundamental mistake we made was using LocalDateTime
here. Once we have those millis, we shouldn't need to do any timestamp manipulation. The main problem is we can't use 0
since zip files only encode dates from some time in the 1980s.
7861f91
to
f646a49
Compare
@shs96c sorry for taking some time with this PR. We've cherry picked it into our builds at Spotify and it has resolved the remote caching issues we had. Now we essentially get 100% remote cache hit rate on local builds when building a commit that was already built by the CI agent. Thank you for the review. Please see my comments and let me know what you think. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thank you for your patience and thoughtful replies with this review!
Adds tests replicating and fixes #735.
The
ZipEntry
APIs are pretty limited when it comes to querying and setting timestamps. The getters return the extended timestamp if that is present and MS-DOS timestamp otherwise, but there is no indication of which one was returned.This change ensures that the manifest timestamp is always 2010-01-01, even if the system default changes during runtime. The rest of ZIP entries are copied over entirely so their modification timestamps (both extended and MS-DOS) remain the same. This also means that some other data is copied over which previously wasn't - flags, access time, creation time. I don't think that would be a problem, but I might be missing something.