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

Isolate macOS wheel builds from Homebrew #8497

Merged
merged 30 commits into from
Nov 18, 2024
Merged
Show file tree
Hide file tree
Changes from 29 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
0fe55d6
Isolate macOS build from Homebrew.
freakboy3742 Oct 25, 2024
fc35fcc
Cleanups and typos identified by code review.
freakboy3742 Oct 25, 2024
00809a2
Tweaks to ensure isolation from Homebrew on x86_64.
freakboy3742 Oct 25, 2024
06dbfed
Bump the multibuild version to fix jpeg-turbo issue.
freakboy3742 Oct 25, 2024
5a8373e
Correct a dumb pip invocation error.
freakboy3742 Oct 25, 2024
140a06e
Explicitly disable libdeflate on libtiff.
freakboy3742 Oct 25, 2024
0961d3d
Possible fix for linux build failures.
freakboy3742 Oct 25, 2024
43c34fc
Copy manylinux lib64 files from the correct built prefix.
freakboy3742 Oct 25, 2024
3e4be4b
Merge branch 'main' into homebrew-isolation
radarhere Oct 28, 2024
0855468
Revert fribidi/raqm changes for macOS builds.
freakboy3742 Oct 28, 2024
8308bf3
Bump multibuild to include more cmake changes.
freakboy3742 Oct 28, 2024
c74a5bd
Correct paths used for Linux build.
freakboy3742 Oct 29, 2024
72d81e2
Simplify Linux config by correcting a logic error in macOS config.
freakboy3742 Oct 29, 2024
ec214e4
Can't check IS_MACOS until common_utils is invoked.
freakboy3742 Oct 29, 2024
d1a4f80
Don't use multibuild variables before invoking multibuild.
freakboy3742 Oct 29, 2024
6d13704
Remove stray debug.
freakboy3742 Oct 29, 2024
467f120
Merge branch 'main' into homebrew-isolation
radarhere Oct 29, 2024
c6912f8
Corrected typo in code comment.
freakboy3742 Oct 29, 2024
96ae15c
Merge branch 'main' into homebrew-isolation
freakboy3742 Oct 29, 2024
01270b5
Use the intended entry point for the x86_64 brew binary.
freakboy3742 Oct 30, 2024
51e3623
Revert x86_64 homebrew location change (with explanation).
freakboy3742 Oct 31, 2024
e82b539
Correct handling of vendored fribidi.
freakboy3742 Nov 6, 2024
904416b
Merge branch 'main' into homebrew-isolation
freakboy3742 Nov 6, 2024
4e35852
Correct typo in comment.
freakboy3742 Nov 7, 2024
681a03b
Apply suggestions from code review
freakboy3742 Nov 9, 2024
378df7a
Disable platform guessing instead of adding dependencies-prefix
radarhere Nov 12, 2024
9dc6904
Correct the lookup of libfribidi on x86 macOS installs.
freakboy3742 Nov 13, 2024
0e3eb70
Merge pull request #1 from radarhere/homebrew-isolation
freakboy3742 Nov 13, 2024
54f2334
More tweaks from code review.
freakboy3742 Nov 16, 2024
96b898c
A couple more cleanups from code review.
freakboy3742 Nov 18, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
142 changes: 97 additions & 45 deletions .github/workflows/wheels-dependencies.sh
Original file line number Diff line number Diff line change
@@ -1,11 +1,33 @@
#!/bin/bash
# Define custom utilities
# Test for macOS with [ -n "$IS_MACOS" ]
if [ -z "$IS_MACOS" ]; then
export MB_ML_LIBC=${AUDITWHEEL_POLICY::9}
export MB_ML_VER=${AUDITWHEEL_POLICY:9}

# Setup that needs to be done before multibuild utils are invoked
PROJECTDIR=$(pwd)
if [[ "$(uname -s)" == "Darwin" ]]; then
# Safety check - macOS builds require that CIBW_ARCHS is set, and that it
# only contains a single value (even though cibuildwheel allows multiple
# values in CIBW_ARCHS).
if [[ -z "$CIBW_ARCHS" ]]; then
echo "ERROR: Pillow macOS builds require CIBW_ARCHS be defined."
exit 1
fi
if [[ "$CIBW_ARCHS" == *" "* ]]; then
echo "ERROR: Pillow macOS builds only support a single architecture in CIBW_ARCHS."
exit 1
fi

