search

python / cpython

The Python programming language
https://www.python.org
Other
63.14k stars 30.23k forks source link

Python 3.11 loses the ability to set PYTHON_DECIMAL_WITH_MACHINE #98557

Open Bo98 opened 2 years ago

Bo98 commented 2 years ago

Bug report

Python 3.10 had the ability to set PYTHON_DECIMAL_WITH_MACHINE to override the choice of configuration for the _decimal module: https://github.com/python/cpython/blob/dcb342b5f9d931b030ca310bf3e175bbc54df5aa/setup.py#L2388-L2397

Since Python 3.11, this is no longer possible. This feature was necessary, on macOS particularly, with the --with-system-libmpdec option if that system libmpdec is configured differently to the default Python config. On macOS, the default Python config forces universal, while setting PYTHON_DECIMAL_WITH_MACHINE allowed it to be single-arch.

libmpdec produces different headers depending on how it was built, which is why the setting is important to be able to override. Without it, the _decimal module will fail to compile if the default does not match how system libmpdec was built.

Homebrew's Python currently depends on this feature.

A test within CPython also seems to depend on this feature: https://github.com/python/cpython/blob/f4c03484da59049eb62a9bf7777b963e2267d187/Modules/_decimal/tests/runall-memorydebugger.sh#L63-L79

Your environment

AlexWaygood commented 1 year ago

Cc. @rhettinger for decimal module expertise.

erlend-aasland commented 1 year ago

FTR, this was removed in GH-29541 (cc. @tiran as PR author and @mdickinson as PR reviewer)

erlend-aasland commented 1 year ago

AFAICS from GH-29541, it should be possible to set PYTHON_DECIMAL_WITH_MACHINE via CFLAGS (for example LIBMPDEC_CFLAGS). @Bo98, can you please test this?

Bo98 commented 1 year ago

What reads PYTHON_DECIMAL_WITH_MACHINE? It's not a define to be passed to C but something that used to change what item is selected from: https://github.com/python/cpython/blob/75a6fadf369315b27e12f670e6295cf2c2cf7d7e/configure.ac#L3882-L3891

Unless you suggest that I instead pass LIBMPDEC_CFLAGS=-DCONFIG_64=1 -DASM=1 on x86_64 and LIBMPDEC_CFLAGS=-DCONFIG_64=1 -DANSI=1 -DHAVE_UINT128_T=1 on arm64? (Assuming this list is never changing.)

erlend-aasland commented 1 year ago

Ah, sorry, you are right. I misread the diff from GH-29541; it is used as an index, not a CFLAG.

erlend-aasland commented 1 year ago

Yes, it seems this ability is now lost; there is no way to manually override that AS_CASE. We'd either have to reintroduce PYTHON_DECIMAL_WITH_MACHINE, or provide a new configure switch for this, if we were to reintroduce this.

erlend-aasland commented 1 year ago

You mentioned Homebrew? Does this prevent Homebrew from building and distributing Python 3.11? Is there a Homebrew ticket for this?

erlend-aasland commented 1 year ago

From https://github.com/python/cpython/issues/98557#issuecomment-1288927599:

Unless you suggest that I instead pass LIBMPDEC_CFLAGS=-DCONFIG_64=1 -DASM=1 on x86_64 and LIBMPDEC_CFLAGS=-DCONFIG_64=1 -DANSI=1 -DHAVE_UINT128_T=1 on arm64? (Assuming this list is never changing.)

Then from the OP:

libmpdec produces different headers depending on how it was built, which is why the setting is important to be able to override. Without it, the _decimal module will fail to compile if the default does not match how system libmpdec was built.

I suggest you use LIBMPDEC_CFLAGS to set these flags depending on how your libmpdec was built. That seems to me to be a more robust approach than letting configure chose flags which might not reflect how your libmpdec was built.

ISTM we don't need a way to override the libmpdec_machine switch in configure.

Bo98 commented 1 year ago

Is there a Homebrew ticket for this?

