gh-127405: Add ABIFLAGS to sysconfig.get_config_vars() on Windows
#131799
Conversation
Modules/_sysconfig.c
Outdated
|
|
||
| // On Unix, the `ABIFLAGS` key is defined via a different logic. | ||
| // | ||
| // Emulate `sys.abiflags` value on Unix for Windows. ABIFLAGS here is only | ||
| // an emulated value. It is not present during build on Windows. | ||
| if (add_string_value(config, "ABIFLAGS", | ||
| # ifdef Py_GIL_DISABLED | ||
| "t" | ||
| # endif | ||
| # ifdef _DEBUG | ||
| "d" | ||
| # endif | ||
| "") | ||
| < 0) { | ||
| Py_DECREF(config); | ||
| return NULL; | ||
| } |
There was a problem hiding this comment.
Instead of setting ABIFLAGS here, I'd prefer to add Py_DEBUG and then construct ABIFLAGS in sysconfig._init_non_posix.
There was a problem hiding this comment.
Agree with adding Py_DEBUG, but if anything we should construct ABIFLAGS further back in the build (e.g. in a .props file) and store that value here.
We don't need or use it, though, so I don't see an issue with constructing it here. But I'd rather have build variables be closer to the build rather than further away.
There was a problem hiding this comment.
IMO, Py_DEBUG and Py_GIL_DISABLED should be in pyconfig.h, removing the need for a native module in the first place.
But ignoring that, I think it would be more maintainable to have all variables that need construction in the same place. Having part of them here, and part of them in the Python module should be avoidable if we can.
Especially for Windows specific variables emulating POSIX ones, sysconfig._init_non_posix is the place I would expect them to be.
There was a problem hiding this comment.
I do not have a strong preference for whether the code should live in Python or C. Both are fine for me.
There was a problem hiding this comment.
I do not have a strong preference for whether the code should live in Python or C. Both are fine for me.
I move the definition from C to Python in the latest commit. I can either keep it or revert it if we reach a consensus.
XuehaiPan
requested review from
a team,
ericsnowcurrently,
brettcannon,
ncoghlan and
warsaw
as code owners
Lib/test/test_sysconfig.py
Outdated
| def test_abi_debug(self): | ||
| ABIFLAGS = sysconfig.get_config_var('ABIFLAGS') | ||
| if support.Py_DEBUG: | ||
| # The 'd' flag should always be the last one. |
There was a problem hiding this comment.
I still don't know if it's guaranteed.
Show me the definition of ABIFLAGS that specifies it (trick question - no such definition exists).
Let's not invent a specification in a barely-related PR. Make this a Windows-specific test that checks for _d at the end (or just check 'd' in ABIFLAGS), but don't touch other platforms.
zooba
added
the
🔨 test-with-buildbots
|
🤖 New build scheduled with the buildbot fleet by @zooba for commit d55b3e6 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F131799%2Fmerge If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again. |
bedevere-bot
removed
the
🔨 test-with-buildbots
|
Looks like the buildbots were fine apart from the |
|
Thanks for the contribution! And for bearing with me to make sure we get it right. |
Set an upper-cased
ABIFLAGS(previously not exist) tosysconfig.get_config_vars()on Windows.The lower-cased
abiflagsinsysconfig.get_config_vars()remains an empty string on Windows.See #127405 (comment).
sys.abiflagson Windows #127405