-
-
Notifications
You must be signed in to change notification settings - Fork 34.5k
gh-127405: Add ABIFLAGS to sysconfig.get_config_vars() on Windows
#131799
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
Merged
Merged
Changes from 1 commit
Commits
Show all changes
58 commits
Select commit
Hold shift + click to select a range
31b7eab
Add `ABIFLAGS` to `sysconfig.get_config_vars()` on Windows
XuehaiPan dc45897
📜🤖 Added by blurb_it.
blurb-it[bot] 9a4586a
Add tests
XuehaiPan 76c85bb
Move `ABIFLAGS` definition to C code
XuehaiPan b98419b
Revert now unrelated changes
XuehaiPan 4729f76
Fix variable name
XuehaiPan 04cbb1c
Refactor string concatination
XuehaiPan a6045ea
Set `Py_DEBUG` flag in sysconfig
XuehaiPan 584e0b0
Update comments from code review
XuehaiPan 93257be
Prefer `Py_DEBUG` over `_DEBUG`
XuehaiPan 30c7b56
Add tests
XuehaiPan 97942b2
Add tests
XuehaiPan 917874c
Merge branch 'main' into windows-add-sysconfig-abiflags
XuehaiPan f49067e
Update tests
XuehaiPan 8fa952b
Update tests
XuehaiPan 08d92db
Merge branch 'main' into windows-add-sysconfig-abiflags
XuehaiPan b667dd9
Remove unnecessary comments
XuehaiPan 30cf6c7
Merge branch 'main' into windows-add-sysconfig-abiflags
XuehaiPan 5df9540
Update What's New
XuehaiPan 018aae3
Merge branch 'main' into windows-add-sysconfig-abiflags
XuehaiPan 4f22ff3
Revert `PCbuild/pyproject.props`
XuehaiPan f918241
Merge branch 'main' into windows-add-sysconfig-abiflags
XuehaiPan a4646c5
Revert `_DEBUG` -> `Py_DEBUG`
XuehaiPan 7b21d7d
Revert `_DEBUG` -> `Py_DEBUG`
XuehaiPan 1ee8230
Add underscore prefix to `d` on Windows
XuehaiPan 084deac
Fix key name
XuehaiPan af50632
Add comments for test for `d` flag in `ABIFLAGS`
XuehaiPan b397f40
Fix failing test
XuehaiPan 1a82cd1
Update tests
XuehaiPan 7990798
Merge branch 'main' into windows-add-sysconfig-abiflags
XuehaiPan 0744690
Merge branch 'main' into windows-add-sysconfig-abiflags
XuehaiPan 518137b
Add a test for `sysconfig.get_config_var('ABIFLAGS')` on Windows
XuehaiPan 486e2a8
Rename test function
XuehaiPan 3bccf1d
Move test location
XuehaiPan 780f340
Move test location
XuehaiPan 98c26f9
Add a note for `test_abiflags`
XuehaiPan e236bc7
Move definition of ABIFLAGS from C to Python
XuehaiPan 0d8bcdd
Merge branch 'main' into windows-add-sysconfig-abiflags
XuehaiPan 278556c
Merge branch 'main' into windows-add-sysconfig-abiflags
XuehaiPan 5689c1f
Merge branch 'main' into windows-add-sysconfig-abiflags
XuehaiPan 299cec6
Update tests
XuehaiPan 21d2e73
Split 't' flag to another test
XuehaiPan d265a59
Update tests
XuehaiPan f3b8570
Update tests
XuehaiPan fe4ea95
Update tests
XuehaiPan da2b4ce
Merge branch 'main' into windows-add-sysconfig-abiflags
XuehaiPan 64f0961
Simplify tests
XuehaiPan 8b16454
Simplify tests
XuehaiPan 1c807f0
Update test comments
XuehaiPan fbb86f5
Merge branch 'main' into windows-add-sysconfig-abiflags
XuehaiPan b702ff9
Merge branch 'main' into windows-add-sysconfig-abiflags
XuehaiPan 932386c
Simplify test comment
XuehaiPan 23b6e6c
Apply suggestions from code review
XuehaiPan 75b6c51
Make 'd' flag test more platform specific
XuehaiPan 3c91201
Merge branch 'main' into windows-add-sysconfig-abiflags
XuehaiPan d55b3e6
Revert non-Windows test changes
XuehaiPan a084070
Fix Windows platform detection
XuehaiPan d2255e6
Merge branch 'main' into windows-add-sysconfig-abiflags
XuehaiPan File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Set
Py_DEBUG flag in sysconfig
- Loading branch information
commit a6045ea23b599eead4e0c22b89bcf70995111ddf
There are no files selected for viewing
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
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
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 think making this
_dis more useful. Provided we keep thetbefore it, it means a free-threaded debug build hasABIFLAGSoft_d, which meanspython{version}{ABIFLAGS}.exewill correctly generatepython3.13t_d.exe. Without the underscore, you can't useABIFLAGSanywhere, so it's pretty pointless.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.
Can't we use something like this?
Explicit is better than implicit.
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.
It isn't "we" - it's "every single user ever for the rest of time". Our job as core maintainers is to do more work and make less obvious decisions so that every single user doesn't have to.
Don't bother quoting the zen at me either. "Special cases aren't special enough" applies just as well here, as does "one obvious way" (which is to use
sysconfigvariables to construct system configuration values).If you want to run with the "explicit > implicit" argument, here's a more explicit version:
Inserting an underscore is neither explicit, nor obvious, and is definitely a special case that isn't special enough to not just use a
sysconfigvariable directly.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 added the underscore prefix in the Python code so that developers can introspect the source more easily.
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.
A constraint of the
_prefix is thedflag should always be the last flag (test added). And users cannot useABIFLAGS.split()to get the components because_is not a valid flag.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.
Users can't split up the value anyway, that's not what it's for. You can parse it specifically, which basically amounts to an
intest for a single character (and will be replaced bysys.abi_features), or insert it into certain places without modification.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 still don't understand why this definition needs to be in the native module, instead of generating it in the Python code.
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.
Ideally, as you say, all these variables should be defined in
pyconfig.h. Let's call that tier 1.1On Windows, we don't parse
pyconfig.hto fill outsysconfig, so instead we set the variables we want in_sysconfig. So that's tier 2, and is closer to the ideal (tier 1). If we had an actual build-timeABIFLAGSthen we'd have to put it in_sysconfig. And I'm not opposed to constructing anAbiFlagsproperty inpython.propsand passing that through to_sysconfigat build-time, but it'd only change the current implementation by a couple of lines.If not set in
_sysconfig, then we are constructing an arbitrary variable purely for cross-platform compatibility reasons, and so we can do that insysconfig(tier 3), but so can anyone who needs it. By definition, if we can construct it in Python code, then so can they, and if they do it then they can backport further than we can (basically the same as the argument for removingdistutils). It's more flexible for libraries that need it to figure it out themselves from first principles, and in reality, I don't think most bother, because it's just not that important a variable.So basically, tier 2 is closer to ideal than tier 3 would be, and importantly it makes it clearer that this is exposing an internally defined value, rather than trying to adhere to some kind of definition of what
ABIFLAGSis meant to be (which doesn't exist). A combination of expressing our intent more clearly along with being in a better position to do it properly one day.Footnotes
They aren't because we don't/shouldn't generate
pyconfig.hon Windows, and we don't rely on the prefix or include directory being duplicated for every ABI group. And we aren't smuggling in a change to that here, it's a 2-3 release deprecation first, which means a PEP describing the transition, and I don't think it's necessary. ↩