# Build macOS dependencies in `build/darwin`
# Install them into `build/deps/darwin`
WORKDIR=$(pwd)/build/darwin
BUILD_PREFIX=$(pwd)/build/deps/darwin
else
# Build prefix will default to /usr/local
WORKDIR=$(pwd)/build
MB_ML_LIBC=${AUDITWHEEL_POLICY::9}
MB_ML_VER=${AUDITWHEEL_POLICY:9}
fi
export PLAT=$CIBW_ARCHS
PLAT=$CIBW_ARCHS

# Define custom utilities
source wheels/multibuild/common_utils.sh
source wheels/multibuild/library_builders.sh
if [ -z "$IS_MACOS" ]; then
Expand Down Expand Up @@ -38,35 +60,44 @@ BZIP2_VERSION=1.0.8
LIBXCB_VERSION=1.17.0
BROTLI_VERSION=1.1.0

function build_pkg_config {
if [ -e pkg-config-stamp ]; then return; fi
# This essentially duplicates the Homebrew recipe:
# https://github.com/Homebrew/homebrew-core/blob/master/Formula/p/pkg-config.rb
radarhere marked this conversation as resolved.
Show resolved Hide resolved
ORIGINAL_CFLAGS=$CFLAGS
CFLAGS="$CFLAGS -Wno-int-conversion"
build_simple pkg-config 0.29.2 https://pkg-config.freedesktop.org/releases tar.gz \
--disable-debug --disable-host-tool --with-internal-glib \
--with-pc-path=$BUILD_PREFIX/share/pkgconfig:$BUILD_PREFIX/lib/pkgconfig \
--with-system-include-path=$(xcrun --show-sdk-path --sdk macosx)/usr/include
CFLAGS=$ORIGINAL_CFLAGS
export PKG_CONFIG=$BUILD_PREFIX/bin/pkg-config
touch pkg-config-stamp
}

