Skip to content

Isolate macOS wheel builds from Homebrew #8497

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.

Sign up for GitHub

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

Merged
merged 30 commits into from
Nov 18, 2024
Merged

Isolate macOS wheel builds from Homebrew #8497

Show file tree
Hide file tree
Changes from 1 commit
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
Prev Previous commit
Next Next commit
Revert fribidi/raqm changes for macOS builds.
  • Loading branch information
@freakboy3742
freakboy3742 committed Oct 28, 2024
commit 08554684b3f382f3d729db5e6cd1fe94126dab66
5 changes: 0 additions & 5 deletions .github/workflows/wheels-dependencies.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,11 +155,6 @@ function build {
fi

build_harfbuzz

if [ -n "$IS_MACOS" ]; then
build_simple fribidi $FRIBIDI_VERSION https://github.com/fribidi/fribidi/releases/download/v$FRIBIDI_VERSION tar.xz --enable-shared
build_raqm
fi
}

# Perform all dependency builds in the build subfolder.
Expand Down
21 changes: 16 additions & 5 deletions .github/workflows/wheels-test.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,24 @@
#!/bin/bash
set -e

# For Unix, ensure fribidi is installed by the system.
if [[ "$OSTYPE" != "darwin"* ]]; then
if [ "${AUDITWHEEL_POLICY::9}" == "musllinux" ]; then
apk add curl fribidi
# Ensure fribidi is installed by the system.
if [[ "$OSTYPE" == "darwin"* ]]; then
# If Homebrew is on the path during the build, it may leak into the wheels.
# However, we need a *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
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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

Copy link
Contributor Author

@freakboy3742 freakboy3742 Oct 30, 2024 •
edited
Loading

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.

Copy link
Contributor Author

@freakboy3742 freakboy3742 Oct 31, 2024 •
edited
Loading

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.

else
yum install -y fribidi
HOMEBREW_HOME=/opt/homebrew
fi
$HOMEBREW_HOME/bin/brew install fribidi

# Add the Homebrew lib folder so that vendored libraries can be found.
export DYLD_LIBRARY_PATH=$HOMEBREW_HOME/lib
elif [ "${AUDITWHEEL_POLICY::9}" == "musllinux" ]; then
apk add curl fribidi
else
yum install -y fribidi
fi

python3 -m pip install numpy
Expand Down
4 changes: 2 additions & 2 deletions pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,8 +95,8 @@ before-all = ".github/workflows/wheels-dependencies.sh"
build-verbosity = 1

config-settings = "raqm=enable raqm=vendor fribidi=vendor imagequant=disable"
# Add an explicit dependencies prefix for macOS, and don't request vendored libraries.
macos.config-settings = "raqm=enable imagequant=disable dependencies-prefix=./build/deps"
# Add an explicit dependencies prefix for macOS.
macos.config-settings = "raqm=enable raqm=vendor fribidi=vendor imagequant=disable dependencies-prefix=./build/deps"

test-command = "cd {project} && .github/workflows/wheels-test.sh"
test-extras = "tests"
Expand Down
2 changes: 1 addition & 1 deletion setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -454,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"])
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