FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-119132: Update sys.version to identify free-threaded or not. by corona10 · Pull Request #119134 · python/cpython · GitHub

/ cpython Public

gh-119132: Update sys.version to identify free-threaded or not. - #119134

Merged
corona10 merged 13 commits into
python:mainfrom
corona10:gh-119132
May 18, 2024
Merged

gh-119132: Update sys.version to identify free-threaded or not.#119134
corona10 merged 13 commits into
python:mainfrom
corona10:gh-119132

Conversation

corona10 commented May 17, 2024
edited by bedevere-app Bot
Loading

Copy link
Copy Markdown
Member

corona10 requested review from hugovk and vstinner May 17, 2024 23:54
corona10 changed the title gh-119132: Update buildinfo to identify free-threaded or not. [WIP] gh-119132: Update buildinfo to identify free-threaded or not. May 17, 2024
corona10 marked this pull request as draft May 18, 2024 00:07

nineteendo commented May 18, 2024
edited
Loading

Copy link
Copy Markdown
Contributor

Could we maybe leave out "default" for the normal build? It doesn't really add value:

Python 3.14.0a0 (heads/main-dirty:31a28cbae0, May 17 2024, 17:30:32) [GCC 14.1.1 20240507 (Red Hat 14.1.1-1)]
Python 3.14.0a0 (heads/main-dirty:31a28cbae0, May 17 2024, 17:30:32, free-threading) [GCC 14.1.1 20240507 (Red Hat 14.1.1-1)]

The tests on GitHub don't include "(default)" either. Maybe even like this:

Python 3.14.0a0 (heads/main-dirty:31a28cbae0, May 17 2024, 17:30:32) [GCC 14.1.1 20240507 (Red Hat 14.1.1-1)]
Python 3.14.0a0 (free-threading) (heads/main-dirty:31a28cbae0, May 17 2024, 17:30:32) [GCC 14.1.1 20240507 (Red Hat 14.1.1-1)]

This could even work for --version, but some people might rely on the format:

Python 3.14.0a0
Python 3.14.0a0 (free-threading)

Copy link
Copy Markdown
Contributor

You still need to update this:

cpython/Lib/platform.py

Lines 1156 to 1161 in 81c3130

sys_version_parser = re.compile(
r'([\w.+]+)\s*' # "version<space>"
r'\(#?([^,]+)' # "(#buildno"
r'(?:,\s*([\w ]*)' # ", builddate"
r'(?:,\s*([\w :]*))?)?\)\s*' # ", buildtime)<space>"
r'\[([^\]]+)\]?', re.ASCII) # "[compiler]"

Copy link
Copy Markdown
Contributor

Currently this test is failing:

def test_sys_version(self):

Do you have an opinion on not including "default"?

Copy link
Copy Markdown
Member Author

Do you have an opinion on not including "default"?

Hey, I am under testing. Would you like to leave a comment once I convert the PR into the official PR?

Copy link
Copy Markdown
Contributor

Old regex:

