Skip to content
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

[BUG] Py_GIL_DISABLED not defined in "free-threaded" Windows C-API extensions #4662

Open
cvijdea-bd opened this issue Sep 27, 2024 · 8 comments

Comments

@cvijdea-bd
Copy link

setuptools version

75.1.0

Python version

3.13

OS

Windows

Additional environment information

No response

Description

When building a native extension against the free-threading build, the Py_GIL_DISABLED macro is not defined, which on Windows causes the built extension to crash on import due to linking to python313.dll instead of python313t.dll.

python/cpython#111650 seems to imply that it should be setuptools' job to handle this.

Expected behavior

Py_GIL_DISABLED macro is defined when building if sysconfig.get_config_var("Py_GIL_DISABLED").

How to Reproduce

py -3.13t -m pip install git+https://github.com/giampaolo/psutil.git@b1e52fcfe4bd0a3d09334fddd6ffd23288f77a79
py -3.13t -c "import psutil"


### Output

segfault in python313.dll
@cvijdea-bd cvijdea-bd added bug Needs Triage Issues that need to be evaluated for severity and status. labels Sep 27, 2024
@jakirkham
Copy link
Contributor

Do you know how Python was built in this case? If so, was --disable-gil passed during the build? That should ensure that Py_GIL_DISABLED is defined ( python/cpython#115850 )

@cvijdea-bd
Copy link
Author

This is using py -3.13t from the python launcher via the python.org distribution: https://www.python.org/downloads/release/python-3130/

@cvijdea-bd
Copy link
Author

As I understand, on Windows there is a shared include directory betwen the normal and disable-gil builds, and a single pyconfig.h

@abravalheri
Copy link
Contributor

abravalheri commented Oct 15, 2024

python/cpython#111650 seems to imply that it should be setuptools' job to handle this.

Hi @cvijdea-bd, I disagree with this interpretation, the issue seems to suggest that it needs to be tackled in both CPython and in setuptools. In that regard, Setuptools is open to any PR proposed by members of the community (if I am not wrong none of the active setuptools maintainers participated on the changes in the GIL, so it is best if someone with expertise in the area proposes the change) - so please feel free to open a PR in setuptools if you think that is necessary for free-thread builds.

An important aspect of the fix seems to be for CPython to define a coherent value for sysconfig.get_config_var('Py_GIL_DISABLED'), as suggested in the docs:

Limited C API and Stable ABI
The free-threaded build does not currently support the Limited C API or the stable ABI. If you use setuptools to build your extension and currently set py_limited_api=True you can use py_limited_api=not sysconfig.get_config_var("Py_GIL_DISABLED") to opt out of the limited API when building with the free-threaded build.

Note You will need to build separate wheels specifically for the free-threaded build. If you currently use the stable ABI, you can continue to build a single wheel for multiple non-free-threaded Python versions.

Could you please confirm that the value of sysconfig.get_config_var('Py_GIL_DISABLED') in your installation makes sense? (I don't think that setuptools would be able to guess that without it being defined in sysconfig).

Also please note that the docs explicitly require Windows user to manually define Py_GIL_DISABLED:

Windows
Due to a limitation of the official Windows installer, you will need to manually define Py_GIL_DISABLED=1 when building extensions from source.

So I suppose this means Py_GIL_DISABLED needs to be manually defined as a macro by the developer, but please feel free to double check with the CPython developers that is what they mean.

@cvijdea-bd
Copy link
Author

Could you please confirm that the value of sysconfig.get_config_var('Py_GIL_DISABLED') in your installation makes sense? (I don't think that setuptools would be able to guess that without it being defined in sysconfig).

Yes, the sysconfig variable correctly reflects the state of Py_GIL_DISABLED

Also please note that the docs explicitly require Windows user to manually define Py_GIL_DISABLED:

That is true, but that also does not preclude setuptools from handling this automatically.

If setuptools does not handle it, then quite literally every library using setuptools to build native extensions have to add code to this effect to their setup.py:

    if os.name == 'nt' and sysconfig.get_config_var("Py_GIL_DISABLED"):
        macros.append(('Py_GIL_DISABLED', 1))

Without the macro defined, the resulting extension will crash the interpreter on import 100% of the time. I just don't see how that could be acceptable default behavior.

However I'm not familiar enough with setuptools to really be able to figure out precisely under what conditions the definition should be implicitly added, but the gist of it is that any extension with the free-threaded abi tag (t in "cp313t") MUST be built with -DPy_GIL_DISABLED=1

@abravalheri
Copy link
Contributor

Thank you for the clarification @cvijdea-bd, I will add the help wanted to this issue in the hope that someone that understands enough about free-thread builds submits a PR.

Meanwhile users that need the dependency should follow the Python docs advice and manually define the macro.

@abravalheri abravalheri added help wanted and removed Needs Triage Issues that need to be evaluated for severity and status. labels Oct 15, 2024
@cvijdea-bd
Copy link
Author

cvijdea-bd commented Oct 15, 2024

For prior art, CMake defines the macro automatically when the name of the dll import library to be linked ends in "t.lib" (e.g. as in python313t.lib, which is the import library for python313t.dll): https://github.com/Kitware/CMake/blob/9c25632ba0ad0525b20195d371b3d78a8bcc4113/Modules/FindPython/Support.cmake#L3728-L3731

colesbury added a commit to colesbury/distutils that referenced this issue Nov 1, 2024
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
colesbury added a commit to colesbury/distutils that referenced this issue Nov 1, 2024
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
@colesbury
Copy link

I put up a PR in distutils: pypa/distutils#310

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

No branches or pull requests

4 participants