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

Py_GIL_DISABLED not defined in "free-threaded" Windows C-API extensions #111650

Closed
Tracked by #108219
colesbury opened this issue Nov 2, 2023 · 10 comments
Closed
Tracked by #108219
Assignees
Labels
3.13 new features, bugs and security fixes topic-free-threading type-bug An unexpected behavior, bug, or error

Comments

@colesbury
Copy link
Contributor

colesbury commented Nov 2, 2023

Bug report

On Windows, the Py_GIL_DISABLED macro is not defined when building C API extensions with the free-threaded build of CPython 3.13 (i.e., --disable-gil). Now that biased reference counting (6dfb8fe) landed, this leads to incorrect/failed compilation of extensions on Windows 1. The problem does not occur on Linux/macOS and other ./configure based builds because that build system defines the Py_NOGIL macro as part of pyconfig.h. On Windows, Py_GIL_DISABLED instead defined as a pre-processor definition in pyproject.props, but that's used by C-API extensions.

Among other problems, this leads to test_peg_generator failing on the nogil Windows buildbot (example failure).

I can think of two possible approaches to fixing this:

  1. Modify setuptools (maybe setuptools.distutils?) to define Py_GIL_DISABLED when invoking the compiler if sysconfig.get_config_var("Py_NOGIL") is True.
  2. In the CPython Windows build, generate a header file defining/undefing Py_GIL_DISABLED depending on if the build is invoked with --disable-gil or not.

The first option seems a little more straightforward to me, but there is a risk that we might encounter this same issue across multiple extension build systems. The second option seems like it would more robustly address this issue.

Linked PRs

Footnotes

  1. Modules built as part of the CPython build (i.e., standard library modules) still work

@colesbury colesbury added type-bug An unexpected behavior, bug, or error 3.13 new features, bugs and security fixes topic-free-threading labels Nov 2, 2023
@colesbury
Copy link
Contributor Author

@zooba, I'd appreciate your feedback and suggestions about the best way to make sure that Windows C API extensions have Py_NOGIL defined when building with the free-threaded (--disable-gil) 3.13 builds.

@corona10
Copy link
Member

corona10 commented Nov 2, 2023

Among other problems, this leads to test_peg_generator failing on the nogil Windows buildbot (example failure).

I was curious about how GHA passed the test, but I noticed that it was skipped from the GHA..
https://github.com/python/cpython/actions/runs/6695685454/job/18191835485

0:16:09 load avg: 10.69 [405/469/1] test_peg_generator skipped (resource denied) -- running (1): test_importlib (42.1 sec)
test_peg_generator skipped -- Use of the 'cpu' resource not enabled

@zooba
Copy link
Member

zooba commented Nov 2, 2023

I think we probably need to start modifying pyconfig.h at some stage in the build process. Eventually we should get an improved sysconfig [alternative] that can include the flag, but until then I don't think we want to make modifications in every build backend that's out there, nor do we want to force them to launch the target interpreter in order to get the right settings (which prevents cross-compilation, for example).

I'm not sure how best to do it though. Probably the ideal looks like:

  • copy/patch pyconfig.h and write it to $(IntDir) (not back into the source tree)
  • do rest of build referencing that directory instead of PC\
  • copy patched pyconfig.h into $(OutDir)
  • make sure Tools/msi and PC/layout pull it from the output directory and not the sources
  • make sure the build in release-tools does the same (though I think it should happen automatically with the previous changes)

We shouldn't have to patch anything in the stdlib that finds the header, I'm pretty sure they were only in distutils. And an install layout should have it end up in the Include directory if Tools/msi and PC/layout are updated.

So there's a lot of moving parts, plus we need to figure out the best way to template the header so it can be modified as part of the build. There's not really anything built into our build system for this yet.

@colesbury
Copy link
Contributor Author

Hi @zooba, I started implementing this and ran into a few issues. Not having a pyconfig.h in PC or Include breaks setuptools. The install layout can place the generated pyconfig.h in the installed Include directory, but you can also run pip directly from the CPython source directory without the installer. It seems like we'll run into fewer issues if we place the generated pyconfig.h in the PC directory.

colesbury added a commit to colesbury/cpython that referenced this issue Nov 16, 2023
colesbury added a commit to colesbury/cpython that referenced this issue Nov 16, 2023
colesbury added a commit to colesbury/cpython that referenced this issue Nov 16, 2023
Prior to this change, the Py_NOGIL macro was not defined when building
C API extensions with the `--disable-gil` build on Windows. `Py_NOGIL`
was only defined as a pre-processor definition in pyproject.props, but
that is not used by C-API extensions.

