-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Isolate macOS wheel builds from Homebrew #8497
base: main
Are you sure you want to change the base?
Conversation
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RAQM_VERSION=0.7.1 | |
RAQM_VERSION=0.10.2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
@hugovk /me casually drops this article into the chat :-) FWIW - BeeWare ditched Codecov a couple of years ago specifically because of weird inconsistencies like this one; and although the output of a raw coverage report is marginally less pretty (i.e., no annotated code listings), you can get the same report locally that you get in CI, and you get none of the weird outages and reporting issues that seem to plague Coverage. YMMV; but if there's interest, it's a relatively straightforward switch, especially if coverage reporting is already in place. |
Co-authored-by: Andrew Murray <[email protected]>
The (seemingly inconsistent) Windows/pypy3.10 test failure also fascinates me... is this the same cache issue mentioned by #8513? |
.github/workflows/wheels-test.sh
Outdated
# However, we *do* need Homebrew to provide a copy of fribidi for | ||
# testing purposes so that we can verify the fribidi shim works as expected. | ||
if [[ "$(uname -m)" == "x86_64" ]]; then | ||
HOMEBREW_HOME=/usr/local/homebrew |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HOMEBREW_HOME=/usr/local/homebrew | |
HOMEBREW_HOME=/usr/local |
I would have thought just '/usr/local', based on https://docs.brew.sh/Installation
The script installs Homebrew to its default, supported, best prefix (/opt/homebrew for Apple Silicon, /usr/local for macOS Intel
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Turns out both work (for brew
at least) - Homebrew keeps the "original" versions in /usr/local/homebrew. However, I agree that /usr/local
is the generally intended entry point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I take that back - it does matter (and making this change caused test failures on x86_64).
Firstly, it's possible for homebrew to have packages installed, but not linked. Using the /usr/local/Homebrew
location ensures that the binary can be used regardless (note also the capitalisation fix - doesn't strictly matter on HFS+, but for accuracy).
Secondly, it's possible for other libraries to be in /usr/local/lib
. Using /usr/local/Homebrew/lib
for the DYLD_LIBRARY_PATH
avoids this.
4e0312c
to
51e3623
Compare
Thanks, that looks better. There still seems to be something wrong with freetype on arm64, https://github.com/python-pillow/Pillow/actions/runs/11604707482/job/32314276332?pr=8497#step:5:14308
But it was built with brotli, https://github.com/python-pillow/Pillow/actions/runs/11604707482/job/32314276332?pr=8497#step:5:7768:
We need to support this for #6554. We could also consider compiling libwebp before libtiff so that WEBP-compressed tiff files can be read (AFAIK webp only looks for libtiff so that it can compile companion tools, but does not actually use it in the library - as I mentioned in #6562). But we don't need to do it now since the test was already being skipped on macOS and Linux before this PR.
|
Weird. My immediate guess is that it might have something to do with the fact that a vendored version of fribidi is being used, which might not be triggering brotli support (or at least not triggering the test). I'll take a look and see what is going on.
I saw that, and ultimately gave up trying to fix it - firstly because I was trying to limit the scope of the changes, but also because I'm not sure it can be fixed. libwebp has a dependency on tiff... but then lcms2 needs tiff; and openjpeg needs libpng, tiff, and lcms2. So - I'm not sure this can be fixed (or, at least, it can't be fixed without some sort of 2-pass compile and the use of dynamic linking, which iOS can't use anyway). |
The test skip is triggered by a specific error raised by the freetype library which itself does not use fribidi.
AFAIK all of these dependencies are just for the binary utilities that we don't need so we can disable them with relevant compile flags; the libraries themselves do not have dependencies between them (except for libtiff optionally using libwebp). But I agree that it might be best to leave that for a separate PR. |
@nulano I've found the culprit - Homebrew's copy of freetype was leaking into the test environment. By using This can be fixed by pointing the The update includes 2 other small changes:
|
Thanks, looks good to me now. |
Co-authored-by: Andrew Murray <[email protected]>
Co-authored-by: Andrew Murray <[email protected]>
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was the reason for this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
.github/workflows/wheels-test.sh
Outdated
# installed copy of fribidi is cellared. This ensures we don't pick up the | ||
# Homebrew version of any other library that we're dependent on (most notably, | ||
# freetype). | ||
export DYLD_LIBRARY_PATH=$(dirname $(realpath $HOMEBREW_HOME/lib/libfribidi.dylib)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
export DYLD_LIBRARY_PATH=$(dirname $(realpath $HOMEBREW_HOME/lib/libfribidi.dylib)) | |
if [[ "$(uname -m)" == "x86_64" ]]; then | |
FRIBIDI_DYLIB=$(find /usr/local/Cellar/fribidi -type f -name libfribidi.dylib) | |
else | |
FRIBIDI_DYLIB=$HOMEBREW_HOME/lib/libfribidi.dylib | |
fi | |
export DYLD_LIBRARY_PATH=$(dirname $FRIBIDI_DYLIB) |
https://github.com/python-pillow/Pillow/actions/runs/11752543578/job/32744256384?pr=8497#step:5:9142
realpath: /usr/local/Homebrew/lib/libfribidi.dylib: No such file or directory
libfribidi.dylib isn't located there on the macOS Intel jobs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh... at some point, I've clearly done something to mess up my x86 test machine's Homebrew install, because I've got a /usr/local/Homebrew/Cellar
... but I now see I'm also getting a bunch of warnings because of the existence of that folder.
The find
-based solution you've got here will fail (or be prone to failure) if there's more than one version of fribidi installed, which is possible on user machines; but if HOMEBREW_HOME
is moved back to /usr/local
, the realpath
approach should work.
Disable platform guessing instead of adding dependencies-prefix
PLAT=$CIBW_ARCHS | ||
else | ||
# Build prefix will default to /usr/local | ||
WORKDIR=$(pwd)/build | ||
PLAT=$CIBW_ARCHS | ||
MB_ML_LIBC=${AUDITWHEEL_POLICY::9} | ||
MB_ML_VER=${AUDITWHEEL_POLICY:9} | ||
fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PLAT=$CIBW_ARCHS | |
else | |
# Build prefix will default to /usr/local | |
WORKDIR=$(pwd)/build | |
PLAT=$CIBW_ARCHS | |
MB_ML_LIBC=${AUDITWHEEL_POLICY::9} | |
MB_ML_VER=${AUDITWHEEL_POLICY:9} | |
fi | |
else | |
# Build prefix will default to /usr/local | |
WORKDIR=$(pwd)/build | |
MB_ML_LIBC=${AUDITWHEEL_POLICY::9} | |
MB_ML_VER=${AUDITWHEEL_POLICY:9} | |
fi | |
PLAT=$CIBW_ARCHS |
# Custom tiff build to include jpeg; by default, configure won't include | ||
# headers/libs in the custom macOS prefix. Explicitly disable webp and | ||
# zstd, because on x86_64 macs, it will pick up the Homebrew versions of | ||
# webp and zstd from /usr/local. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this comment be updated to also mention libdeflate being disabled?
The existing macOS
cibuildwheel
configuration 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 ofsudo
to make changes in the/usr/local
tree. This means it is difficult to testcibuildwheel
changes locally, or to recommend usingcibuildwheel
as a local build solution.Secondly, Homebrew builds could be incompatible with Pillow builds. The
MACOSX_DEPLOYMENT_TARGET
for 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
build
folder 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.gitignore
file.Sets up an isolated build prefix in the
build
folder (./build/deps
) where dependencies can be installed, andmake installs
all dependencies into that path.Forces
PATH
to 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-config
to the dependencies. This is the one build tool that autotools and cmake often require that Xcode doesn't provide. Providing a custom build ofpkg-config
also 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 of
libXdmcp
, to avoid a dependency on the Homebrew-provided version.Adds builds offribidi
andraqm
, rather than using the Homebrew versions as "vendored" versions. Thesetup.py
configuration passed in bycibuildwheel
has been modified from the default on other platforms to not use thevendor
version.Updates the pinned multibuild version to the current
devel
branch 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 thelib64
suffix.Adds a
dependencies-prefix
configuration 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.