([\w.+]+)\s*\(#?([^,]+)(?:,\s*([\w ]*)(?:,\s*([\w :]*))?)?\)\s*\[([^\]]+)\]?

New regex:

([\w.+]+)\s*\(#?([^,]+)(?:,\s*([\w ]*)(?:,\s*([\w :]*))?)(?:,\s*(free-threading))?\)*\s*\[([^\]]+)\]?

Unmatched string:

2.4.3 (truncation) 
[GCC]

Comment thread Lib/platform.py Outdated
Co-authored-by: Nice Zombies <nineteendo19d0@gmail.com>
Comment thread Lib/platform.py Outdated
Co-authored-by: Nice Zombies <nineteendo19d0@gmail.com>

Copy link
Copy Markdown
Member Author

@nineteendo Thanks for the comment

Still issue (nah I am really bad at regex)

- ('CPython', '2.4.3', '', '', 'truncation) \n', '', 'GCC')
?                                         ----

+ ('CPython', '2.4.3', '', '', 'truncation', '', 'GCC')

Copy link
Copy Markdown
Member

I proposed a similar idea last August, but it was rejected at that time: #108239

Copy link
Copy Markdown
Member

I suggest to mention sys.version in the PR title.

The "(...)" part of sys.version is the Git information, I would prefer to not touch it.

If we change sys.version, I would prefer to "add a new field" in sys.version. For example, add free-threading or [free-threading] after the version. Example:

Python 3.14.0a0 free-threading (heads/main:81c3130c51, May 18 2024, 07:29:44) [GCC 14.0.1 20240411 (Red Hat 14.0.1-0)] on linux

The drawback of changing sys.version is that it breaks tooling parsing it in a strict way.

corona10 commented May 18, 2024
edited
Loading

Copy link
Copy Markdown
Member Author

Then we need to discuss with @hugovk

Copy link
Copy Markdown
Contributor

You accidentally added * after \):

-([\w.+]+)\s*\(#?([^,]+)(?:,\s*([\w ]*)(?:,\s*([\w :]*))?)?(?:,\s*(free-threading))?\)*\s*\[([^\]]+)\]?
+([\w.+]+)\s*\(#?([^,]+)(?:,\s*([\w ]*)(?:,\s*([\w :]*))?)?(?:,\s*(free-threading))?\)\s*\[([^\]]+)\]?

But I would prefer a new field as well.

Comment thread Lib/platform.py Outdated
Comment thread Modules/getbuildinfo.c Outdated
corona10 changed the title [WIP] gh-119132: Update buildinfo to identify free-threaded or not. gh-119132: Update sys.version to identify free-threaded or not. May 18, 2024

Copy link
Copy Markdown
Member Author

@Yhg1s What do you think about backporting it into the 3.13 since it will help the free-threading ecosystem?

Comment thread Lib/platform.py Outdated
Comment thread Python/getversion.c Outdated
Comment thread Python/getversion.c
Comment thread Lib/platform.py Outdated

vstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM, but see my comment on regex.

Co-authored-by: Victor Stinner <vstinner@python.org>
corona10 added the needs backport to 3.13 bugs and security fixes label May 18, 2024
corona10 enabled auto-merge (squash) May 18, 2024 19:20
corona10 merged commit c141d43 into python:main May 18, 2024

Copy link
Copy Markdown

Thanks @corona10 for the PR 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request May 18, 2024
…pythongh-119134)

(cherry picked from commit c141d43)

Co-authored-by: Donghee Na <donghee.na@python.org>

bedevere-app Bot commented May 18, 2024

Copy link
Copy Markdown

GH-119153 is a backport of this pull request to the 3.13 branch.

bedevere-app Bot removed the needs backport to 3.13 bugs and security fixes label May 18, 2024
corona10 added a commit that referenced this pull request May 18, 2024
gh-119134) (#119153)

gh-119132: Update sys.version to identify free-threaded or not. (gh-119134)
(cherry picked from commit c141d43)

Co-authored-by: Donghee Na <donghee.na@python.org>

Copy link
Copy Markdown
Contributor

Sorry, I haven't been paying close attention, but was there a decision not to tweak sys.version_info as well?

>>> sys.version
'3.14.0a0 experimental free-threading build (heads/main:697465ff88e, May 19 2024, 07:40:21) [Clang 15.0.0 (clang-1500.3.9.4)]'
>>> sys.version_info
sys.version_info(major=3, minor=14, micro=0, releaselevel='alpha', serial=0)

It seems that since it's a named tuple there'd be little problem adding a new field which distinguishes the two builds. Something like:

>>> sys.version_info
sys.version_info(major=3, minor=14, micro=0, releaselevel='alpha', serial=0, free_threading=True)

gpshead commented May 19, 2024

Copy link
Copy Markdown
Member

was there a decision not to tweak sys.version_info as well?

build information is not part of version_info. the version string is meant for humans and includes other detailed things that are also not in version_info such as compiler details.

there's the https://docs.python.org/3.13/library/sys.html#sys._is_gil_enabled may-change-in-the-future API for people wanting to know the current status of the GIL regardless of build type.

Copy link
Copy Markdown
Contributor

Thanks. Just checking. As long as there is somewhere to query the status at run-time without parsing a human-readable string.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants


Back | FazBrowse Home | New Git URL