Skip to content

gh-133779: Revert Windows generation of pyconfig.h and go back to a static header. #133966

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

Merged
merged 7 commits into from
May 19, 2025

Conversation

zooba
Copy link
Member

@zooba zooba commented May 13, 2025

This means that extension builders will need to specify Py_GIL_DISABLED if they want to link to the free-threaded builds.

…to a static header.

This means that extension builders will need to specify Py_GIL_DISABLED if they want to link to the free-threaded builds.
@zooba zooba requested a review from FFY00 as a code owner May 13, 2025 12:46
@zooba
Copy link
Member Author

zooba commented May 16, 2025

!buildbot .windows.

@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @zooba for commit 142b60a 🤖

Results will be shown at:

https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F133966%2Fmerge

The command will test the builders whose names match following regular expression: .*windows.*

The builders matched are:

  • AMD64 Windows11 Refleaks PR
  • AMD64 Windows PGO NoGIL PR
  • ARM64 Windows Non-Debug PR
  • AMD64 Windows Server 2022 NoGIL PR
  • AMD64 Windows11 Non-Debug PR
  • AMD64 Windows10 PR
  • AMD64 Windows11 Bigmem PR
  • ARM64 Windows PR
  • AMD64 Windows PGO PR

@zooba
Copy link
Member Author

zooba commented May 17, 2025

@encukou You'll probably be interested in the test_cext failure, as it relates to _Py_ALIGN_AS raising warning C5274 on MSVC. Details at https://learn.microsoft.com/en-us/cpp/overview/cpp-conformance-improvements?view=msvc-170#application-of-_alignas-on-a-structured-type-in-c

In short, I think it's one we can suppress for our entire header, which I'll do here to unblock this release blocking PR. You may have ideas/preferences about how best to structure that sort of thing, so feel free to change it later.

(FWIW, my guess is that the test previously wasn't handling Py_GIL_DISABLED properly and now is, since the only use of _Py_ALIGN_AS is when building free-threaded.)

@zooba zooba added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label May 17, 2025
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @zooba for commit 7a8d6ab 🤖

Results will be shown at:

https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F133966%2Fmerge

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label May 17, 2025
@zooba zooba merged commit 986c367 into python:main May 19, 2025
131 of 132 checks passed
@miss-islington-app
Copy link

Thanks @zooba for the PR 🌮🎉.. I'm working now to backport this PR to: 3.14.
🐍🍒⛏🤖

@miss-islington-app
Copy link

Sorry, @zooba, I could not cleanly backport this to 3.14 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 986c3670285670558b6e6f77ff1507dfcc2c99fa 3.14

@zooba zooba deleted the gh-133779 branch May 19, 2025 10:35
zooba added a commit to zooba/cpython that referenced this pull request May 19, 2025
…to a static header. (pythonGH-133966)

Extension builders must specify Py_GIL_DISABLED if they want to link to the free-threaded builds.
This was usually the case already, but this change guarantees it in all circumstances.
@bedevere-app
Copy link

bedevere-app bot commented May 19, 2025

GH-134211 is a backport of this pull request to the 3.14 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.14 bugs and security fixes label May 19, 2025
@brandtbucher
Copy link
Member

@zooba, looks like these changes may have broken JIT builds (https://github.com/python/cpython/actions/runs/15116899287/job/42490545690?pr=134240).

I think the new generated JIT stencils directory doesn't exist, so it can't be used as a working directory in the build scripts. Further, the Python script that runs expects pyconfig.h and the generated stencil files to live side-by-side (in the directory where the script is run). We could change that assumption if needed and pass the path explicitly... but unless it's a big issue, I think we should leave the stencils wherever we're putting pyconfig.h.

@brandtbucher
Copy link
Member

Oh, I see. It looks like pyconfig.h doesn't live in the build dir anymore... hm. Okay, we'll probably need to modify the JIT build script to have that path passed in instead.

@brandtbucher
Copy link
Member

brandtbucher commented May 19, 2025

Would it be too difficult to go back to copying pyconfig.h into the build directory, where the JIT stencils are going? We can handle it if not, it just requires a bit of surgery to untangle all of those assumptions in the JIT build scripts.

@zooba
Copy link
Member Author

zooba commented May 19, 2025

Would it be too difficult to go back to copying pyconfig.h into the build directory, where the JIT stencils are going?

I'm going to start by saying "yes", because we went at least a decade with a static config file, and the experimental free-threading build is the only reason we stopped. So we're back to "you'd better have a really strong reason to complicate the build system", and I don't think this is it.

zooba added a commit that referenced this pull request May 19, 2025
… to a static header. (GH-133966)

Extension builders must specify Py_GIL_DISABLED if they want to link to the free-threaded builds.
This was usually the case already, but this change guarantees it in all circumstances.
@encukou
Copy link
Member

encukou commented Jun 5, 2025

You'll probably be interested in the test_cext failure

I am, see #135183. Sorry for the delay :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants