Skip to content

Conversation

@fosslinux
Copy link
Contributor

@fosslinux fosslinux commented Dec 21, 2022

This seems like the most reasonable fix, given the structure of __DATE__ normally.

Should this fix be backported?

@bedevere-bot
Copy link

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

@ghost
Copy link

ghost commented Dec 21, 2022

All commit authors signed the Contributor License Agreement.
CLA signed

@fosslinux fosslinux force-pushed the fix-undefined-date branch 4 times, most recently from 96039b8 to af03a4d Compare December 22, 2022 01:11
@arhadthedev
Copy link
Member

@malemburg (as the platform expert), would you mind to review and possibly merge this PR?

Copy link
Member

@iritkatriel iritkatriel left a 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.

@bedevere-bot
Copy link

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 have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@fosslinux
Copy link
Contributor Author

(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 CFLAGS=-U__DATE__. This code path is not used by the preprocessor on any of the automated unit tests, so any unit test I do add will pass both before and after this PR....

@malemburg
Copy link
Member

Please rebase the PR for a review. Thanks.

@iritkatriel
Copy link
Member

(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 CFLAGS=-U__DATE__. This code path is not used by the preprocessor on any of the automated unit tests, so any unit test I do add will pass both before and after this PR....

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.

@malemburg
Copy link
Member

The issue is that the platform.py parser expects the date string to only use \w chars. The "/" in the default string for __DATE__ causes it to fail. A possible unit test for this would to check whether the platform.py parser accepts the new default string "XXX XX XXXX".

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).

@fosslinux
Copy link
Contributor Author

fosslinux commented Feb 5, 2025

On main:

$ ./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')

@fosslinux
Copy link
Contributor Author

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).

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).

@hugovk hugovk changed the title gh-100388: Change undefined __DATE__ to XXX XX XXXX gh-100388: Change undefined __DATE__ to the Unix epoch Feb 6, 2025
Copy link
Member

@malemburg malemburg left a 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

@malemburg
Copy link
Member

@iritkatriel Are you fine with this now ?

@iritkatriel
Copy link
Member

Yes, the bad output was posted in a comment.

@malemburg malemburg merged commit 4f14b7e into python:main Mar 3, 2025
45 of 46 checks passed
@malemburg
Copy link
Member

Thanks, @fosslinux

@fosslinux fosslinux deleted the fix-undefined-date branch March 4, 2025 06:44
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.

5 participants