Isolate macOS wheel builds from Homebrew #8497
Conversation
radarhere
changed the title
| @@ -23,6 +23,8 @@ OPENJPEG_VERSION=2.5.2 | |||
| XZ_VERSION=5.6.3 | |||
| TIFF_VERSION=4.6.0 | |||
| LCMS2_VERSION=2.16 | |||
| RAQM_VERSION=0.7.1 | |||
There was a problem hiding this comment.
| RAQM_VERSION=0.7.1 | |
| RAQM_VERSION=0.10.2 |
There was a problem hiding this comment.
It definitely makes sense to use the most recent available version; but pillow-depends-main doesn't include this version. What's the procedure for updating that repo?
(Related: pkg-config, libXdmcp, and an updated webp sources should probably be added to that project)
There was a problem hiding this comment.
The theory is that pillow-depends-main is just a more reliable source for the dependencies, and PRs should work without it.
Once this is merged, I will likely add them there.
|
The manylinux failures look like they'll be fixed by this PR to multi build. The x86_64 macOS failures are probably a leakage from /usr/local that I haven't accounted for. I'm looking into that one now. I'm not sure how to interpret the coverage drop, though... |
|
@radarhere I've updated the pin for multibuild to include the (just merged) PR reverting the libjpeg-turbo CMAKE_INSTALL_LIBDIR change; and I've addressed some additional locations where Homebrew can leak in on x86_64. |
This has already happened many times in the past, so thanks for working on this.
I'm not sure if I understand what you mean here. We need to use a vendored version of raqm in wheels for license reasons. When using |
Thanks - that's some helpful context. I was seeing the missing fribidi detection during the build and interpreting that as something that was part of the move away from homebrew; but I see now that Pillow is loading from Homebrew by design in this instance. That also might explain the coverage drop (as supplying fribidi means any runtime loading code won't be in use). I'll take another swing at that part (and resolve the linux build issues as well). Out of interest - what's the license issue with raqm? The source code indicates It looks to be MIT licensed... did it used to be GPL (or something else problematic?) |
From #2753 in 2017:
FriBiDi is LGPL. Since then, Raqm added support for Apache-licensed SheenBiDi in 2021: HOST-Oman/libraqm#138, HOST-Oman/libraqm#139. Perhaps we could switch. |
Co-authored-by: Andrew Murray <3112309+radarhere@users.noreply.github.com>
| @@ -448,7 +454,7 @@ def _remove_extension(self, name: str) -> None: | |||
| def get_macos_sdk_path(self) -> str | None: | |||
| try: | |||
| sdk_path = ( | |||
| subprocess.check_output(["xcrun", "--show-sdk-path"]) | |||
| subprocess.check_output(["xcrun", "--show-sdk-path", "--sdk", "macosx"]) | |||
There was a problem hiding this comment.
What was the reason for this change?
There was a problem hiding this comment.
Explicitness. Xcode can manage multiple SDKs (iOS being an obvious one); adding the explicit --sdk macosx SDK ensures that you're definitely getting the macOS one. That will be the default on almost every install of Xcode, but if you have a stray SDKROOT environment variable from some other activity, it could be inadvertently pointing at an iOS, tvOS, visionOS or MacCatalyst SDK.
Disable platform guessing instead of adding dependencies-prefix
| build_libwebp | ||
| CFLAGS=$ORIGINAL_CFLAGS | ||
|
|
||
| build_brotli | ||
|
|
||
| if [ -n "$IS_MACOS" ]; then | ||
| # Custom freetype build | ||
| build_simple freetype $FREETYPE_VERSION https://download.savannah.gnu.org/releases/freetype tar.gz --with-harfbuzz=no | ||
| build_simple freetype $FREETYPE_VERSION https://download.savannah.gnu.org/releases/freetype tar.gz --without-harfbuzz |
There was a problem hiding this comment.
https://gitlab.freedesktop.org/freetype/freetype/-/blob/master/builds/unix/configure.raw?ref_type=heads#L429 mentions --with-harfbuzz, but not --without-harfbuzz. What was the thinking here?
There was a problem hiding this comment.
"--with/--without" handing is a standard feature of autoconf, AFAIK; I think this change got made when I was trying to diagnose some harfbuzz issues. Happy to revert, as it's code churn with no explicit purpose.
| @@ -77,67 +108,88 @@ function build { | |||
| if [ -n "$IS_MACOS" ]; then | |||
| build_simple xorgproto 2024.1 https://www.x.org/pub/individual/proto | |||
| build_simple libXau 1.0.11 https://www.x.org/pub/individual/lib | |||
| build_simple libXdmcp 1.1.5 https://www.x.org/pub/individual/lib | |||
There was a problem hiding this comment.
Adds a build of
libXdmcp, to avoid a dependency on the Homebrew-provided version.
I suspect you took out all the brew remove statements, then noticed this was being pulled from brew, and then added it here to replace that.
I'm saying that this wouldn't be part of the Pillow wheels at the moment, so if it's still not after this PR, that sounds fine to me.
This function
Line 834 in 5bff2f3
There was a problem hiding this comment.
My recollection was that Xdmcp was needed for XCB to compile at all; but I've just removed it and done a clean build, and everything worked as expected (AFAICT). I'll remove it.
|
Thanks for your patience. This is getting close - aside from my latest comments, I'm happy with the current state. |
|
@radarhere No problem at all - thanks for all the reviews. I've just pushed an update that I think addresses all your comments. |
|
Thanks @freakboy3742 @radarhere @nulano ! |
|
Thank you! |
The existing macOS
cibuildwheelconfiguration relies on Homebrew to provide some dependencies. This clearly works in practice, but there are some issues associated with this choice.Firstly, it is destructive when used on a local build machine. The wheel dependency script invokes
brew remove --ignore-dependencies(which can leave the host machine in a broken, and potentially difficult to restore state); and requires the use ofsudoto make changes in the/usr/localtree. This means it is difficult to testcibuildwheelchanges locally, or to recommend usingcibuildwheelas a local build solution.Secondly, Homebrew builds could be incompatible with Pillow builds. The
MACOSX_DEPLOYMENT_TARGETfor Homebrew dependencies is fixed by the Homebrew build chain, rather than the Pillow build tools, so a change in Pillow's configuration may not be satisfied by Homebrew's build configuration. This isn't an issue at present, but it could easily become one in future.Related to this, there is also the potential that a change in a Homebrew recipe could lead to the inadvertent introduction of additional binary libraries into the delocated macOS binaries. This is especially problematic with the fribidi, raqm and freetype dependedencies which are included as "vendor" sources.
However, the real motivation for proposing this change is that it is a precursor for PEP 730-compliant iOS builds. If Homebrew is on the build path when compiling iOS binaries, compiler tooling will often find an ARM64 Homebrew binary and attempt link it into an iOS library - which then (predictably) breaks. So - Homebrew isolation is essential for iOS compilation. Since the build processes for iOS and macOS are very similar, and Homebrew isolation was necessary for iOS, adding Homebrew isolation for macOS will simplify any future iOS patch. It also provides a way to submit iOS support in a series of smaller pieces, rather than a single "monster" patch. Plus, it provides all the side benefits of isolation described above.
An overview of the changes:
Uses the
buildfolder as a location for all dependency builds. This prevents the root checkout from becoming dirtied by dependency artefacts, simplifying cleanup, and avoiding the need for extensive modifications to the.gitignorefile.Sets up an isolated build prefix in the
buildfolder (./build/deps/darwin) where dependencies can be installed, andmake installsall dependencies into that path.Forces
PATHto be a "clean" environment that only includes bare system tools, plus the Python binary being used for the build, and the isolated build prefix. macOS doesn't include most of it's development libraries in /usr or /usr/local, instead using a path provided by the macOS SDK (which is configured as part of the compiler toolchain).Adds a build of
pkg-configto the dependencies. This is the one build tool that autotools and cmake often require that Xcode doesn't provide. Providing a custom build ofpkg-configalso ensures that no dependencies other than the ones provided by cibuildwheel are included. The build recipe that is used is derived from the Homebrew recipe.Adds a build oflibXdmcp, to avoid a dependency on the Homebrew-provided version.Adds builds offribidiandraqm, rather than using the Homebrew versions as "vendored" versions. Thesetup.pyconfiguration passed in bycibuildwheelhas been modified from the default on other platforms to not use thevendorversion.Updates the pinned multibuild version to the current
develbranch hash. This is to incorporate Python3.13 support, plus a number of fixes required to support installs into locations other than/usr/local, and corrections to LIBDIR handling for platforms that use thelib64suffix.Adds adependencies-prefixconfiguration option tosetup.py. This allows the user to provide a specific location to look for dependencies, rather than the series of Fink/MacPorts/Homebrew fallbacks that currently exist.