-
Notifications
You must be signed in to change notification settings - Fork 206
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
Fix non-reproducible archives #304
Fix non-reproducible archives #304
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
@googlebot I signed it! |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
I notice that this appears to be the same comment and line of code from the Gradle source code you linked. The original code is Apache 2.0 licensed, but I'd just check that this is compatible with the Google CLA, which requires you to guarantee that you own the copyright or are authorized to submit to the CLA on someone's behalf. Separately from that, I don't think switching to GregorianCalendar here is ideal. It looks to me like the code in rules_kotlin is wrong, because it's taking a LocalDateTime (which has no offset or zone information, so it's kind of an "abstract" datetime) at a specific point in time, and then converting it to the system default time zone. Which is wrong, since the actual timestamp you'll get then depends on the timezone of the system running it. Then when it goes to an Instant and then milliseconds, that will convert to various values (since the Unix epoch is in UTC, so it will do the offset conversion to that). I don't feel I fully understand the comment, but I gather from it that Java's Jar or Zip handling classes have some weird (or at least different) behavior depending on whether a date is before or after January 1, 1980, and this conflicts with the zip format not allowing dates before 1980. If I understand correctly, the fact that the drifting time zone can cause values before or after this date when you are specifying this exact date at the system time zone, and then trigger different output. But this seems to me like a side-issue from the reproducibility perspective: If it was sending the exact same instant in time into the function, it should be generating deterministic results. So it seems to me like all that needs to change here is that the LocalDateTime's date is changed to something other than January 1, 1980, and due to the zip format, dates before that are unavailable. So I think all that really needs to happen is change the original code to a date after January 1, 1980 (Feb 1 1980 seems fine; I think I've seen Bazel use 2010-01-01 elsewhere), and then change |
Good call out on the potential license issues of copying over the Gradle comment. I'll remove to remove any ambiguities there. I'm not too familiar with Java 8 time APIs so not sure what the implications are of switching to Throughout the main bazel repo it seems there's usages of both styles:
Interestingly, many of the GregorianCalendar usages are susceptible to the same issue... I have a PR out to fix the archive creation in the android tools code as well. I'm more than happy to go with whatever approach is clearer. Maybe the first approach as its more consistent with the original JarHelper file? |
Yeah, I'm actually pretty sure the GregorianCalendar version will work, I just think that's a problematic, oversimplified API that wasn't well-considered originally. It led to the creation of JodaTime to provide much more rigor in the handling of time and dates in Java. Even that had some issues, and its creator thinks that what it eventually evolved into, the The real issue is the time zone. You can use any time zone, as long as it's always the same one (UTC seems like the best pick here, though, since the unix epoch is expressed in UTC). Whether you ask for an offset ( In the original code, it's requesting "whatever the system running this code is using for a time zone" ( |
I had that same concern when I first inspected the code that the usage of system Default was problematic. However, I patched it to use a consistent time zone (UTC) and it caused even stranger results. I believe during the conversion to dos timestamps for the zip file, the mismatching (system vs timestamp) timezones will cause it to get encoded within the zip entry again. I observed that the zip file would still include extra fields on zip entries when inspecting with zipinfo |
Huh. Well, I'm back to not understanding this issue at all again. |
Apologies for the delay, and thanks for doing this! |
TBH, we should probably stop using the Java zip utils. They are a bit on the funky side. |
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.
Looks good (if weird, because dates are weird), other than needing a rebase.
@cgruber I rebased and replaced the comment I grabbed from Gradle's source code just in case. |
I think something went off on the rebase, probably an import statement, as CI seems to be failing on a missing symbol for GregorianCalendar. |
Yep. The import got dropped. :/ |
Happy to merge, @benjaminRomano, once you add back that import statement. |
Woot! Looks great. |
Thanks so much @benjaminRomano |
* upstream/master: Fix non-reproducible archives (bazelbuild#304) Adds a kt_plugin rule (bazelbuild#308) Ensure that KotlionBuilder workers use a clean directory for each compilation. (bazelbuild#298) Apply autoformatting to all files. (bazelbuild#302) Optional outputs (bazelbuild#291) Change plugins to use depsets, as opposed to lists. (bazelbuild#292) Add Corbin to the codeowners. (bazelbuild#293) Update Protobuf to 3.11.3 (bazelbuild#286) Remove tree artifacts (bazelbuild#287) Cleanup src tree (bazelbuild#288) Update README.md (bazelbuild#285) Filter non-kotlin code out of generated sources (bazelbuild#263) Update readme so the dev instructions highlight using a local clone (bazelbuild#283) Remove third_party checked in jars, and properly pull maven dependencies. (bazelbuild#279) Only propagate srcjar if it isn't the default empty jar added in ae70089 to fix bazelbuild/intellij#1616 (bazelbuild#276) Allow resources to be in a kotlin directory (bazelbuild#268)
Fix non-reproducible archives by creating a fixed GregorianCalendar on a given date (slightly later than Jan 1 1980, since Java 7 and 8 have different special casing there).
Background
#301
I noticed cache output inconsistencies between my local and CI builds. I traced it down to the zip library in Java includes the timezone info within the extra field section of each zip entry. This appears to be a problem starting with Java 8 (See comment from Gradle's ZipCopyAction source code for more info: https://github.com/gradle/gradle/blob/master/subprojects/core/src/main/java/org/gradle/api/internal/file/archive/ZipCopyAction.java#L42-L56)
Changes
February 1, 1980
instead to avoid this edge caseTest Plan
Ensure unit tests pass