Fix non-reproducible archives

[benjaminRomano]

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

• Switch to using February 1, 1980 instead to avoid this edge case

Test Plan
Ensure unit tests pass

[googlebot]

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 @googlebot I signed it! and we'll verify it.

---

What to do if you already signed the CLA

Individual signers

• It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

• Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
• The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
• The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

[benjaminRomano]

@googlebot I signed it!

[googlebot]

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

[davidsantiago]

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 .atZone(ZoneId.systemDefault()) to .atOffset(ZoneOffset.UTC) so that UTC is used every time instead of whatever the system happens to be using.

[benjaminRomano]

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.atOffset(ZoneOffset.UTC) from .atZone(ZoneId.systemDefault() in addition to adjusting the date.

Throughout the main bazel repo it seems there's usages of both styles:

• Using the original method with the date 2010-01-01 instead (For example the original JarHelper implementation)
• Usages of Gregorian Calendar

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?

[davidsantiago]

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 java.time package, is even better. Anyways, date and time handling is incredibly complicated and subtle, as we are seeing in this issue, so my instinct is just to stick to the more carefully considered version. I will also argue that the java.time version makes it much easier to see what exactly is being requested in the timestamp, since it's makes explicit statements about date, time, and zone, while GregorianCalendar kind of papers a bunch of issues over.

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 (.atOffset) or a zone (.atZone) is largely a matter of how complicated you want to make things: an offset is just the hours to add to UTC, while a zone incorporates complicated logic about daylight savings and other things to map an administrative time zone to an actual offset to UTC.

In the original code, it's requesting "whatever the system running this code is using for a time zone" (.atZone(ZoneId.systemDefault())), so even if a build is run at the exact same instant on identical machines in different time zones, they'll generate different artifacts. That seems like a reproducibility problem no matter what, so I find it kind of worrisome that the main JarHelper code in Bazel is done this way. Maybe I'm missing something that mitigates this.

[benjaminRomano]

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

[davidsantiago]

Huh. Well, I'm back to not understanding this issue at all again.

[restingbull]

Apologies for the delay, and thanks for doing this!

[restingbull]

Huh. Well, I'm back to not understanding this issue at all again.

TBH, we should probably stop using the Java zip utils. They are a bit on the funky side.

[cgruber]

Looks good (if weird, because dates are weird), other than needing a rebase.

[benjaminRomano]

@cgruber I rebased and replaced the comment I grabbed from Gradle's source code just in case.

[cgruber]

I think something went off on the rebase, probably an import statement, as CI seems to be failing on a missing symbol for GregorianCalendar.

[cgruber]

Yep. The import got dropped. :/

[cgruber]

Happy to merge, @benjaminRomano, once you add back that import statement.

[cgruber]

Woot! Looks great.

[cgruber]

Thanks so much @benjaminRomano