pypa / setuptools

Official project repository for the Setuptools build system
https://pypi.org/project/setuptools/
MIT License
2.54k stars 1.2k forks source link

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

Open cvijdea-bd opened 2 months ago

cvijdea-bd commented 2 months ago

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.

https://github.com/python/cpython/issues/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
jakirkham commented 1 month ago

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 ( https://github.com/python/cpython/pull/115850 )

cvijdea-bd commented 1 month ago

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 commented 1 month ago

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

abravalheri commented 1 month ago

https://github.com/python/cpython/issues/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 commented 1 month ago

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 commented 1 month ago

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.

cvijdea-bd commented 1 month ago

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 commented 1 month ago

I put up a PR in distutils: https://github.com/pypa/distutils/pull/310