Not yet considering we don't ship betas, but what we'll probably do for 3.11.0 (and what I've done in a local test rc2 build) is a s/libmpdec_machine=universal/libmpdec_machine=x64/ (uint128 on arm64) on configure. Not a desirable long-term solution but should be ok for a short-term fix.

I suggest you use LIBMPDEC_CFLAGS to set these flags depending on how your libmpdec was built. That seems to me to be a more robust approach than letting configure chose flags which might not reflect how your libmpdec was built.

PYTHON_DECIMAL_WITH_MACHINE did directly match the option you would pass to libmpdec configure. If you passed MACHINE=x64 to libmpdec while building then you would pass the same to Python.

It made sense and is what Stefan Krah recommended we did.

Might I propose an alternative way forward here however. I propose looking into not setting any CFLAGS for the system libmpdec and checking MPD_CONFIG_64 and MPD_CONFIG_32 in _decimal source code, given mpdecimal.h should tell you how it was built by setting those.

FWIW, this is what the relevent section of mpdecimal.h looks like on a different configurations of system libmpdec:

Universal ``` /* Mac OS X: support for building universal binaries */ #if defined(MPD_CONFIG_64) || defined(MPD_CONFIG_32) #error "cannot use MPD_CONFIG_64 or MPD_CONFIG_32 with universal header." #endif #if defined(CONFIG_64) || defined(CONFIG_32) #error "cannot use CONFIG_64 or CONFIG_32 with universal header." #endif #if defined(__ppc__) #define MPD_CONFIG_32 1 #ifdef UNIVERSAL #define CONFIG_32 #define ANSI #endif #elif defined(__ppc64__) #define MPD_CONFIG_64 1 #ifdef UNIVERSAL #define CONFIG_64 #define ANSI #endif #elif defined(__i386__) #define MPD_CONFIG_32 1 #ifdef UNIVERSAL #define CONFIG_32 #define ANSI #endif #elif defined(__x86_64__) #define MPD_CONFIG_64 1 #ifdef UNIVERSAL #define CONFIG_64 #define ASM #endif #elif defined(__arm64__) #define MPD_CONFIG_64 1 #ifdef UNIVERSAL #define CONFIG_64 #define ANSI #endif #else #error "unknown architecture for universal build." #endif ```
x64 and uint128 ``` /* ABI: 64-bit */ #define MPD_CONFIG_64 1 #ifdef MPD_CONFIG_32 #error "cannot use MPD_CONFIG_32 with 64-bit header." #endif #ifdef CONFIG_32 #error "cannot use CONFIG_32 with 64-bit header." #endif ```
erlend-aasland commented 1 year ago

Might I propose an alternative way forward here however. I propose looking into not setting any CFLAGS for the system libmpdec and checking MPD_CONFIG_64 and MPD_CONFIG_32 in _decimal source code, given mpdecimal.h should tell you how it was built by setting those.

That sounds like a better way forward indeed. I've already pinged Christian on this issue; let's wait and see if he also agrees.

carlocab commented 2 weeks ago

It's been a while here. Can we find a way to move this forward?

erlend-aasland commented 1 week ago

It's been a while here. Can we find a way to move this forward?

For 3.11? Unfortunately not; 3.11 only received security fixes. For 3.12 and newer, the environment variables LIBMPDEC_CFLAGS and LIBMPDEC_LIBS are available if you want to customise how _decimal is built.

See also #115119.

Bo98 commented 1 week ago

Looks like #115406 might have fixed this by moving the assumptions into a AS_VAR_IF([with_system_libmpdec], [no] in 3.13. Specifically, the flag and decimal.c changes rather than the actual pkg-config part, so the fix works for pre-4.0 too.

For 3.11 and 3.12 it was previously unfixable without patching the configure file (LIBMPDEC_CFLAGS won't override it), unless a backport of that new patch is applied.

erlend-aasland commented 1 week ago

Right, so my gut feel would be that a backport of #115406 probably won't happen; such changes to the configure script are always slight behavioural changes to the build system, so backporting them may create havoc for distros.