Skip to content

Set Py_GIL_DISABLED=1 for free threaded Python on Windows - #310

Merged
FFY00 merged 2 commits into
pypa:mainfrom
colesbury:freethreading-windows
Nov 26, 2024
Merged

Set Py_GIL_DISABLED=1 for free threaded Python on Windows#310
FFY00 merged 2 commits into
pypa:mainfrom
colesbury:freethreading-windows

Conversation

@colesbury

Copy link
Copy Markdown
Contributor

When free threaded CPython is installed from the official Windows installer it doesn't have the macro Py_GIL_DISABLED properly set becuase its pyconfig.h file is shared across the co-installed default build.

Define the macro when building free threaded Python extensions on Windows so that each individual C API extension doesn't have to work around this limitation.

See pypa/setuptools#4662

@Zheaoli Zheaoli left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM

Comment thread distutils/command/build_ext.py Outdated
@Zheaoli

Zheaoli commented Nov 1, 2024

Copy link
Copy Markdown

BTW I think we should release a new version that includes this PR ASAP. I think this issue have already been a block reason for the people who want to build free threading extension on windows

@rgommers rgommers left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

In case it helps: this change looks correct to me, and is the right (and probably the only reasonable) choice to make in my opinion. We made the same change in Meson (xref mesonbuild/meson#13338) and have tested it pretty heavily by now - it's working as advertised, while without the Py_GIL_DISABLED define, users are getting hard to understand errors at build time.

So +1 for merging this.

@vstinner vstinner left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. It's unfortunate that we have to do that, but the fix looks good to me.

@FFY00 FFY00 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think the workaround is okay, but we should look into fixing the source of the issue.

@FFY00

FFY00 commented Nov 26, 2024

Copy link
Copy Markdown
Member

The test failures seem unrelated. Unfortunately, they are also failing on main, but they don't look the same. I'll rebase this PR on main and manually compare the PR tests with the ones from main.

When free threaded CPython is installed from the official Windows
installer it doesn't have the macro `Py_GIL_DISABLED` properly set
becuase its `pyconfig.h` file is shared across the co-installed default
build.

Define the macro when building free threaded Python extensions on
Windows so that each individual C API extension doesn't have to work
around this limitation.

See pypa/setuptools#4662
@FFY00
FFY00 force-pushed the freethreading-windows branch from 407bd87 to de1e624 Compare November 26, 2024 13:09
@FFY00

FFY00 commented Nov 26, 2024

Copy link
Copy Markdown
Member

The tests seem similar enough, I'll merge this and then see if I have time to fix the CI 🙃

@FFY00
FFY00 merged commit 49e362c into pypa:main Nov 26, 2024
@rgommers

rgommers commented Nov 26, 2024

Copy link
Copy Markdown

I think the workaround is okay, but we should look into fixing the source of the issue.

I believe @zooba has already tried, and declared the approach in this PR as the solution to be able to build against the official CPython Windows installers until free-threaded becomes the default build - from https://discuss.python.org/t/windows-installer-freethreading-and-building-extension-modules/54391:

"The multiple pyconfig.h problem is why we skipped an alpha, and this is the workaround, which will probably last until the day when the variable becomes redundant."

"It looks like you’re using setuptools. They can start detecting the variable and passing it themselves whenever they like."

@FFY00

FFY00 commented Nov 26, 2024

Copy link
Copy Markdown
Member

This specific issue is a symptom of Python not being designed to have paths shared between multiple builds. I think it makes sense to at least discuss in more depth how this could be avoided.

@zooba

zooba commented Nov 26, 2024

Copy link
Copy Markdown
Contributor

I think it makes sense to at least discuss in more depth how this could be avoided.

Supporting the same set of headers for all platforms (or at minimum, all Windows architectures) and relying on preprocessor switches to choose the build options is a pretty standard way of doing this on Windows.

I certainly wouldn't want to switch wholesale to a configure/make model for all Windows users just for the sake of a temporary flag. If there's a second feature that deserves this, we can consider it.

@lysnikolaou

Copy link
Copy Markdown

Hey @FFY00! Are you going to merge this in pypa/setuptools as well? Having a released version of setuptools with this included would help a lot in unblocking CI for a lot of projects.

@FFY00

FFY00 commented Dec 3, 2024

Copy link
Copy Markdown
Member

Hey, sorry for the late reply, I am not a setuptools maintainer.

cc @jaraco

@jaraco

jaraco commented Dec 26, 2024

Copy link
Copy Markdown
Member

My new job has been keeping me busy, so I'm just getting caught up on this. Let's see...

For future reference, I would have done the merge differently:

  • Merges in distutils need to have project-absolute references (e.g. pypa/distutils#310 instead of #310) because these commits are merged into Setuptools and when they're merged, they'll become Setuptools references.
  • Also, to help maintain fidelity, this project prefers to use merge commits instead of squash commits, as it provides the ability to summarize changes by looking at merge commits or dive into the details.

I'll get those things repaired and then get the latest changes merged into Setuptools.

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.

8 participants