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

Python 3.13a4 ships with wrong pyconfig.h? #115582

Closed
Starbuck5 opened this issue Feb 17, 2024 · 11 comments
Closed

Python 3.13a4 ships with wrong pyconfig.h? #115582

Starbuck5 opened this issue Feb 17, 2024 · 11 comments
Assignees
Labels
3.13 bugs and security fixes OS-windows release-blocker topic-free-threading type-bug An unexpected behavior, bug, or error

Comments

@Starbuck5
Copy link

Starbuck5 commented Feb 17, 2024

Bug report

Bug description:

Hello,

I tried to build a C-extension locally with python 3.13a4 and got the error LINK : fatal error LNK1104: cannot open file 'python313t.lib'.

(I got Python 3.13a4 from the python.org installer, tested Windows 32 and 64 bit).

After trying to install the free threaded builds using the new installer option, trying to build using those, and then installing everything fresh, I found that in include/pyconfig.h, there is a line #define Py_GIL_DISABLED 1. This line causes the compiler to look for python313t.lib instead of python313.lib, and commenting it out fixed the build.

It seems to me that the non-freethreaded build is shipping out with the wrong pyconfig.h setup.

This was reported by Pillow here: python-pillow/Pillow#7805

They linked it to #115545, but I think that may be a different issue? The interpreters launch fine from the python.org install, they just don't seem to work for building C-extensions with.

Thank you.

CPython versions tested on:

3.13

Operating systems tested on:

Windows

Linked PRs

@Starbuck5 Starbuck5 added the type-bug An unexpected behavior, bug, or error label Feb 17, 2024
@hugovk hugovk added OS-windows 3.8 only security fixes 3.13 bugs and security fixes release-blocker and removed 3.8 only security fixes labels Feb 17, 2024
@hugovk
Copy link
Member

hugovk commented Feb 17, 2024

I'm guessing these might both have the same root cause, but cc @zooba

@zooba
Copy link
Member

zooba commented Feb 19, 2024

Yeah, entirely possible that things have gotten mixed up. I already have #115545 which is going to be the same root cause.

@zooba zooba self-assigned this Feb 19, 2024
@zooba
Copy link
Member

zooba commented Feb 19, 2024

So fundamentally the problem is that I tried to share the Include directory between both free-threaded and normal builds, but that obviously doesn't work.

I'm most tempted to just drop the free-threaded binaries from the installer and only release them as a zip (essentially, the Nuget package, but a bit more discoverable).

The alternatives are to:

  1. separate the Include directory so it's not shared, which requires some contortions in sysconfig and for anyone doing cross-compilation
  2. separate the entire prefix, which messes with anyone trying to launch/reference python3.13t.exe
  3. make compilers have to set Py_DISABLE_GIL themselves

The only one of those I kind of like is 3, but I suspect we'll run into more and more issues with trying to keep them together, and just splitting it out into a totally separate package is going to be safest.

I'm thinking out loud right now - going to sleep on it before making a decision, so if anyone has thoughts, please speak up!

@encukou
Copy link
Member

encukou commented Feb 20, 2024

For reference/prior art: Fedora uses the same include directory for 64-bit and 32-bit builds.
Python has all the configuration in a single header file -- others use #ifdef with values defined in pyconfig.h. So, there is pyconfig-64.h and pyconfig-32.h, and the shared one includes one of them:

$ cat /usr/include/python3.13/pyconfig.h 
#include <bits/wordsize.h>

#if __WORDSIZE == 32
#include "pyconfig-32.h"
#elif __WORDSIZE == 64
#include "pyconfig-64.h"
#else
#error "Unknown word size"
#endif

Ubuntu takes the idea much further -- for all architectures.

pyconfig.h from an Ubuntu container
# cat /usr/include/python3.10/pyconfig.h 
#if defined(__linux__)
# if defined(__x86_64__) && defined(__LP64__)
#  include <x86_64-linux-gnu/python3.10/pyconfig.h>
# elif defined(__x86_64__) && defined(__ILP32__)
#  include <x86_64-linux-gnux32/python3.10/pyconfig.h>
# elif defined(__i386__)
#  include <i386-linux-gnu/python3.10/pyconfig.h>
# elif defined(__aarch64__) && defined(__AARCH64EL__)
#  include <aarch64-linux-gnu/python3.10/pyconfig.h>
# elif defined(__arc__)
#  include <arc-linux-gnu/python3.10/pyconfig.h>
# elif defined(__alpha__)
#  include <alpha-linux-gnu/python3.10/pyconfig.h>
# elif defined(__ARM_EABI__) && defined(__ARM_PCS_VFP)
#  include <arm-linux-gnueabihf/python3.10/pyconfig.h>
# elif defined(__ARM_EABI__) && !defined(__ARM_PCS_VFP)
#  include <arm-linux-gnueabi/python3.10/pyconfig.h>
# elif defined(__hppa__)
#  include <hppa-linux-gnu/python3.10/pyconfig.h>
# elif defined(__ia64__)
#  include <ia64-linux-gnu/python3.10/pyconfig.h>
# elif defined(__m68k__) && !defined(__mcoldfire__)
#  include <m68k-linux-gnu/python3.10/pyconfig.h>
# elif defined(__mips_hard_float) && defined(__mips_isa_rev) && (__mips_isa_rev >=6) && defined(_MIPSEL)
#  if _MIPS_SIM == _ABIO32
#   include <mipsisa32r6el-linux-gnu/python3.10/pyconfig.h>
#  elif _MIPS_SIM == _ABIN32
#   include <mipsisa64r6el-linux-gnuabin32/python3.10/pyconfig.h>
#  elif _MIPS_SIM == _ABI64
#   include <mipsisa64r6el-linux-gnuabi64/python3.10/pyconfig.h>
#  else
#   error unknown multiarch location for pyconfig.h
#  endif
# elif defined(__mips_hard_float) && defined(__mips_isa_rev) && (__mips_isa_rev >=6)
#  if _MIPS_SIM == _ABIO32
#   include <mipsisa32r6-linux-gnu/python3.10/pyconfig.h>
#  elif _MIPS_SIM == _ABIN32
#   include <mipsisa64r6-linux-gnuabin32/python3.10/pyconfig.h>
#  elif _MIPS_SIM == _ABI64
#   include <mipsisa64r6-linux-gnuabi64/python3.10/pyconfig.h>
#  else
#   error unknown multiarch location for pyconfig.h
#  endif
# elif defined(__mips_hard_float) && defined(_MIPSEL)
#  if _MIPS_SIM == _ABIO32
#   include <mipsel-linux-gnu/python3.10/pyconfig.h>
#  elif _MIPS_SIM == _ABIN32
#   include <mips64el-linux-gnuabin32/python3.10/pyconfig.h>
#  elif _MIPS_SIM == _ABI64
#   include <mips64el-linux-gnuabi64/python3.10/pyconfig.h>
#  else
#   error unknown multiarch location for pyconfig.h
#  endif
# elif defined(__mips_hard_float)
#  if _MIPS_SIM == _ABIO32
#   include <mips-linux-gnu/python3.10/pyconfig.h>
#  elif _MIPS_SIM == _ABIN32
#   include <mips64-linux-gnuabin32/python3.10/pyconfig.h>
#  elif _MIPS_SIM == _ABI64
#   include <mips64-linux-gnuabi64/python3.10/pyconfig.h>
#  else
#   error unknown multiarch location for pyconfig.h
#  endif
# elif defined(__or1k__)
#  include <or1k-linux-gnu/python3.10/pyconfig.h>
# elif defined(__powerpc__) && defined(__SPE__)
#  include <powerpc-linux-gnuspe/python3.10/pyconfig.h>
# elif defined(__powerpc64__)
#  if defined(__LITTLE_ENDIAN__)
#    include <powerpc64le-linux-gnu/python3.10/pyconfig.h>
#  else
#    include <powerpc64-linux-gnu/python3.10/pyconfig.h>
#  endif
# elif defined(__powerpc__)
#  include <powerpc-linux-gnu/python3.10/pyconfig.h>
# elif defined(__s390x__)
#  include <s390x-linux-gnu/python3.10/pyconfig.h>
# elif defined(__s390__)
#  include <s390-linux-gnu/python3.10/pyconfig.h>
# elif defined(__sh__) && defined(__LITTLE_ENDIAN__)
#  include <sh4-linux-gnu/python3.10/pyconfig.h>
# elif defined(__sparc__) && defined(__arch64__)
#  include <sparc64-linux-gnu/python3.10/pyconfig.h>
# elif defined(__sparc__)
#  include <sparc-linux-gnu/python3.10/pyconfig.h>
# elif defined(__riscv)
#  if __riscv_xlen == 64
#    include <riscv64-linux-gnu/python3.10/pyconfig.h>
#  else
#    include <riscv32-linux-gnu/python3.10/pyconfig.h>
#  endif
# else
#   error unknown multiarch location for pyconfig.h
# endif
#elif defined(__FreeBSD_kernel__)
# if defined(__LP64__)
#  include <x86_64-kfreebsd-gnu/python3.10/pyconfig.h>
# elif defined(__i386__)
#  include <i386-kfreebsd-gnu/python3.10/pyconfig.h>
# else
#   error unknown multiarch location for pyconfig.h
# endif
#elif defined(__gnu_hurd__)
# include <i386-gnu/python3.10/pyconfig.h>
#else
# error unknown multiarch location for pyconfig.h
#endif

Of course, free-threaded builds depend on Py_DISABLE_GIL being defined rather than on the target architecture, so the situation is different. But I don't think it's a bad idea to make all the config settings build-time (not configure-time) and base them on #ifdef Py_DISABLE_GIL. Let's make option 3 safe for the whole multi-year experiment.
If the the nogil crowd agrees with the constraint, of course.

@zooba
Copy link
Member

zooba commented Feb 20, 2024

Yeah, Windows also uses the same Include directory (including pyconfig.h) for all platforms. This is the first CPython build option other than debug (_d.exe) that needs to be matched by an extension build.

I believe everything in the headers is already based on #ifdef Py_GIL_DISABLED (my typo above), so option 3 is safe. The issue is that the build backends need to know when to define it, and if we start telling them to do it then we may run into challenges later (e.g. if it becomes the only option, do we fail builds without it?).

And defining it in your extension doesn't actually disable the GIL, it just says that your extension requires a build of Python that has it defined, which very much implies that you should be able to get it from somewhere. We have no precedent of a CFLAGS setting on Windows, so perhaps that needs to be added? The static config file will help, though that may not support distinguishing between different runtimes in the same directory either, which means we'll need to just have totally separate installs.

The simplest immediate fix would be to make sure that defining Py_GIL_DISABLED externally works, and then ship headers that don't define it (currently it would be undef'd on Windows, and I'm not sure about a generated pyconfig.h, but probably both need changes to be consistent). Probably for experimentation, that's fine, and maybe backends will choose to detect that they're running in/targeting free-threaded and define it themselves, even if we later label it experimental and say to stop doing it?

@encukou
Copy link
Member

encukou commented Feb 21, 2024

The simplest immediate fix does sound fine for experimentation. AFAIK, in 3.13, building extensions for free-threaded builds won't be a smooth experience in general; it's OK to just make it possible.

I can see Py_GIL_DISABLED becoming no-op in the future, but I don't think it needs to break builds when free-threaded becomes the only option. Same as with PY_SSIZE_T_CLEAN: you can still define it, but it doesn't do anything.

@zooba
Copy link
Member

zooba commented Feb 21, 2024

So then the question we have is whether our header files should be agnostic towards Py_GIL_DISABLED and always require the builder to specify it? Right now the config header has it in there to explicitly match how Python was built, but this doesn't allow for sharing the headers between two different builds.

Alternatively, if Py_GIL_DISABLED is specified (either 0 or 1) before including us, should it always override what is in the config header? So pyconfig.h looks like this:

#ifdef Py_GIL_DISABLED
#if !(Py_GIL_DISABLED+0)
#undef Py_GIL_DISABLED
#endif
#else
// Either:
#define Py_GIL_DISABLED 1
// or:
#undef Py_GIL_DISABLED
// selected at build time and frozen into the file
#endif

Or do we just allow "upgrading" (we won't undef it, so if you def it then you get it).

@Starbuck5
Copy link
Author

Maybe this wouldn't be the simplest in the short term, but it seems like totally separate install folders (mentioned by zooba) would be a good option.

example-separate-installs
(Artists rendition of what this would look like in directories)

PS C:\Users\charl> py -0
 -V:3.13t *       Python 3.13 (64-bit, freethreaded)
 -V:3.13          Python 3.13 (64-bit)
 -V:3.13-32       Python 3.13 (32-bit)
 -V:3.12          Python 3.12 (64-bit)
 -V:3.11          Python 3.11 (64-bit)

It would be nicely symmetrical with the current permutations possible to be picked up by the py launcher (32/64 bit), which currently have separate install directories.

It also allows for separate pips and separate site-packages directories so people can easily coexist Python 3.13 installations like 32/64 bit installations can coexist today.

And this saves any complexity needed to update the build backends to set Py_GIL_DISABLED, theoretically.

@encukou
Copy link
Member

encukou commented Feb 23, 2024

I lean toward only allowing “upgrading”. When the experiment is done, “downgrading” will stop working; we probably don't want -DPy_GIL_DISABLED=0 to be the mechanism for enabling the GIL.

@zooba
Copy link
Member

zooba commented Feb 23, 2024

Maybe this wouldn't be the simplest in the short term, but it seems like totally separate install folders (mentioned by zooba) would be a good option.

There are two main concerns with this:

  1. More parallel installs can be unreliable (breaking install/uninstall/etc.). There's a real chance this will need more tech support for users than I'm willing/able to provide for this experiment.
  2. More downloads listed on the download page. Users already get confused by the amount of choice present there, and giving them even more choices is going the wrong direction (I've run this concern past the SC and they agreed).

I lean toward only allowing “upgrading”

I'm not sure what's needed for the make build, but I can easily remove the undef from the default Windows headers.

@zooba
Copy link
Member

zooba commented Feb 29, 2024

The fix to our release build will ensure that the non-freethreaded header is shipped next time, and #115850 will allow people to override the variable if they want to build for freethreaded. That's the best we can do right now, and it at least unblocks the use of 3.13.

Once build backends start figuring out how they want to be doing freethreaded builds we can look at ways to make it smoother to select the right setting. Or if having the builds install together becomes more of a pain than the length of the downloads list, we might switch it to its own parallel install and then the default header will be fine.

@zooba zooba closed this as completed Feb 29, 2024
woodruffw pushed a commit to woodruffw-forks/cpython that referenced this issue Mar 4, 2024
adorilson pushed a commit to adorilson/cpython that referenced this issue Mar 25, 2024
diegorusso pushed a commit to diegorusso/cpython that referenced this issue Apr 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.13 bugs and security fixes OS-windows release-blocker topic-free-threading type-bug An unexpected behavior, bug, or error
Projects
Development

No branches or pull requests

5 participants