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

Ensure that the timezone doesn't leak into zips. #8

Merged
merged 3 commits into from
Feb 2, 2024

Conversation

thatch
Copy link
Contributor

@thatch thatch commented Jan 28, 2024

Hello from a fellow connoisseur of zip files. I saw a new release of this project and wondered how you did it, and found one source of non-deterministic builds in that the system timezone affects how SOURCE_DATE_EPOCH is used.

I'm not able to run the tests on CI in my fork, so I don't know if Windows is passing with this test. I'm a little concerned that "Availability: Unix" in the docs for time.tzset means the function doesn't even exist, but I'm hoping it does exist and is a no-op. CI will tell.

When using SOURCE_DATE_EPOCH, previous code was using time.localtime
which is affected by the system timezone.  The test code here with tzset
likely does not demonstrate the problem on Windows, but hopefully is
just a no-op there.
Copy link

codecov bot commented Jan 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (be33cff) 90.7% compared to head (660a6d4) 90.6%.
Report is 3 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##            main      #8     +/-   ##
=======================================
- Coverage   90.7%   90.6%   -0.1%     
=======================================
  Files          2       2             
  Lines        162     161      -1     
=======================================
- Hits         147     146      -1     
  Misses        15      15             
Files Coverage Δ
repro_zipfile.py 86.9% <100.0%> (ø)

... and 1 file with indirect coverage changes

@jayqi
Copy link
Member

jayqi commented Jan 29, 2024

Ahhh, thanks for the catch! I definitely did not pay enough attention when I looked up how to convert from epoch seconds to a struct_time object.

I approved this PR to run the CI, and unfortunately, it looks like time.tzset indeed is not available for Windows. Based on this StackOverflow post, I don't think there's an alternative way to change the timezone either. I'm fine with just making the timezone change a no-op on Windows and skip testing it for Windows.

@jayqi jayqi self-requested a review February 2, 2024 06:17
Copy link
Member

@jayqi jayqi left a comment

Choose a reason for hiding this comment

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

Thanks, @thatch! I fixed the Windows issue.

@jayqi jayqi merged commit 7bc64a8 into drivendataorg:main Feb 2, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants