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.11 loses the ability to set PYTHON_DECIMAL_WITH_MACHINE #98557

Open
Bo98 opened this issue Oct 22, 2022 · 14 comments
Open

Python 3.11 loses the ability to set PYTHON_DECIMAL_WITH_MACHINE #98557

Bo98 opened this issue Oct 22, 2022 · 14 comments
Labels
build The build process and cross-build type-bug An unexpected behavior, bug, or error

Comments

@Bo98
Copy link
Contributor

Bo98 commented Oct 22, 2022

Bug report

Python 3.10 had the ability to set PYTHON_DECIMAL_WITH_MACHINE to override the choice of configuration for the _decimal module:

cpython/setup.py

Lines 2388 to 2397 in dcb342b

machine = os.environ.get('PYTHON_DECIMAL_WITH_MACHINE')
if machine:
# Override automatic configuration to facilitate testing.
define_macros = config[machine]
elif MACOS:
# Universal here means: build with the same options Python
# was built with.
define_macros = config['universal']
elif sizeof_size_t == 8:

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:

unset PYTHON_DECIMAL_WITH_MACHINE
libmpdec_config=$config
if [ X"$config" != X"auto" ]; then
PYTHON_DECIMAL_WITH_MACHINE=$config
export PYTHON_DECIMAL_WITH_MACHINE
else
libmpdec_config=""
fi
############ refleak tests ###########
print_config "refleak tests: config=$config" $args
printf "\nbuilding python ...\n\n"
cd ../../
$GMAKE distclean > /dev/null 2>&1
./configure CFLAGS="$ADD_CFLAGS" LDFLAGS="$ADD_LDFLAGS" --with-pydebug $args > /dev/null 2>&1
$GMAKE | grep _decimal

Your environment

  • CPython versions tested on: 3.11.0rc2
  • Operating system and architecture: macOS 12 (x86_64 and arm64)
@Bo98 Bo98 added the type-bug An unexpected behavior, bug, or error label Oct 22, 2022
@AlexWaygood
Copy link
Member

Cc. @rhettinger for decimal module expertise.

@erlend-aasland
Copy link
Contributor

erlend-aasland commented Oct 24, 2022

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

@erlend-aasland
Copy link
Contributor

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?

@erlend-aasland erlend-aasland added the build The build process and cross-build label Oct 24, 2022
@Bo98
Copy link
Contributor Author

Bo98 commented Oct 24, 2022

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:

cpython/configure.ac

Lines 3882 to 3891 in 75a6fad

AS_CASE([$libmpdec_machine],
[x64], [AS_VAR_APPEND([LIBMPDEC_CFLAGS], [" -DCONFIG_64=1 -DASM=1"])],
[uint128], [AS_VAR_APPEND([LIBMPDEC_CFLAGS], [" -DCONFIG_64=1 -DANSI=1 -DHAVE_UINT128_T=1"])],
[ansi64], [AS_VAR_APPEND([LIBMPDEC_CFLAGS], [" -DCONFIG_64=1 -DANSI=1"])],
[ppro], [AS_VAR_APPEND([LIBMPDEC_CFLAGS], [" -DCONFIG_32=1 -DANSI=1 -DASM=1 -Wno-unknown-pragmas"])],
[ansi32], [AS_VAR_APPEND([LIBMPDEC_CFLAGS], [" -DCONFIG_32=1 -DANSI=1"])],
[ansi-legacy], [AS_VAR_APPEND([LIBMPDEC_CFLAGS], [" -DCONFIG_32=1 -DANSI=1 -DLEGACY_COMPILER=1"])],
[universal], [AS_VAR_APPEND([LIBMPDEC_CFLAGS], [" -DUNIVERSAL=1"])],
[AC_MSG_ERROR([_decimal: unsupported architecture])]
)

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
Copy link
Contributor

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

@erlend-aasland
Copy link
Contributor

erlend-aasland commented Oct 24, 2022

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
Copy link
Contributor

erlend-aasland commented Oct 24, 2022

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

@erlend-aasland
Copy link
Contributor

erlend-aasland commented Oct 24, 2022

From #98557 (comment):

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
Copy link
Contributor Author

Bo98 commented Oct 24, 2022

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
Copy link
Contributor

erlend-aasland commented Oct 24, 2022

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
Copy link

carlocab commented Oct 4, 2024

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

@erlend-aasland
Copy link
Contributor

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
Copy link
Contributor Author

Bo98 commented Oct 14, 2024

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
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build The build process and cross-build type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

No branches or pull requests

4 participants