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

Unvendor libmpdec sources #115119

Open
3 of 11 tasks
zware opened this issue Feb 7, 2024 · 15 comments
Open
3 of 11 tasks

Unvendor libmpdec sources #115119

zware opened this issue Feb 7, 2024 · 15 comments
Assignees
Labels
build The build process and cross-build extension-modules C modules in the Modules dir

Comments

@zware
Copy link
Member

zware commented Feb 7, 2024

To facilitate cleaner updates of the externally-maintained libmpdec required by the _decimal module, we should migrate away from the bundled copy in Modules/_decimal/libmpdec and towards an "external" in cpython-source-deps for Windows and --with-system-libmpdec by default elsewhere.

My tentative plan is as follows:

Linked PRs

@zware zware added the build The build process and cross-build label Feb 7, 2024
@zware zware self-assigned this Feb 7, 2024
@erlend-aasland erlend-aasland added the extension-modules C modules in the Modules dir label Feb 7, 2024
@erlend-aasland
Copy link
Contributor

[...] with a fallback to the bundled copy (maybe with a warning?)

+1 for an autoconf warning.

@ned-deily
Copy link
Member

Adding an external libmpdec to the macOS installer build shouldn't be a big issue. Assign it to me if you proceed with this.

@erlend-aasland
Copy link
Contributor

FTR: the source deps repo is now updated.

@erlend-aasland
Copy link
Contributor

@ned-deily: for build-installer.py, is there more to do than adding a configuration for mpdecimal? I've used the following configure options: ['--disable-cxx', 'MACHINE=universal']

@ned-deily
Copy link
Member

Likely yes, I'll take care of it. Thanks.

@ned-deily
Copy link
Member

I took a quick look at this and be aware that this change proposal will likely impact everyone building Python 3.13 on macOS. macOS does not ship a copy of libmpdec so, if the bundled version is removed, anyone building Python on macOS will now need to provide a local copy, either building from source or using a version from one of the widely-used third-party package managers, like Homebrew or MacPorts. Both currently have mpdecimal packages at the 4.0.0 level. (Recall that, unlike Windows builds, we do not currently provide non-vendored third-party library binaries for macOS users, other than those built for and linked into the Pythons provided by python.org installers for macOS.) In a quick test, it appeared that with ./configure --with-system-libmpdec today, pkg-config info for libmpdec was not being used. If so, that should be corrected. And, in any case, the devguide instructions for installing dependencies will need to be updated to cover libmpdec. We should add all this and get this working before moving on to changing the installer.

@erlend-aasland
Copy link
Contributor

In a quick test, it appeared that with ./configure --with-system-libmpdec today, pkg-config info for libmpdec was not being used. If so, that should be corrected.

Last time I checked, libmpdec did not provide pkgconfig files:

$ ls $(brew --prefix libmpdec)/lib/pkgconfig
ls: /opt/homebrew/opt/mpdecimal/lib/pkgconfig: No such file or directory
$ cd mpdecimal-2.5.1; find . | grep pkg
$

@ned-deily
Copy link
Member

ned-deily commented Feb 13, 2024

With the current 4.0.0 version, it appears both Homebrew and MacPorts ship them:

$ pkg-config libmpdec --cflags
-I/opt/homebrew/Cellar/mpdecimal/4.0.0/include
$ pkg-config libmpdec --libs
-L/opt/homebrew/Cellar/mpdecimal/4.0.0/lib -lmpdec -lm

...

$ pkg-config libmpdec --libs
-L/opt/macports/lib -lmpdec -lm

@erlend-aasland
Copy link
Contributor

Yes, the 4.0.0 version does indeed provide pkg-config configuration files. I'll work on an Autoconf patch.

erlend-aasland added a commit to erlend-aasland/cpython that referenced this issue Feb 13, 2024
Only libmpdec 4.0.0 supports pkg-config.
zware added a commit that referenced this issue Mar 18, 2024
This includes adding what should be a relatively temporary
`Modules/_decimal/windows/mpdecimal.h` shim to choose between `mpdecimal32vc.h`
or `mpdecimal64vc.h` based on which of `CONFIG_64` or `CONFIG_32` is defined.
vstinner pushed a commit to vstinner/cpython that referenced this issue Mar 20, 2024
…-115182)

This includes adding what should be a relatively temporary
`Modules/_decimal/windows/mpdecimal.h` shim to choose between `mpdecimal32vc.h`
or `mpdecimal64vc.h` based on which of `CONFIG_64` or `CONFIG_32` is defined.
adorilson pushed a commit to adorilson/cpython that referenced this issue Mar 25, 2024
…-115182)

This includes adding what should be a relatively temporary
`Modules/_decimal/windows/mpdecimal.h` shim to choose between `mpdecimal32vc.h`
or `mpdecimal64vc.h` based on which of `CONFIG_64` or `CONFIG_32` is defined.
diegorusso pushed a commit to diegorusso/cpython that referenced this issue Apr 17, 2024
…-115182)

This includes adding what should be a relatively temporary
`Modules/_decimal/windows/mpdecimal.h` shim to choose between `mpdecimal32vc.h`
or `mpdecimal64vc.h` based on which of `CONFIG_64` or `CONFIG_32` is defined.
erlend-aasland added a commit that referenced this issue Apr 29, 2024
pkg-config is supported for libmpdec 4.0.0 and newer.
@erlend-aasland
Copy link
Contributor

@zware are we still aiming for 3.13 for the first batch of items?

@erlend-aasland
Copy link
Contributor

In a quick test, it appeared that with ./configure --with-system-libmpdec today, pkg-config info for libmpdec was not being used. If so, that should be corrected.

Resolved by:

@zware
Copy link
Member Author

zware commented Apr 29, 2024

@zware are we still aiming for 3.13 for the first batch of items?

I would like to, but I don't know when I'm going to have a chance to get back to this.

@ned-deily
Copy link
Member

There is an issue with unvendoring on macOS that will affect some users (and this may be something you ran into when testing, @erlend-aasland). For some reason, mpdecimal, for both 2.5.1 and 4.0.0, chooses to define @rpath-prefixed names for the macOS dynamic library install name for the shared libraries it builds. This can cause builds on macOS to fail in some cases unless additional action is taken.

By default, the libmpdec build produces both shared and a static libraries. On macOS, the shared library install name looks like this:

$ otool -L libmpdec.dylib
libmpdec.dylib:
	@rpath/libmpdec.4.dylib (compatibility version 4.0.0, current version 4.0.0)
	/usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1345.100.2)

Note the @rpath. Typically, shared libraries built on macOS have an absolute path by default when installed, for example:

$ otool -L libsqlite3.dylib
libsqlite3.dylib:
	/opt/local/lib/libsqlite3.0.dylib (compatibility version 9.0.0, current version 9.6.0)
	/usr/lib/libz.1.dylib (compatibility version 1.0.0, current version 1.2.12)
	/usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1345.100.2)

An @rpath install name can be useful if you are building libraries that can be installed in arbitrary locations or are being embedded in something else but using an @rpath-based name requires additional configuration when building, like adding an rpath parameter to the linking of main executables.

To further cloud the issue, it appears that Homebrew, when building and installing its mpdecimal package, converts the @rpath/libmpdec.4.dylib install name to a conventional absolute path name:

$ otool -L /usr/local/Cellar/mpdecimal/4.0.0/lib/libmpdec.dylib
/usr/local/Cellar/mpdecimal/4.0.0/lib/libmpdec.dylib:
	/usr/local/opt/mpdecimal/lib/libmpdec.4.dylib (compatibility version 4.0.0, current version 4.0.0)
	/usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1336.61.1)

and, at the moment, MacPorts does not:

$ otool -L /opt/macports/lib/libmpdec.dylib
/opt/macports/lib/libmpdec.dylib:
	@rpath/libmpdec.4.dylib (compatibility version 4.0.0, current version 4.0.0)
	/usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1336.61.1)

Also, like most similar packages providing libraries, mpdecimal by default installs both a shared library and a static (.a) library but the linker will normally always prefer a shared library when both are present in the same directory. Both Homebrew and MacPorts install both.

AFAICT, this leaves the following scenarios as of now when building 3.13 with --with-system-libmpdec enabled:

  1. If no third-party or private copy of libmpdec is available, the Python build fails with:
./Modules/_decimal/_decimal.c:38:10: fatal error: 'mpdecimal.h' file not found
#include <mpdecimal.h>
         ^~~~~~~~~~~~~
1 error generated.
  1. If building with a current MacPorts copy of libmpdec or if building with a private copy that was built without explicitly disabling its shared library option (--disable-shared), the Python build will fail with something like:
[ERROR] _decimal failed to import: dlopen(/path/to/build/lib.macosx-14.4-x86_64-3.13/_decimal.cpython-313-darwin.so, 0x0002): Library not loaded: @rpath/libmpdec.4.dylib
  Referenced from: <A8A8EE60-CE66-309E-A108-472100134C51> /path/to/source/Modules/_decimal.cpython-313-darwin.so
  Reason: no LC_RPATH's found

Following modules built successfully but were removed because they could not be imported:
_decimal
  1. If building with a current Homebrew copy of libmpdec, the Python build succeeds (because Homebrew removed the @rpath in the library install name) and links with the Homebrew installed shared libmpdec, not the static one.

Now there are various workarounds that I can think of to get around the situations where you are using an environment that contains a pre-built libmpdec with the default @rpath-named shared library, like if you are using MacPorts to supply all of the necessary third-party libs. But I can't think of any that doesn't involve some sort of kludge that requires more configuration options and/or creating alternate library directories. While probably most core developers building on macOS today use Homebrew, I believe there are some using MacPorts (besides me) and there are certainly downstream users in this situation as well as the MacPorts project itself. This segment of users could be addressed if the MacPorts project would opt to do what Homebrew is doing and remove the @rpath install name. That still leaves any other users building Python and its third-party libraries from scratch on macOS; they will be affected initially one way or another although they can adopt strategies like ensuring no libmpdec shared library is available to their builds.

The big question I have is how best to document this to minimize the impact on the various categories of users. At this point, I don't have good answers and I feel I can't spend any more time on this prior to feature code cutoff. But we shouldn't impose this change on users until we can fully explain its impact and how to workaround it.

@ned-deily
Copy link
Member

(Opened MacPorts issue: https://trac.macports.org/ticket/69870)

@erlend-aasland
Copy link
Contributor

Thank you for chiming in, Ned! I'll look into how to improve the docs here.

Even with the extra week before the feature freeze, it's going to be hard to land this nicely. I may find some time for this later this week, but I can't (and won't, and should not) promise anything.

There is an issue with unvendoring on macOS that will affect some users (and this may be something you ran into when testing, @erlend-aasland)

Yes, this may explain my issues from some weeks ago.

If no third-party or private copy of libmpdec is available, the Python build fails with [...]

For this case, we should consider marking _decimal as a missing module instead of failing. After all, this is what we do with similar extension modules.

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 extension-modules C modules in the Modules dir
Projects
None yet
Development

No branches or pull requests

3 participants