This instead generates the `pyconfig.h` header on Windows as part of
the build process. For now, `Py_NOGIL` is the only macro that may
be conditionally defined in the generated file.
colesbury added a commit to colesbury/cpython that referenced this issue Nov 16, 2023
Prior to this change, the Py_NOGIL macro was not defined when building
C API extensions with the `--disable-gil` build on Windows. `Py_NOGIL`
was only defined as a pre-processor definition in pyproject.props, but
that is not used by C-API extensions.

This instead generates the `pyconfig.h` header on Windows as part of
the build process. For now, `Py_NOGIL` is the only macro that may
be conditionally defined in the generated file.
colesbury added a commit to colesbury/cpython that referenced this issue Nov 16, 2023
Prior to this change, the Py_NOGIL macro was not defined when building
C API extensions with the `--disable-gil` build on Windows. `Py_NOGIL`
was only defined as a pre-processor definition in pyproject.props, but
that is not used by C-API extensions.

This instead generates the `pyconfig.h` header on Windows as part of
the build process. For now, `Py_NOGIL` is the only macro that may
be conditionally defined in the generated file.
colesbury added a commit to colesbury/cpython that referenced this issue Nov 20, 2023
Prior to this change, the Py_NOGIL macro was not defined when building
C API extensions with the `--disable-gil` build on Windows. `Py_NOGIL`
was only defined as a pre-processor definition in pyproject.props, but
that is not used by C-API extensions.

This instead generates the `pyconfig.h` header on Windows as part of
the build process. For now, `Py_NOGIL` is the only macro that may
be conditionally defined in the generated file.
@zooba
Copy link
Member

zooba commented Nov 29, 2023

Left a few suggestions on the draft PR - in short:

  • I prefer Include\pyconfig.h over PC\pyconfig.h, and I suspect/hope every other tool will, too.
  • setuptools is definitely going to need an update of some kind here, at least for running within build directories (likely in code taken from distutils). Regular installs shouldn't have any issue. I think we should do the setuptools update in parallel, and not try to make their older versions work.
  • As neat as the PowerShell script is, we already assume a bootstrap Python is available, so it may as well be a Python script

@colesbury
Copy link
Contributor Author

POSIX places the generated pyconfig.h in the root directory, not in Include. Placing it in Include/pyconfig.h is would be a third location for tools to handle, breaks setuptools (and possibly other build tools), and does not get us any closer to unifying the builds.

setuptools is definitely going to need an update of some kind here...

If we don't move pyconfig.h, what update will setuptools need?

As neat as the PowerShell script is, we already assume a bootstrap Python is available, so it may as well be a Python script

Sure - I'll convert the script to Python.

@zooba
Copy link
Member

zooba commented Dec 4, 2023

If we don't move pyconfig.h, what update will setuptools need?

If we don't move it, then a whole lot of other stuff needs updating instead. But we should 100% move it, because we don't put build-generated files into the source tree - they always live in the build tree, which does not overlap. (POSIX doesn't work like this, but people who have thought about it say that it should've, and that writing build files into the source tree is an unfortunate design choice. Our Windows build gets this right, and I don't want to regress it.)

@colesbury
Copy link
Contributor Author

I don't know how to implement what you are asking for and I've already spent a few days on this, so I'm going to defer this and come back to it later.

@zooba zooba self-assigned this Dec 5, 2023
@zooba
Copy link
Member

zooba commented Dec 5, 2023

I'll get it figured out. Chances are I'm going to have to do all the integration into the release process anyway, so I may as well go end-to-end.

@colesbury colesbury changed the title Py_NOGIL not defined in "free-threaded" Windows C-API extensions Py_GIL_DISABLED not defined in "free-threaded" Windows C-API extensions Dec 6, 2023
zooba added a commit to zooba/cpython that referenced this issue Dec 6, 2023
zooba added a commit to zooba/cpython that referenced this issue Dec 7, 2023
@ambv
Copy link
Contributor

ambv commented Jan 7, 2024

Is there anything left to do here?

@ambv ambv closed this as completed Jan 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.13 new features, bugs and security fixes topic-free-threading type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

No branches or pull requests

4 participants