gh-127405: Add ABIFLAGS to sysconfig.get_config_vars() on Windows - #131799
gh-127405: Add ABIFLAGS to sysconfig.get_config_vars() on Windows#131799zooba merged 58 commits into
Conversation
Sorry, something went wrong.
|
|
||
| // 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.
Sorry, something went wrong.
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.
Sorry, something went wrong.
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.
Sorry, something went wrong.
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.
Sorry, something went wrong.
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.
Sorry, something went wrong.
merwok
left a comment
There was a problem hiding this comment.
LGTM – waiting for Steve’s approval
Sorry, something went wrong.
zooba
left a comment
There was a problem hiding this comment.
Nearly there! I just want to see that unspecified test clarified/removed, and we'll run it across all the platforms just to make sure the other test change is valid for them all.
Sorry, something went wrong.
| 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.
Sorry, something went wrong.
|
🤖 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. |
Sorry, something went wrong.
|
Looks like the buildbots were fine apart from the darwin ones, which has been fixed. So I'll merge it and we should keep an eye on any more failures (though I'm about to be AFK for the weekend and won't actually be watching... will look on Monday!) |
Sorry, something went wrong.
|
Thanks for the contribution! And for bearing with me to make sure we get it right. |
Sorry, something went wrong.
Set an upper-cased ABIFLAGS (previously not exist) to sysconfig.get_config_vars() on Windows.
The lower-cased abiflags in sysconfig.get_config_vars() remains an empty string on Windows.
See #127405 (comment).