Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

gh-88402: Add new sysconfig variables on Windows (GH-110049)#110049

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
FFY00 merged 10 commits intopython:mainfromcolesbury:sysconfig_win
Oct 4, 2023

Conversation

@colesbury
Copy link
Contributor

@colesburycolesbury commentedSep 28, 2023
edited by bedevere-appbot
Loading

This addsLIBRARY,LDLIBRARY,LIBPL,SOABI, andPy_NOGIL variables to sysconfig on Windows. Note thatPy_NOGIL is only defined in--disable-gil builds.

This adds `LIBRARY`, `LDLIBRARY`, `LIBPL`, `SOABI`, and `Py_NOGIL`variables to sysconfig on Windows. Note that `Py_NOGIL` is only definedin `--disable-gil` builds.
vars['SOABI']=soabi

# e.g., check for a "t" (for "threading") in SOABI
ifre.match(r'cp\d+t',soabi):
Copy link
ContributorAuthor

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., insys,_imp,_winapi or someplace else)

Copy link
Member

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

sys.flags?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Isn'tsys.flags for command line flags?

Copy link
Member

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

Copy link
Member

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 😕

colesbury reacted with thumbs up emoji
Copy link
ContributorAuthor

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.

Co-authored-by: Filipe Laíns <filipe.lains@gmail.com>
[clinic start generated code]*/

staticPyObject*
_winapi__sysconfig_vars_impl(PyObject*module)
Copy link
Member

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

Copy link
ContributorAuthor

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?

Copy link
Member

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 intogetbuildinfo.c (git info) andsysmodule.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.

colesbury reacted with thumbs up emoji
Copy link
Member

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 inPCbuild/pythoncore.vcxproj (search forGITVERSION= andVPATH=). 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.

Copy link
ContributorAuthor

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 toPC/config.c -- that made it easier. I don't think we need to smuggle data fromPCbuild/pythoncore.vcxproj like we do forGITVERSION andVPATH here.

FFY00 and zooba reacted with thumbs up emoji
@brettcannonbrettcannon removed their request for reviewOctober 4, 2023 16:30
@colesburycolesbury requested a review froma team as acode ownerOctober 4, 2023 20:10
@zooba
Copy link
Member

In principle, I'm happy with this change (and if we don't move more build-related variables into_sysconfig now, then let's leave an open issue to do it at some point in the future).

I'll need more time to do a thorough review, but if someone else is happy with the details, don't wait for me.

FFY00 reacted with thumbs up emoji

Comment on lines +553 to +554
# Add EXT_SUFFIX, SOABI, and Py_NOGIL
vars.update(_sysconfig.config_vars())
Copy link
Member

@FFY00FFY00Oct 4, 2023
edited
Loading

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 originalsysconfigdata name in the C module, as attributes likesys.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 patchsysconfig.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.

colesbury reacted with thumbs up emoji
Copy link
Member

@FFY00FFY00 left a 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 fromdynload_*.c topycore_internaldl.h, which would allow us to pull both theSOABI andEXT_SUFFIX values directly, without having to add C globals to the module implementation.

Comment on lines 45 to 50
if (_PyImport_DynLoadFiletab[0]) {
if (add_string_value(config,"EXT_SUFFIX",_PyImport_DynLoadFiletab[0])<0) {
Py_DECREF(config);
returnNULL;
}
}
Copy link
Member

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 thatEXT_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 includesSOABI, not a different one (eg. theabi3 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.

zooba reacted with thumbs up emoji
colesburyand others added2 commitsOctober 4, 2023 17:39
Co-authored-by: Filipe Laíns <filipe.lains@gmail.com>
It should be zero or one and always defined.
@FFY00
Copy link
Member

@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!

@colesbury
Copy link
ContributorAuthor

@FFY00 - I'm happy to make the suggested improvements.

@colesbury
Copy link
ContributorAuthor

@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.

FFY00 reacted with thumbs up emoji

@FFY00
Copy link
Member

It looks good, I am gonna schedule it for merging. Thanks again!

colesbury reacted with thumbs up emoji

@FFY00FFY00 changed the titlegh-88402: Add new sysconfig variables on Windowsgh-88402: Add new sysconfig variables on Windows (GH-110049)Oct 4, 2023
@FFY00FFY00enabled auto-merge (squash)October 4, 2023 22:03
@FFY00
Copy link
Member

The generated files check seems to be failing due to the clinic output not being up-to-date.

colesbury reacted with thumbs up emoji

The C _sysconfig module no longer defines EXT_SUFFIX for POSIX systems
@FFY00FFY00 merged commitcf6f23b intopython:mainOct 4, 2023
@vstinner
Copy link
Member

#define PYD_TAGGED_SUFFIX PYD_DEBUG_SUFFIX "." PYD_SOABI ".pyd"

Would it be possible to move thed tag as part of the "SOABI" flags? Maybe it's time to add sys.abiflags to Windows?

@FFY00
Copy link
Member

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.

@zooba
Copy link
Member

The tag is actually_d and not justd. I think we need a deprecation cycle to change the naming convention here (and there are going to be a significant number of flow on changes in the launcher and various tests, so be prepared for a bit more work than just one define).

@colesburycolesbury deleted the sysconfig_win branchOctober 6, 2023 15:40
Glyphack pushed a commit to Glyphack/cpython that referenced this pull requestSep 2, 2024
Co-authored-by: Filipe Laíns <filipe.lains@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@FFY00FFY00FFY00 approved these changes

@ericsnowcurrentlyericsnowcurrentlyAwaiting requested review from ericsnowcurrentlyericsnowcurrently is a code owner

@ncoghlanncoghlanAwaiting requested review from ncoghlanncoghlan is a code owner

@warsawwarsawAwaiting requested review from warsawwarsaw is a code owner

@kumaraditya303kumaraditya303Awaiting requested review from kumaraditya303kumaraditya303 is a code owner

@zoobazoobaAwaiting requested review from zooba

Assignees

No one assigned

Labels

3.13bugs and security fixesOS-windowstopic-sysconfig

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@colesbury@zooba@FFY00@vstinner

[8]ページ先頭

©2009-2025 Movatter.jp