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.
gh-127405: Add
ABIFLAGS
tosysconfig.get_config_vars()
on Windows #131799New 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
gh-127405: Add
ABIFLAGS
tosysconfig.get_config_vars()
on Windows #131799Changes from 7 commits
31b7eab
dc45897
9a4586a
76c85bb
b98419b
4729f76
04cbb1c
a6045ea
584e0b0
93257be
30c7b56
97942b2
917874c
f49067e
8fa952b
08d92db
b667dd9
30cf6c7
5df9540
018aae3
4f22ff3
f918241
a4646c5
7b21d7d
1ee8230
084deac
af50632
b397f40
1a82cd1
7990798
0744690
518137b
486e2a8
3bccf1d
780f340
98c26f9
e236bc7
0d8bcdd
278556c
5689c1f
299cec6
21d2e73
d265a59
f3b8570
fe4ea95
da2b4ce
64f0961
8b16454
1c807f0
fbb86f5
b702ff9
932386c
23b6e6c
75b6c51
3c91201
d55b3e6
a084070
d2255e6
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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
_d
is more useful. Provided we keep thet
before it, it means a free-threaded debug build hasABIFLAGS
oft_d
, which meanspython{version}{ABIFLAGS}.exe
will correctly generatepython3.13t_d.exe
. Without the underscore, you can't useABIFLAGS
anywhere, so it's pretty pointless.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
sysconfig
variables 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
sysconfig
variable 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.
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 thed
flag 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
in
test for a single character (and will be replaced bysys.abi_features
), or insert it into certain places without modification.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.h
to 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-timeABIFLAGS
then we'd have to put it in_sysconfig
. And I'm not opposed to constructing anAbiFlags
property inpython.props
and passing that through to_sysconfig
at 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
ABIFLAGS
is 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.h
on 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. ↩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.
Instead of setting
ABIFLAGS
here, I'd prefer to addPy_DEBUG
and then constructABIFLAGS
insysconfig._init_non_posix
.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.
Agree with adding
Py_DEBUG
, but if anything we should constructABIFLAGS
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO,
Py_DEBUG
andPy_GIL_DISABLED
should be inpyconfig.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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.