-
-
Notifications
You must be signed in to change notification settings - Fork 30.8k
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
gh-88402: Add new sysconfig variables on Windows (GH-110049) #110049
Conversation
This adds `LIBRARY`, `LDLIBRARY`, `LIBPL`, `SOABI`, and `Py_NOGIL` variables to sysconfig on Windows. Note that `Py_NOGIL` is only defined in `--disable-gil` builds.
Lib/sysconfig.py
Outdated
vars['SOABI'] = soabi | ||
|
||
# e.g., check for a "t" (for "threading") in SOABI | ||
if re.match(r'cp\d+t', soabi): |
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.
The string processing is a bit icky. It would be convenient to just pass these values (SOABI
, Py_NOGIL
) from a C to Python, but it wasn't clear to me where to put that (i.e., in sys
, _imp
, _winapi
or someplace else)
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 agree. Maybe a good option would be putting them on _winapi
and then simply loading them in _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.
sys.flags
?
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.
Isn't sys.flags
for command line flags?
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.
sys.implementation
then? We added these namespaces specifically to be extensible
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.
But this information (SOABI
, Py_NOGIL
, etc) doesn't seem quite right there, I think. Though maybe this is just my interpretation.
I have a prototype locally to export this information in _winapi
, avoiding any public API changes. It doesn't feel amazing either, but 🤷.
Really, this should just be exposed by sysconfig directly, which may involve a C _sysconfig
module that exports this information out to Python, or something else. It's unfortunate that there's such a large discrepancy between how Makefile and Windows builds work 😞. This is kind of a mess, and we should probably try to unify things, it's just... hard, and a lot of work. I'm a bit tired of all this 😕
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 moved some of these to _winapi
. A _sysconfig
C module might also make sense, but it would be very small.
Misc/NEWS.d/next/Library/2023-09-28-12-32-57.gh-issue-88402.hoa3Gx.rst
Outdated
Show resolved
Hide resolved
Co-authored-by: Filipe Laíns <[email protected]>
Modules/_winapi.c
Outdated
[clinic start generated code]*/ | ||
|
||
static PyObject * | ||
_winapi__sysconfig_vars_impl(PyObject *module) |
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 don't like this being here, I'd rather it be in the sys module. It's not a Windows API
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.
What do you think about putting it in a _sysconfig
C module?
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 we're probably past due having one of those. We already smuggle constants into getbuildinfo.c
(git info) and sysmodule.c
(VPATH) on Windows, so may as well put them all into a _sysconfig.c
that is importable.
Make it a built-in module though, not its own .pyd. PC/config.c
lists the modules that are directly linked in, so it'll look like one of those.
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.
You can see the smuggling in PCbuild/pythoncore.vcxproj
(search for GITVERSION=
and VPATH=
). Doesn't necessarily have to be done the same way, but copying it is going to be the easiest way to ensure incremental builds keep working normally.
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've moved the computation to a _sysconfig
C module. Thanks for the pointer to PC/config.c
-- that made it easier. I don't think we need to smuggle data from PCbuild/pythoncore.vcxproj
like we do for GITVERSION
and VPATH
here.
In principle, I'm happy with this change (and if we don't move more build-related variables into I'll need more time to do a thorough review, but if someone else is happy with the details, don't wait for me. |
# Add EXT_SUFFIX, SOABI, and Py_NOGIL | ||
vars.update(_sysconfig.config_vars()) |
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.
(note for the future)
I think we can move this to _init_config_vars
, I am sure it will end up there soon anyway. The only thing to keep in mind is that we allow the POSIX config vars module (eg. _sysconfigdata__linux_x86_64-linux-gnu
) to be overwritten (to help in cross-compilation) by setting _PYTHON_SYSCONFIGDATA_NAME
, and we should raise an error if the value given by the user is different from what we computed.
To be even more cautious, I think we should probably save the original sysconfigdata
name in the C module, as attributes like sys.implementation
, etc., are often monkey patched when cross-compiling. If needed, people working on cross-compilation should also monkey patch the _sysconfig
module explicitly, or better, just monkey patch sysconfig.get_config_vars
.
You don't need to do it in this PR, I just wanted to write this down, especially to document the non-obvious issue with _PYTHON_SYSCONFIGDATA_NAME
.
I don't think relying on Makefile variables is great, for a ton of reasons, meaning the new sysconfig API (GH-103480) will very likely use _sysconfig.config_vars
on other platforms, so I expect the _PYTHON_SYSCONFIGDATA_NAME
issue to be relevant soon.
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.
Thanks @colesbury 😊
I think the PR is in good enough shape to merge.
A possible improvement would be moving the suffix calculation from dynload_*.c
to pycore_internaldl.h
, which would allow us to pull both the SOABI
and EXT_SUFFIX
values directly, without having to add C globals to the module implementation.
Modules/_sysconfig.c
Outdated
if (_PyImport_DynLoadFiletab[0]) { | ||
if (add_string_value(config, "EXT_SUFFIX", _PyImport_DynLoadFiletab[0]) < 0) { | ||
Py_DECREF(config); | ||
return NULL; | ||
} | ||
} |
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.
This is one of the things I wanted to avoid 😅
I think it's probably okay, but one thing to keep in mind is that EXT_SUFFIX
is usually used as the extension to give native modules. We likely want to make sure we are setting it to the one that includes SOABI
, not a different one (eg. the abi3
one). In this case, I think we should probably be okay since we know Windows will always have one, and put it in the first entry.
Co-authored-by: Filipe Laíns <[email protected]>
It should be zero or one and always defined.
@colesbury could you please let me know if you are happy to merge this now, or if you still want to make any changes based on the rest of the review feedback? Any of the mentioned possible improvements could be done later, so I don't want to block this. I'm just asking if you are planning on updating the PR further so that I don't merge it before you have a chance to push the changes. Thanks! |
@FFY00 - I'm happy to make the suggested improvements. |
@FFY00, ok I'm done making the changes from your feedback (assuming the tests pass). Let me know if this matches what you were thinking. |
It looks good, I am gonna schedule it for merging. Thanks again! |
The generated files check seems to be failing due to the clinic output not being up-to-date. |
The C _sysconfig module no longer defines EXT_SUFFIX for POSIX systems
Would it be possible to move the |
Yes, I think that makes sense, particularly, I'd like to align the Windows tags with POSIX. I just haven't gotten around to look deeper into the reason we do it this way currently. |
The tag is actually |
Co-authored-by: Filipe Laíns <[email protected]>
This adds
LIBRARY
,LDLIBRARY
,LIBPL
,SOABI
, andPy_NOGIL
variables to sysconfig on Windows. Note thatPy_NOGIL
is only defined in--disable-gil
builds.