function build_brotli {
if [ -e brotli-stamp ]; then return; fi
local cmake=$(get_modern_cmake)
local out_dir=$(fetch_unpack https://github.com/google/brotli/archive/v$BROTLI_VERSION.tar.gz brotli-$BROTLI_VERSION.tar.gz)
(cd $out_dir \
&& $cmake -DCMAKE_INSTALL_PREFIX=$BUILD_PREFIX -DCMAKE_INSTALL_NAME_DIR=$BUILD_PREFIX/lib . \
&& $cmake -DCMAKE_INSTALL_PREFIX=$BUILD_PREFIX -DCMAKE_INSTALL_LIBDIR=$BUILD_PREFIX/lib -DCMAKE_INSTALL_NAME_DIR=$BUILD_PREFIX/lib . \
&& make install)
if [[ "$MB_ML_LIBC" == "manylinux" ]]; then
cp /usr/local/lib64/libbrotli* /usr/local/lib
cp /usr/local/lib64/pkgconfig/libbrotli* /usr/local/lib/pkgconfig
fi
touch brotli-stamp
}

function build_harfbuzz {
if [ -e harfbuzz-stamp ]; then return; fi
python3 -m pip install meson ninja

local out_dir=$(fetch_unpack https://github.com/harfbuzz/harfbuzz/releases/download/$HARFBUZZ_VERSION/$HARFBUZZ_VERSION.tar.xz harfbuzz-$HARFBUZZ_VERSION.tar.xz)
(cd $out_dir \
&& meson setup build --buildtype=release -Dfreetype=enabled -Dglib=disabled)
&& meson setup build --prefix=$BUILD_PREFIX --libdir=$BUILD_PREFIX/lib --buildtype=release -Dfreetype=enabled -Dglib=disabled)
(cd $out_dir/build \
&& meson install)
if [[ "$MB_ML_LIBC" == "manylinux" ]]; then
cp /usr/local/lib64/libharfbuzz* /usr/local/lib
fi
touch harfbuzz-stamp
}

function build {
if [[ -n "$IS_MACOS" ]] && [[ "$CIBW_ARCHS" == "arm64" ]]; then
sudo chown -R runner /usr/local
fi
build_xz
if [ -z "$IS_ALPINE" ] && [ -z "$IS_MACOS" ]; then
yum remove -y zlib-devel
Expand All @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

PyImaging_GrabScreenX11(PyObject *self, PyObject *args) {
is the only use of XCB in Pillow, so if it's not necessary, I don't think it needs to be here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

build_simple libpthread-stubs 0.5 https://xcb.freedesktop.org/dist
if [[ "$CIBW_ARCHS" == "arm64" ]]; then
cp /usr/local/share/pkgconfig/xcb-proto.pc /usr/local/lib/pkgconfig
fi
else
sed s/\${pc_sysrootdir\}// /usr/local/share/pkgconfig/xcb-proto.pc > /usr/local/lib/pkgconfig/xcb-proto.pc
sed s/\${pc_sysrootdir\}// $BUILD_PREFIX/share/pkgconfig/xcb-proto.pc > $BUILD_PREFIX/lib/pkgconfig/xcb-proto.pc
fi
build_simple libxcb $LIBXCB_VERSION https://www.x.org/releases/individual/lib

build_libjpeg_turbo
build_tiff
if [ -n "$IS_MACOS" ]; then
# Custom tiff build to include jpeg; by default, configure won't include
# headers/libs in the custom macOS prefix. Explicitly disable webp,
# libdeflate and zstd, because on x86_64 macs, it will pick up the
# Homebrew versions of those libraries from /usr/local.
build_simple tiff $TIFF_VERSION https://download.osgeo.org/libtiff tar.gz \
--with-jpeg-include-dir=$BUILD_PREFIX/include --with-jpeg-lib-dir=$BUILD_PREFIX/lib \
--disable-webp --disable-zstd --disable-libdeflate
radarhere marked this conversation as resolved.
Show resolved Hide resolved
else
build_tiff
fi

build_libpng
build_lcms2
build_openjpeg

ORIGINAL_CFLAGS=$CFLAGS
CFLAGS="$CFLAGS -O3 -DNDEBUG"
if [[ -n "$IS_MACOS" ]]; then
CFLAGS="$CFLAGS -Wl,-headerpad_max_install_names"
fi
radarhere marked this conversation as resolved.
Show resolved Hide resolved
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"--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.

else
build_freetype
fi

build_harfbuzz
}

# Perform all dependency builds in the build subfolder.
mkdir -p $WORKDIR
pushd $WORKDIR > /dev/null

# Any stuff that you need to do before you start building the wheels
# Runs in the root directory of this repository.
curl -fsSL -o pillow-depends-main.zip https://github.com/python-pillow/pillow-depends/archive/main.zip
untar pillow-depends-main.zip

if [[ -n "$IS_MACOS" ]]; then
# libdeflate may cause a minimum target error when repairing the wheel
# libtiff and libxcb cause a conflict with building libtiff and libxcb
# libxau and libxdmcp cause an issue on macOS < 11
# remove cairo to fix building harfbuzz on arm64
# remove lcms2 and libpng to fix building openjpeg on arm64
# remove jpeg-turbo to avoid inclusion on arm64
# remove webp and zstd to avoid inclusion on x86_64
# curl from brew requires zstd, use system curl
brew remove --ignore-dependencies libpng libtiff libxcb libxau libxdmcp curl cairo lcms2 zstd
if [[ "$CIBW_ARCHS" == "arm64" ]]; then
brew remove --ignore-dependencies jpeg-turbo
else
brew remove --ignore-dependencies libdeflate webp
if [[ ! -d $WORKDIR/pillow-depends-main ]]; then
if [[ ! -f $PROJECTDIR/pillow-depends-main.zip ]]; then
echo "Download pillow dependency sources..."
curl -fSL -o $PROJECTDIR/pillow-depends-main.zip https://github.com/python-pillow/pillow-depends/archive/main.zip
fi
echo "Unpacking pillow dependency sources..."
untar $PROJECTDIR/pillow-depends-main.zip
fi

brew install pkg-config
if [[ -n "$IS_MACOS" ]]; then
# Homebrew (or similar packaging environments) install can contain some of
# the libraries that we're going to build. However, they may be compiled
# with a MACOSX_DEPLOYMENT_TARGET that doesn't match what we want to use,
# and they may bring in other dependencies that we don't want. The same will
# be true of any other locations on the path. To avoid conflicts, strip the
# path down to the bare minimum (which, on macOS, won't include any
# development dependencies).
export PATH="$BUILD_PREFIX/bin:$(dirname $(which python3)):/usr/bin:/bin:/usr/sbin:/sbin:/Library/Apple/usr/bin"
export CMAKE_PREFIX_PATH=$BUILD_PREFIX

# Link the brew command into our isolated build directory.
radarhere marked this conversation as resolved.
Show resolved Hide resolved
mkdir -p "$BUILD_PREFIX/bin"
mkdir -p "$BUILD_PREFIX/lib"

# Ensure pkg-config is available
build_pkg_config
# Ensure cmake is available
python3 -m pip install cmake
fi

wrap_wheel_builder build

# Return to the project root to finish the build
popd > /dev/null

# Append licenses
for filename in wheels/dependency_licenses/*; do
echo -e "\n\n----\n\n$(basename $filename | cut -f 1 -d '.')\n" | cat >> LICENSE
Expand Down
20 changes: 16 additions & 4 deletions .github/workflows/wheels-test.sh
Original file line number Diff line number Diff line change
@@ -1,12 +1,24 @@
#!/bin/bash
set -e

# Ensure fribidi is installed by the system.
if [[ "$OSTYPE" == "darwin"* ]]; then
brew install fribidi
export PKG_CONFIG_PATH="/usr/local/opt/openblas/lib/pkgconfig"
if [ -f /opt/homebrew/lib/libfribidi.dylib ]; then
sudo cp /opt/homebrew/lib/libfribidi.dylib /usr/local/lib
# If Homebrew is on the path during the build, it may leak into the wheels.
# 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_PREFIX=/usr/local
else
HOMEBREW_PREFIX=/opt/homebrew
fi
$HOMEBREW_PREFIX/bin/brew install fribidi

# Add the lib folder for fribidi so that the vendored library can be found.
# Don't use $HOMEWBREW_PREFIX/lib directly - use the lib folder where the
# 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_PREFIX/lib/libfribidi.dylib))
elif [ "${AUDITWHEEL_POLICY::9}" == "musllinux" ]; then
apk add curl fribidi
else
Expand Down
5 changes: 5 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ lib64/
parts/
sdist/
var/
wheelhouse/
*.egg-info/
.installed.cfg
*.egg
Expand Down Expand Up @@ -90,5 +91,9 @@ Tests/images/msp
Tests/images/picins
Tests/images/sunraster

# Test and dependency downloads
pillow-depends-main.zip
pillow-test-images.zip

# pyinstaller
*.spec
7 changes: 7 additions & 0 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -93,10 +93,17 @@ version = { attr = "PIL.__version__" }
[tool.cibuildwheel]
before-all = ".github/workflows/wheels-dependencies.sh"
build-verbosity = 1

config-settings = "raqm=enable raqm=vendor fribidi=vendor imagequant=disable"
# Disable platform guessing on macOS
macos.config-settings = "raqm=enable raqm=vendor fribidi=vendor imagequant=disable platform-guessing=disable"

test-command = "cd {project} && .github/workflows/wheels-test.sh"
test-extras = "tests"

[tool.cibuildwheel.macos.environment]
PATH = "$(pwd)/build/deps/darwin/bin:$(dirname $(which python3)):/usr/bin:/bin:/usr/sbin:/sbin:/Library/Apple/usr/bin"

[tool.black]
exclude = "wheels/multibuild"

Expand Down
3 changes: 2 additions & 1 deletion setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -448,7 +448,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"])
Copy link
Member

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?

Copy link
Contributor Author

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.

.strip()
.decode("latin1")
)
Expand Down Expand Up @@ -606,6 +606,7 @@ def build_extensions(self) -> None:
_add_directory(library_dirs, "/usr/X11/lib")
_add_directory(include_dirs, "/usr/X11/include")

# Add the macOS SDK path.
sdk_path = self.get_macos_sdk_path()
if sdk_path:
_add_directory(library_dirs, os.path.join(sdk_path, "usr", "lib"))
Expand Down
Loading