Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34k
gh-91279: ZipFile.writestr now respect SOURCE_DATE_EPOCH#124435
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
Wulian233 commented Sep 24, 2024 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
ghost commented Sep 24, 2024 • edited by ghost
Loading Uh oh!
There was an error while loading. Please reload this page.
edited by ghost
Uh oh!
There was an error while loading. Please reload this page.
Misc/NEWS.d/next/Library/2024-09-24-20-25-52.gh-issue-91279.9oMUwW.rst Outdated Show resolvedHide resolved
Uh oh!
There was an error while loading. Please reload this page.
ZeroIntensity commented Sep 24, 2024
Could you add a test for this? |
Wulian233 commented Sep 24, 2024
Yes, this change should also be synchronized in the tests. Note: https://github.com/python/cpython/blob/main/Lib/test/test_zipfile/_path/test_path.py#L668-L668 |
ZeroIntensity commented Sep 24, 2024
Oh! TIL |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
115adc6 to 6a19ed1CompareUh oh!
There was an error while loading. Please reload this page.
ZeroIntensity left a comment
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.
Uh oh!
There was an error while loading. Please reload this page.
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
b7ca376 to 3126ac3Compare3126ac3 to 81376feCompareWulian233 commented Dec 30, 2024 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
I have made the requested changes; please review again |
Thanks for making the requested changes! @jaraco: please review the changes made to this pull request. |
Lib/zipfile/__init__.py Outdated
| # gh-91279: Set the SOURCE_DATE_EPOCH to a specific timestamp | ||
| epoch=os.environ.get('SOURCE_DATE_EPOCH') | ||
| get_time=int(epoch) ifepochelsetime.time() | ||
| self.date_time=time.gmtime(get_time)[:6] |
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.
I'm a little uneasy that time.localtime() is now replaced by time.gmtime(). That means that the value will change depending on the local time zone of the machine running the code, which may not be adequately exercised in the code. Are there existing tests that validate that gmtime is the correct usage here?
Wulian233Jan 1, 2025 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
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.
I checked the documentation. Should time.gmtime return UTC time and not change with time zones? time.localtime says "Like gmtime() but converts to local time"
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.
What I mean is that this change alters the behavior for all cases. The issue doesn't make mention of gmtime and only mentions localtime in relation to the current implementation. What's the motivation for changing it?
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.
Sorry, I see that I was mistaken. To implement this PR I originally thought it was necessary to change it to gmtime, but in fact, this modification should not have been made😥
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
Wulian233 commented Jan 1, 2025
I have made the requested changes; please review again Sorry again for the inconvenience |
Thanks for making the requested changes! @jaraco: please review the changes made to this pull request. |
5d57959 into python:mainUh oh!
There was an error while loading. Please reload this page.
No
SOURCE_DATE_EPOCHenvironment variable makes pip installing a package generate non-reproducible build artifacts.SOURCE_DATE_EPOCHis a standardised environment variable that distributions can set centrally and have build tools consume this in order to produce reproducible output. In practice, specifies the last modification of something, usually the source code, measured in the number seconds since the Unix epoch, ie. .SOURCE_DATE_EPOCHJanuary 1st 1970, 00:00:00 UTCSee https://reproducible-builds.org/docs/source-date-epoch/
details in issue: #91279