-
-
Notifications
You must be signed in to change notification settings - Fork 33.7k
gh-100388: Change undefined __DATE__ to the Unix epoch #100389
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
Conversation
|
Most changes to Python require a NEWS entry. Please add it using the blurb_it web app or the blurb command-line tool. |
96039b8 to
af03a4d
Compare
|
@malemburg (as the platform expert), would you mind to review and possibly merge this PR? |
iritkatriel
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.
Please add a unit test that fails without this patch and passes with it, so we can understand what you are changing and why, and so that the issue you are fixing will not be accidentally re-introduced in the future.
|
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 |
|
(I completely forgot about this PR, and was reminded of it today! Apologies) @iritkatriel I'm a bit confused regarding how I am meant to create a unit test for this. This is a bug that only arises when Python (namely, the getbuildinfo module) is compiled with |
|
Please rebase the PR for a review. Thanks. |
I understand. If you could just post here screenshots of before and after from your local build that would help us by creating a record of what the problem was. |
|
The issue is that the platform.py parser expects the date string to only use \w chars. The "/" in the default string for Now, regarding the default string itself, I think it would be better to use a valid date string, since "XXX XX XXXX" will make platform.py's parser happy, but fail to parse as a date string downstream (in code using the platform.py python_build() API). "Jan 01 1970" would work (the Unix epoch). |
|
On $ ./python
Python 3.14.0a4+ (remotes/upstream/main:cdcacec79f, xx/xx/xx, xx:xx:xx) [GCC 14.2.1 20241221] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import platform
>>> platform._sys_version()
Traceback (most recent call last):
File "<python-input-4>", line 1, in <module>
platform._sys_version()
~~~~~~~~~~~~~~~~~~~~~^^
File "/home/user/projects/cpython/Lib/platform.py", line 1201, in _sys_version
raise ValueError(
'failed to parse CPython sys.version: %s' %
repr(sys_version))
ValueError: failed to parse CPython sys.version: '3.14.0a4+ (remotes/upstream/main:cdcacec79f, xx/xx/xx, xx:xx:xx) [GCC 14.2.1 20241221]'On this branch: $ ./python
Python 3.14.0a4+ (heads/fix-undefined-date:5d72c723dc, Jan 01 1970, 00:00:00) [GCC 14.2.1 20241221] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import platform
>>> platform._sys_version()
('CPython', '3.14.0a4+', 'heads/fix-undefined-date', '5d72c723dc', 'heads/fix-undefined-date:5d72c723dc', 'Jan 01 1970 00:00:00', 'GCC 14.2.1 20241221') |
No objections to that. Done. Also, to keep it consistent, I changed the default time string from "xx:xx:xx" to "00:00:00" (so now the default is Unix epoch). |
c22cf83 to
5d72c72
Compare
malemburg
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 now. Thanks @fosslinux
|
@iritkatriel Are you fine with this now ? |
|
Yes, the bad output was posted in a comment. |
|
Thanks, @fosslinux |
This seems like the most reasonable fix, given the structure of
__DATE__normally.Should this fix be backported?