Cross compile for x86_64 macs on arm macs, bump some mac/manylinux deps #3587
Conversation
📝 WalkthroughWalkthroughUpdated manylinux base images; reordered libwebp build steps; bumped multiple third‑party library versions and checksums; migrated freetype/harfbuzz to Meson; converted several Autotools builds to CMake; added macOS x86_64 cross‑compile flags; disabled in-repo glib builds; relaxed a font-size test assertion. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CI as CI/container
participant DL as Downloader
participant SV as SHA512 verifier
participant Build as Build system
participant Sys as Sysroot/install
Note over CI,DL: download updated tarballs and .sha512 manifests
CI->>DL: fetch tarballs
DL-->>CI: tarballs
CI->>SV: verify SHA512
SV-->>CI: ok / fail
alt Meson freetype+harfbuzz flow
CI->>Build: meson setup & build harfbuzz
Build->>Build: meson setup & build freetype (with harfbuzz support)
Build-->>Sys: install artifacts
end
alt CMake-based builds (libwebp, libpng, libsndfile, fluidsynth, etc.)
CI->>Build: cmake + $PG_BASE_CMAKE_FLAGS
Note right of Build: webp: apply SIMD guard via PG_WEBP_EXTRA_ARGS
Build->>Build: make && make install
Build-->>Sys: installed libs
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
buildconfig/manylinux-build/docker_base/portmidi/build-portmidi.sh (1)
6-6: Version bump to 2.0.6 is consistent with checksum manifest.No issues spotted with the download URL or directory naming.
Consider out-of-source CMake to keep the tree clean:
- cmake . $PG_BASE_CMAKE_FLAGS - make - make install + cmake -S . -B _build $PG_BASE_CMAKE_FLAGS + cmake --build _build -- -j"$(nproc)" + cmake --install _buildbuildconfig/manylinux-build/docker_base/freetype/build-freetype.sh (2)
6-12: Version bumps and checksum gate are correct.Downloads/filenames align with freetype.sha512. Good retry logic on curl.
Slightly simplify extraction of HarfBuzz:
-unxz ${HARFBUZZ_NAME}.tar.xz -tar xf ${HARFBUZZ_NAME}.tar +tar -xJf ${HARFBUZZ_NAME}.tar.xz
21-25: Make pkg-config discovery explicit to reduce flakiness.Explicit PKG_CONFIG_PATH helps Meson find earlier-built libs predictably in manylinux.
-meson setup _build $PG_BASE_MESON_FLAGS -Dbrotli=enabled -Dharfbuzz=dynamic \ +PKG_CONFIG_PATH="$PG_DEP_PREFIX/lib/pkgconfig:$PG_DEP_PREFIX/share/pkgconfig:$PKG_CONFIG_PATH" \ + meson setup _build $PG_BASE_MESON_FLAGS -Dbrotli=enabled -Dharfbuzz=dynamic \ -Dpng=enabled -Dzlib=systemIf you switch away from “dynamic” per above, keep the same PKG_CONFIG_PATH addition.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
buildconfig/manylinux-build/docker_base/Dockerfile-aarch64(1 hunks)buildconfig/manylinux-build/docker_base/Dockerfile-i686(1 hunks)buildconfig/manylinux-build/docker_base/Dockerfile-x86_64(1 hunks)buildconfig/manylinux-build/docker_base/freetype/build-freetype.sh(2 hunks)buildconfig/manylinux-build/docker_base/freetype/freetype.sha512(1 hunks)buildconfig/manylinux-build/docker_base/portmidi/build-portmidi.sh(1 hunks)buildconfig/manylinux-build/docker_base/portmidi/portmidi.sha512(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: x86_64 deps
- GitHub Check: arm64 deps
- GitHub Check: msys2 (clang64, clang-x86_64)
- GitHub Check: dev-check
- GitHub Check: x86
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: AMD64
- GitHub Check: build (macos-14)
- GitHub Check: i686
- GitHub Check: x86_64
- GitHub Check: build (windows-latest)
- GitHub Check: aarch64
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: Debian (Bookworm - 12) [s390x]
- GitHub Check: Debian (Bookworm - 12) [armv7]
- GitHub Check: Debian (Bookworm - 12) [armv6]
- GitHub Check: Debian (Bookworm - 12) [ppc64le]
- GitHub Check: debug_coverage (ubuntu-24.04, 3.13.5)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.9.23)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.14.0rc1)
🔇 Additional comments (6)
buildconfig/manylinux-build/docker_base/portmidi/portmidi.sha512 (1)
1-1: Action: verify checksum locally — sha512sum missing in sandbox.Attempted
sha512sum -c portmidi.sha512but sandbox returned "/bin/bash: sha512sum: command not found". From the repo root run:
cd buildconfig/manylinux-build/docker_base/portmidi && sha512sum -c portmidi.sha512buildconfig/manylinux-build/docker_base/freetype/freetype.sha512 (1)
1-2: Cannot verify checksums — referenced archives missingfreetype.sha512 contains updated hashes for freetype-2.14.0.tar.gz and harfbuzz-11.4.5.tar.xz, but those archive files are not present in buildconfig/manylinux-build/docker_base/freetype (sandbox also lacked sha512sum), so checksums could not be validated. Place both archives in that directory and run
sha512sum -c freetype.sha512locally or provide the archives for re-run verification.buildconfig/manylinux-build/docker_base/Dockerfile-aarch64 (1)
2-2: Base image bump to 2025.09.08-1.Pin looks fine; expect toolchain and RPM metadata churn. Keep an eye on cache bust and any ABI drifts in CI.
To sanity-check compiler/GLIBC deltas during the Docker build, echo versions:
RUN gcc --version && ld --version && rpm -q glibcbuildconfig/manylinux-build/docker_base/Dockerfile-i686 (1)
2-2: Base image bump to 2025.09.08-1 (i686).Looks good. i686 often hits subtle linker flags; watch for -m32 pkg-config paths.
Add a quick assert during build:
RUN linux32 pkg-config --modversion freetype2 || { echo "Missing freetype2.pc"; exit 1; }buildconfig/manylinux-build/docker_base/Dockerfile-x86_64 (1)
2-2: Base image bump to 2025.09.08-1 (x86_64).No concerns; consistent across arches.
buildconfig/manylinux-build/docker_base/freetype/build-freetype.sh (1)
36-41: HarfBuzz configure flags look sane for our use-case.Disabling glib/gobject/cairo/chafa/icu/tests/docs matches our minimal needs; keeping freetype enabled is fine once FT is installed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/font_test.py (1)
621-623: PreferassertAlmostEqual(..., delta=1)for clearer failures and consistencySame tolerance, better error messages; matches the style used in
test_get_height.- # test width and height match, with a tolerance for 1 pixel errors - self.assertTrue(w - 1 <= s.get_width() <= w + 1) - self.assertTrue(h - 1 <= s.get_height() <= h + 1) + # match within ±1px tolerance + self.assertAlmostEqual(s.get_width(), w, delta=1) + self.assertAlmostEqual(s.get_height(), h, delta=1)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/font_test.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
test/font_test.py (2)
buildconfig/stubs/pygame/surface.pyi (1)
get_width(739-743)buildconfig/stubs/pygame/font.pyi (1)
get_height(83-83)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: arm64
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: dev-check
- GitHub Check: build (windows-latest)
- GitHub Check: build (macos-14)
- GitHub Check: x86
- GitHub Check: Debian (Bookworm - 12) [armv6]
- GitHub Check: Debian (Bookworm - 12) [ppc64le]
- GitHub Check: Debian (Bookworm - 12) [armv7]
- GitHub Check: Debian (Bookworm - 12) [s390x]
- GitHub Check: debug_coverage (ubuntu-24.04, 3.14.0rc1)
- GitHub Check: msys2 (clang64, clang-x86_64)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.9.23)
- GitHub Check: msys2 (mingw64, x86_64)
- GitHub Check: x86_64
- GitHub Check: msys2 (ucrt64, ucrt-x86_64)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.13.5)
- GitHub Check: i686
- GitHub Check: aarch64
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
buildconfig/macdependencies/build_mac_deps.sh (1)
15-17: Fix PATH for GNU coreutils on Apple Silicon (otherwise readlink -f/sha512sum will fail).Homebrew’s coreutils live under /opt/homebrew on arm64. Without adding that, manylinux sub-scripts using readlink -f and sha512sum will break on M1/M2 runners.
Apply:
-export PATH="/usr/local/opt/coreutils/libexec/gnubin:$PATH" +export PATH="/usr/local/opt/coreutils/libexec/gnubin:/opt/homebrew/opt/coreutils/libexec/gnubin:$PATH"
🧹 Nitpick comments (1)
buildconfig/manylinux-build/docker_base/libjpegturbo/build-jpeg-turbo.sh (1)
9-9: Have curl fail on HTTP errors.Small hardening so bad URLs don’t silently pass.
Apply:
-curl -sL --retry 10 https://github.com/libjpeg-turbo/libjpeg-turbo/releases/download/${JPEG_VERSION}/${JPEG}.tar.gz > ${JPEG}.tar.gz +curl -fsSL --retry 10 --retry-delay 2 --retry-connrefused \ + "https://github.com/libjpeg-turbo/libjpeg-turbo/releases/download/${JPEG_VERSION}/${JPEG}.tar.gz" \ + -o "${JPEG}.tar.gz"
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
buildconfig/macdependencies/build_mac_deps.sh(1 hunks)buildconfig/manylinux-build/docker_base/libjpegturbo/build-jpeg-turbo.sh(1 hunks)buildconfig/manylinux-build/docker_base/libjpegturbo/libjpegturbo.sha512(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: dev-check
- GitHub Check: Debian (Bookworm - 12) [armv6]
- GitHub Check: Debian (Bookworm - 12) [armv7]
- GitHub Check: Debian (Bookworm - 12) [s390x]
- GitHub Check: Debian (Bookworm - 12) [ppc64le]
- GitHub Check: msys2 (clang64, clang-x86_64)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.14.0rc1)
- GitHub Check: msys2 (ucrt64, ucrt-x86_64)
- GitHub Check: msys2 (mingw64, x86_64)
- GitHub Check: i686
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.13.5)
- GitHub Check: x86_64
- GitHub Check: build (windows-latest)
- GitHub Check: AMD64
- GitHub Check: aarch64
- GitHub Check: build (macos-14)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.9.23)
- GitHub Check: x86
- GitHub Check: build (ubuntu-22.04)
🔇 Additional comments (3)
buildconfig/macdependencies/build_mac_deps.sh (1)
49-51: Yasm switch for x86_64 libjpeg-turbo builds: OKThis is a fine choice; libjpeg-turbo’s CMake will happily use yasm on x86_64.
buildconfig/manylinux-build/docker_base/libjpegturbo/libjpegturbo.sha512 (1)
1-1: Checksum verified — libjpeg-turbo 3.1.2 SHA-512 matches downloaded tarball.
buildconfig/manylinux-build/docker_base/libjpegturbo/libjpegturbo.sha512 equals libjpeg-turbo-3.1.2.tar.gz; no changes required.buildconfig/manylinux-build/docker_base/libjpegturbo/build-jpeg-turbo.sh (1)
6-6: Version bump to 3.1.2: LGTM.Matches the updated checksum file.
buildconfig/manylinux-build/docker_base/libjpegturbo/build-jpeg-turbo.sh
Show resolved
Hide resolved
ankith26
force-pushed
the
ankith26-deps-updates
branch
from
5646178 to
c7adc90
Compare
ankith26
changed the title
ankith26
force-pushed
the
ankith26-deps-updates
branch
from
c7adc90 to
a1c6527
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
buildconfig/manylinux-build/docker_base/libwebp/build-webp.sh (1)
2-12: Harden the downloader and shell safety (HTTPS, fail-fast, quoting).Use HTTPS and stricter curl flags; add pipefail/nounset; fix quoting to avoid path/globbing issues.
Apply:
- set -e -x + set -euo pipefail -x @@ - cd $(dirname `readlink -f "$0"`) + cd "$(dirname "$(readlink -f "$0")")" @@ - curl -sL --retry 10 http://storage.googleapis.com/downloads.webmproject.org/releases/webp/${WEBP}.tar.gz > ${WEBP}.tar.gz + curl -fsSL --retry 10 --retry-all-errors --connect-timeout 30 --max-time 300 \ + --proto '=https' --tlsv1.2 \ + "https://storage.googleapis.com/downloads.webmproject.org/releases/webp/${WEBP}.tar.gz" \ + -o "${WEBP}.tar.gz" @@ - tar xzf ${WEBP}.tar.gz + tar xzf "${WEBP}.tar.gz" - cd $WEBP + cd "$WEBP"
🧹 Nitpick comments (2)
buildconfig/manylinux-build/docker_base/sndfile/build-sndfile.sh (1)
15-17: CMake switch looks good; consider out-of-source build + explicit flags for reproducibility and speed
- Avoid in-source builds and use cmake --build/--install.
- Disable tests explicitly; keep only shared libs; parallelize build.
- This also helps prevent accidental pull-in of optional tools during test discovery.
Please confirm PG_BASE_CMAKE_FLAGS already sets CMAKE_INSTALL_PREFIX and CMAKE_INSTALL_LIBDIR=lib (not lib64) to match manylinux expectations.
Apply:
-cmake . $PG_BASE_CMAKE_FLAGS -DBUILD_PROGRAMS=OFF -DBUILD_EXAMPLES=OFF -make -make install +cmake -S . -B build $PG_BASE_CMAKE_FLAGS \ + -DBUILD_PROGRAMS=OFF \ + -DBUILD_EXAMPLES=OFF \ + -DBUILD_TESTING=OFF \ + -DBUILD_SHARED_LIBS=ON +cmake --build build -j"$(nproc)" +cmake --install buildbuildconfig/manylinux-build/docker_base/libwebp/build-webp.sh (1)
14-16: Prefer out-of-tree CMake; use cmake --build / --install; do not set non‑existent WEBP_BUILD_LIBWEBPDEMUXFile: buildconfig/manylinux-build/docker_base/libwebp/build-webp.sh Lines: 14-16
Replace the in-tree sequence:
cmake . $PG_BASE_CMAKE_FLAGS
make
make installwith an out-of-tree build and the modern build/install commands (example):
cmake -S . -B build $PG_BASE_CMAKE_FLAGS
cmake --build build --parallel
cmake --install buildReason: more deterministic and avoids generator-specific make targets. In libwebp 1.6.0 WEBP_BUILD_LIBWEBPMUX and WEBP_BUILD_WEBPMUX exist and default ON; libwebpdemux is added unconditionally (no CMake option), so remove the proposed -DWEBP_BUILD_LIBWEBPDEMUX=ON (invalid).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
buildconfig/macdependencies/build_mac_deps.sh(0 hunks)buildconfig/manylinux-build/docker_base/libjpegturbo/build-jpeg-turbo.sh(1 hunks)buildconfig/manylinux-build/docker_base/libjpegturbo/libjpegturbo.sha512(1 hunks)buildconfig/manylinux-build/docker_base/libwebp/build-webp.sh(1 hunks)buildconfig/manylinux-build/docker_base/libwebp/webp.sha512(1 hunks)buildconfig/manylinux-build/docker_base/sndfile/build-sndfile.sh(1 hunks)
💤 Files with no reviewable changes (1)
- buildconfig/macdependencies/build_mac_deps.sh
✅ Files skipped from review due to trivial changes (1)
- buildconfig/manylinux-build/docker_base/libjpegturbo/libjpegturbo.sha512
🚧 Files skipped from review as they are similar to previous changes (1)
- buildconfig/manylinux-build/docker_base/libjpegturbo/build-jpeg-turbo.sh
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: Debian (Bookworm - 12) [armv7]
- GitHub Check: Debian (Bookworm - 12) [s390x]
- GitHub Check: Debian (Bookworm - 12) [armv6]
- GitHub Check: Debian (Bookworm - 12) [ppc64le]
- GitHub Check: msys2 (clang64, clang-x86_64)
- GitHub Check: msys2 (ucrt64, ucrt-x86_64)
- GitHub Check: dev-check
- GitHub Check: msys2 (mingw64, x86_64)
- GitHub Check: x86_64 deps
- GitHub Check: arm64 deps
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: x86
- GitHub Check: AMD64
- GitHub Check: debug_coverage (ubuntu-24.04, 3.14.0rc1)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.13.5)
- GitHub Check: i686
- GitHub Check: aarch64
- GitHub Check: x86_64
- GitHub Check: debug_coverage (ubuntu-24.04, 3.9.23)
🔇 Additional comments (2)
buildconfig/manylinux-build/docker_base/libwebp/webp.sha512 (1)
1-1: LGTM: hash entry matches tarball name.Filename aligns with the script’s WEBP var (libwebp-1.6.0). Looks good.
buildconfig/manylinux-build/docker_base/libwebp/build-webp.sh (1)
6-6: Version bump var matches checksum filename.
WEBP=libwebp-1.6.0is consistent with webp.sha512.
ankith26
force-pushed
the
ankith26-deps-updates
branch
from
a1c6527 to
c2a5f74
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
buildconfig/macdependencies/clean_usr_local.sh (1)
15-39: Reduce duplication: loop over brew prefixes and targets.The list is getting long and easy to drift across
/usr/localvs/opt/homebrew. A tiny loop keeps both prefixes in sync and simplifies future edits.Example pattern:
# At top of the cleanup block BREW_PREFIXES=(/usr/local /opt/homebrew) # Libraries for P in "${BREW_PREFIXES[@]}"; do rm -rf "$P/lib/libtiff*" "$P/lib/libzstd*" "$P/lib/libwebp*" "$P/lib/libdeflate*" \ "$P/lib/libsndfile*" "$P/lib/glib*" "$P/lib/libglib*" "$P/lib/libgthread*" \ "$P/lib/libintl*" "$P/lib/libbrotlidec*" "$P/lib/libopus*" "$P/lib/freetype*" \ "$P/lib/libgif*" done # Cellars for P in "${BREW_PREFIXES[@]}"; do rm -rf "$P/Cellar/libtiff" "$P/Cellar/libsndfile" "$P/Cellar/libdeflate"* \ "$P/Cellar/glib" "$P/Cellar/brotli" "$P/Cellar/pcre"* \ "$P/Cellar/opusfile" "$P/Cellar/opus" "$P/Cellar/freetype"* \ "$P/Cellar/giflib" "$P/Cellar/zstd" done
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
buildconfig/macdependencies/build_mac_deps.sh(0 hunks)buildconfig/macdependencies/clean_usr_local.sh(2 hunks)buildconfig/manylinux-build/docker_base/libjpegturbo/build-jpeg-turbo.sh(1 hunks)buildconfig/manylinux-build/docker_base/libjpegturbo/libjpegturbo.sha512(1 hunks)buildconfig/manylinux-build/docker_base/libwebp/build-webp.sh(1 hunks)buildconfig/manylinux-build/docker_base/libwebp/webp.sha512(1 hunks)buildconfig/manylinux-build/docker_base/sndfile/build-sndfile.sh(1 hunks)
💤 Files with no reviewable changes (1)
- buildconfig/macdependencies/build_mac_deps.sh
✅ Files skipped from review due to trivial changes (1)
- buildconfig/manylinux-build/docker_base/libwebp/webp.sha512
🚧 Files skipped from review as they are similar to previous changes (4)
- buildconfig/manylinux-build/docker_base/libjpegturbo/build-jpeg-turbo.sh
- buildconfig/manylinux-build/docker_base/sndfile/build-sndfile.sh
- buildconfig/manylinux-build/docker_base/libjpegturbo/libjpegturbo.sha512
- buildconfig/manylinux-build/docker_base/libwebp/build-webp.sh
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.14.0rc1)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.13.5)
- GitHub Check: Debian (Bookworm - 12) [armv7]
- GitHub Check: Debian (Bookworm - 12) [s390x]
- GitHub Check: Debian (Bookworm - 12) [ppc64le]
- GitHub Check: x86_64 deps
- GitHub Check: Debian (Bookworm - 12) [armv6]
- GitHub Check: dev-check
- GitHub Check: build (windows-latest)
- GitHub Check: arm64 deps
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: aarch64
- GitHub Check: i686
- GitHub Check: x86_64
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: msys2 (mingw64, x86_64)
- GitHub Check: msys2 (ucrt64, ucrt-x86_64)
- GitHub Check: msys2 (clang64, clang-x86_64)
- GitHub Check: x86
- GitHub Check: AMD64
🔇 Additional comments (2)
buildconfig/macdependencies/clean_usr_local.sh (2)
16-16: Purge zstd Cellar dirs for parity with other brewsAdd to the Cellar removal section in buildconfig/macdependencies/clean_usr_local.sh:
rm -rf /usr/local/Cellar/zstd /opt/homebrew/Cellar/zstdSandbox returned no evidence of these paths — verify on a macOS host with:
#!/bin/bash set -euo pipefail for p in /usr/local /opt/homebrew; do for d in "$p/lib" "$p/Cellar"; do [ -d "$d" ] || continue echo "Checking $d for zstd..." find "$d" -maxdepth 2 \( -name 'libzstd*' -o -name 'zstd' \) -print || true done done
28-28: Remove libgif from /usr/local (Intel) and optionally purge Cellar — verify locallyFile: buildconfig/macdependencies/clean_usr_local.sh — change the single-prefix removal so Intel machines don't keep /usr/local/lib/libgif*.
Apply diff:
-rm -rf /opt/homebrew/lib/libgif* +rm -rf /usr/local/lib/libgif* /opt/homebrew/lib/libgif*Optional Cellar purge:
rm -rf /usr/local/Cellar/giflib /opt/homebrew/Cellar/giflibVerification to run on a Mac/CI runner (this environment produced no results):
#!/bin/bash set -euo pipefail for p in /usr/local /opt/homebrew; do for d in "$p/lib" "$p/Cellar"; do [ -d "$d" ] || continue echo "Checking $d for gif..." find "$d" -maxdepth 2 \( -name 'libgif*' -o -name 'giflib*' \) -print || true done done
ankith26
force-pushed
the
ankith26-deps-updates
branch
2 times, most recently
from
a2b9db5 to
c267027
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
buildconfig/manylinux-build/docker_base/mpg123/build-mpg123.sh (6)
8-8: Harden the download step (fail fast + robust retries).Add --fail, quote the output, and consider retrying on all errors.
-curl -sL --retry 10 https://downloads.sourceforge.net/sourceforge/mpg123/${MPG123}.tar.bz2 > ${MPG123}.tar.bz2 +curl -fsSL --retry 10 --retry-all-errors --connect-timeout 15 --max-time 600 \ + "https://downloads.sourceforge.net/sourceforge/mpg123/${MPG123}.tar.bz2" \ + -o "${MPG123}.tar.bz2"
2-2: Use stricter bash safety flags.Prevents unbound vars and pipe swallow; preserves -x behavior.
-set -e -x +set -Eeuo pipefail +set -x
4-4: Quote command substitutions to avoid path splitting.Safer if directories contain spaces (defensive habit).
-cd $(dirname `readlink -f "$0"`) +cd "$(dirname "$(readlink -f "$0")")"
11-13: Simplify extraction (one step) and keep quoting.Functional equivalent; slightly cleaner.
-bzip2 -d ${MPG123}.tar.bz2 -tar xf ${MPG123}.tar -cd $MPG123 +tar xjf "${MPG123}.tar.bz2" +cd "${MPG123}"
18-19: Optional: parallelize make.Speeds CI builds without affecting determinism.
-make -make install +make -j"$(nproc)" +make install
8-9: Optional: add upstream authenticity check (GPG) in addition to SHA512.If mpg123 publishes .asc signatures, verify with their release key to defend against mirror tampering.
Would you like a small snippet to import the mpg123 release key and verify ${MPG123}.tar.bz2.asc?
buildconfig/manylinux-build/docker_base/zlib-ng/build-zlib-ng.sh (1)
6-6: Optional hardening: curl flags, quoting, and out‑of‑source CMake.Consider modest robustness tweaks:
- Make curl fail loudly and retry on more transient errors; use
-oinstead of shell redirection.- Quote variable expansions to avoid accidental globbing.
- Use out‑of‑source CMake build and parallelize via
$(nproc).Example (non‑diff snippet):
set -euo pipefail set -x ZLIB_NG_VER=2.2.5 ZLIB_NG_NAME="zlib-ng-$ZLIB_NG_VER" curl -fL --retry 10 --retry-delay 5 --retry-all-errors \ -o "${ZLIB_NG_NAME}.tar.gz" \ "https://github.com/zlib-ng/zlib-ng/archive/refs/tags/${ZLIB_NG_VER}.tar.gz" sha512sum -c zlib-ng.sha512 tar -xf "${ZLIB_NG_NAME}.tar.gz" cd "${ZLIB_NG_NAME}" cmake -S . -B build $PG_BASE_CMAKE_FLAGS -DZLIB_COMPAT=1 cmake --build build -j"$(nproc)" cmake --install build
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
buildconfig/macdependencies/build_mac_deps.sh(0 hunks)buildconfig/macdependencies/clean_usr_local.sh(2 hunks)buildconfig/manylinux-build/docker_base/Dockerfile-aarch64(2 hunks)buildconfig/manylinux-build/docker_base/Dockerfile-i686(2 hunks)buildconfig/manylinux-build/docker_base/Dockerfile-x86_64(2 hunks)buildconfig/manylinux-build/docker_base/libjpegturbo/build-jpeg-turbo.sh(1 hunks)buildconfig/manylinux-build/docker_base/libjpegturbo/libjpegturbo.sha512(1 hunks)buildconfig/manylinux-build/docker_base/libwebp/build-webp.sh(1 hunks)buildconfig/manylinux-build/docker_base/libwebp/webp.sha512(1 hunks)buildconfig/manylinux-build/docker_base/mpg123/build-mpg123.sh(1 hunks)buildconfig/manylinux-build/docker_base/mpg123/mpg123.sha512(1 hunks)buildconfig/manylinux-build/docker_base/sndfile/build-sndfile.sh(1 hunks)buildconfig/manylinux-build/docker_base/zlib-ng/build-zlib-ng.sh(1 hunks)buildconfig/manylinux-build/docker_base/zlib-ng/zlib-ng.sha512(1 hunks)
💤 Files with no reviewable changes (1)
- buildconfig/macdependencies/build_mac_deps.sh
✅ Files skipped from review due to trivial changes (4)
- buildconfig/manylinux-build/docker_base/mpg123/mpg123.sha512
- buildconfig/manylinux-build/docker_base/libjpegturbo/libjpegturbo.sha512
- buildconfig/manylinux-build/docker_base/libwebp/webp.sha512
- buildconfig/manylinux-build/docker_base/libjpegturbo/build-jpeg-turbo.sh
🚧 Files skipped from review as they are similar to previous changes (6)
- buildconfig/macdependencies/clean_usr_local.sh
- buildconfig/manylinux-build/docker_base/Dockerfile-i686
- buildconfig/manylinux-build/docker_base/Dockerfile-x86_64
- buildconfig/manylinux-build/docker_base/sndfile/build-sndfile.sh
- buildconfig/manylinux-build/docker_base/Dockerfile-aarch64
- buildconfig/manylinux-build/docker_base/libwebp/build-webp.sh
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.14.0rc1)
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: Debian (Bookworm - 12) [ppc64le]
- GitHub Check: Debian (Bookworm - 12) [armv7]
- GitHub Check: Debian (Bookworm - 12) [armv6]
- GitHub Check: Debian (Bookworm - 12) [s390x]
- GitHub Check: AMD64
- GitHub Check: x86
- GitHub Check: dev-check
- GitHub Check: aarch64
- GitHub Check: x86_64
- GitHub Check: arm64 deps
- GitHub Check: x86_64 deps
- GitHub Check: i686
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: msys2 (clang64, clang-x86_64)
- GitHub Check: msys2 (mingw64, x86_64)
- GitHub Check: msys2 (ucrt64, ucrt-x86_64)
🔇 Additional comments (4)
buildconfig/manylinux-build/docker_base/mpg123/build-mpg123.sh (2)
15-16: Confirm configure flags still exist in 1.33.2.Flags like --disable-components and --enable-int-quality can change across releases; please verify they’re recognized to avoid a no-op build config.
6-6: Checksum file updated — no action required.
buildconfig/manylinux-build/docker_base/mpg123/mpg123.sha512 contains only mpg123-1.33.2.tar.bz2 and the 1.33.0 entry is absent, so sha512sum -c will pass.buildconfig/manylinux-build/docker_base/zlib-ng/zlib-ng.sha512 (1)
1-1: Checksum/filename update for 2.2.5 looks correct.Entry format matches
sha512sum -cexpectations and filename aligns with the build script’s tarball name.buildconfig/manylinux-build/docker_base/zlib-ng/build-zlib-ng.sh (1)
6-6: Version bump to 2.2.5: LGTM.Consistent with the checksum entry and subsequent steps; no functional concerns.
ankith26
force-pushed
the
ankith26-deps-updates
branch
from
c267027 to
477d159
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
buildconfig/macdependencies/build_mac_deps.sh (1)
49-51: Revisit NASM use on Intel JPEG builds (prior CI failures).Past discussion noted NASM SIMD on mac runners caused JPEG test failures and was removed. It’s still installed here. If the underlying runner/Rosetta issue isn’t resolved, gate NASM behind an env flag (e.g., PG_ENABLE_NASM=1) or skip for CI.
- brew install nasm + if [[ "${PG_ENABLE_NASM:-0}" == "1" ]]; then + brew install nasm + fi
🧹 Nitpick comments (6)
buildconfig/macdependencies/macos_x86_64.ini (2)
4-8: Consider exe_wrapper for cross runs on Apple Silicon.If any Meson subprojects try to run test binaries during configure, add an exe_wrapper to ensure Rosetta is used:
[host_machine] system = 'darwin' cpu_family = 'x86_64' cpu = 'x86_64' endian = 'little' + +[binaries] +exe_wrapper = ['arch', '-x86_64']
21-27: C++ link flags: verify libc++ linkage.Because cpp/objcpp compile args include -stdlib=libc++, linking should be fine via clang++. If any subproject links with clang instead of clang++, consider mirroring -stdlib=libc++ in cpp_link_args/objcpp_link_args.
buildconfig/macdependencies/build_mac_deps.sh (2)
58-60: Also pass CMAKE_OSX_DEPLOYMENT_TARGET explicitly.Relying on the env var works in most cases, but setting the CMake cache entry makes intent unambiguous and avoids project overrides.
- export PG_BASE_CMAKE_FLAGS="$PG_BASE_CMAKE_FLAGS -DCMAKE_OSX_ARCHITECTURES=x86_64" + export PG_BASE_CMAKE_FLAGS="$PG_BASE_CMAKE_FLAGS -DCMAKE_OSX_ARCHITECTURES=x86_64 -DCMAKE_OSX_DEPLOYMENT_TARGET=10.11"
65-67: ShellCheck SC2155: split command substitution from export.Avoid command substitution inside the export to silence SC2155 and improve debuggability.
- export PG_BASE_MESON_FLAGS="$PG_BASE_MESON_FLAGS --cross-file $(pwd)/macos_x86_64.ini" + cross_file="$(pwd)/macos_x86_64.ini" + export PG_BASE_MESON_FLAGS="$PG_BASE_MESON_FLAGS --cross-file ${cross_file}".github/workflows/build-macos.yml (2)
27-28: macos-15 for both arches: ensure Rosetta for x86_64.On Apple Silicon runners, add a conditional Rosetta install for x86_64 builds to keep cibuildwheel and any target executables happy.
- name: Build Mac Deps if: steps.macdep-cache.outputs.cache-hit != 'true' run: | + if [[ "${{ matrix.macarch }}" == "x86_64" ]]; then + /usr/sbin/softwareupdate --install-rosetta --agree-to-license || true + fi export MAC_ARCH="${{ matrix.macarch }}" brew install coreutils cd buildconfig/macdependencies bash ./build_mac_deps.sh
67-68: Mirror the Rosetta guard in build job.Add the same Rosetta install step before invoking cibuildwheel for the x86_64 matrix entry.
- name: Build and test wheels - uses: pypa/[email protected] + run: | + if [[ "${{ matrix.macarch }}" == "x86_64" ]]; then + /usr/sbin/softwareupdate --install-rosetta --agree-to-license || true + fi + uv run python -m cibuildwheel
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
.github/workflows/build-macos.yml(2 hunks)buildconfig/macdependencies/build_mac_deps.sh(1 hunks)buildconfig/macdependencies/clean_usr_local.sh(2 hunks)buildconfig/macdependencies/macos_x86_64.ini(1 hunks)buildconfig/manylinux-build/docker_base/Dockerfile-aarch64(2 hunks)buildconfig/manylinux-build/docker_base/Dockerfile-i686(2 hunks)buildconfig/manylinux-build/docker_base/Dockerfile-x86_64(2 hunks)buildconfig/manylinux-build/docker_base/libjpegturbo/build-jpeg-turbo.sh(1 hunks)buildconfig/manylinux-build/docker_base/libjpegturbo/libjpegturbo.sha512(1 hunks)buildconfig/manylinux-build/docker_base/libwebp/build-webp.sh(1 hunks)buildconfig/manylinux-build/docker_base/libwebp/webp.sha512(1 hunks)buildconfig/manylinux-build/docker_base/zlib-ng/build-zlib-ng.sh(1 hunks)buildconfig/manylinux-build/docker_base/zlib-ng/zlib-ng.sha512(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- buildconfig/manylinux-build/docker_base/zlib-ng/zlib-ng.sha512
- buildconfig/manylinux-build/docker_base/libjpegturbo/libjpegturbo.sha512
- buildconfig/manylinux-build/docker_base/zlib-ng/build-zlib-ng.sh
- buildconfig/manylinux-build/docker_base/Dockerfile-i686
- buildconfig/manylinux-build/docker_base/Dockerfile-x86_64
- buildconfig/manylinux-build/docker_base/libwebp/webp.sha512
- buildconfig/macdependencies/clean_usr_local.sh
- buildconfig/manylinux-build/docker_base/libwebp/build-webp.sh
- buildconfig/manylinux-build/docker_base/libjpegturbo/build-jpeg-turbo.sh
- buildconfig/manylinux-build/docker_base/Dockerfile-aarch64
🧰 Additional context used
🪛 Shellcheck (0.10.0)
buildconfig/macdependencies/build_mac_deps.sh
[warning] 66-66: Declare and assign separately to avoid masking return values.
(SC2155)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: msys2 (clang64, clang-x86_64)
- GitHub Check: msys2 (ucrt64, ucrt-x86_64)
- GitHub Check: msys2 (mingw64, x86_64)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: build (macos-14)
- GitHub Check: arm64 deps
- GitHub Check: i686
- GitHub Check: AMD64
- GitHub Check: aarch64
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: x86_64
- GitHub Check: x86
- GitHub Check: Debian (Bookworm - 12) [ppc64le]
- GitHub Check: Debian (Bookworm - 12) [armv6]
- GitHub Check: Debian (Bookworm - 12) [armv7]
- GitHub Check: Debian (Bookworm - 12) [s390x]
- GitHub Check: dev-check
- GitHub Check: debug_coverage (ubuntu-24.04, 3.14.0rc1)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.13.5)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.9.23)
🔇 Additional comments (3)
buildconfig/macdependencies/macos_x86_64.ini (1)
2-2: Switch to target triple looks good; confirm Meson version compatibility.Meson’s handling of target triples in c_args is fine from 0.56+, but please confirm the runner’s Meson version meets this. Otherwise, fallback to adding -mmacosx-version-min=10.11 alongside -target.
buildconfig/macdependencies/build_mac_deps.sh (2)
52-55: CC/CXX -target approach: LGTM.Setting CC/CXX with -target x86_64-apple-macos10.11 aligns autotools projects with the intended SDK/deployment target.
56-56: Host triple for autotools: LGTM.--host=x86_64-apple-darwin is correct for cross-compiling from arm64.
ankith26
changed the title
|
This PR should be done from my side now. As it turns out, the |
ankith26
force-pushed
the
ankith26-deps-updates
branch
from
477d159 to
77a7542
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
buildconfig/manylinux-build/docker_base/libjpegturbo/build-jpeg-turbo.sh (1)
4-4: Make path resolution POSIX‑portable and fix quoting.Use a POSIX‑safe alternative to readlink -f and avoid word splitting.
-cd $(dirname `readlink -f "$0"`) +cd "$(cd "$(dirname "$0")" && pwd -P)"
🧹 Nitpick comments (11)
buildconfig/manylinux-build/docker_base/freetype/build-freetype.sh (2)
21-27: Prefer uname over OSTYPE for portability; avoid export.OSTYPE can be unset or vary; uname is more reliable. Also no need to export a local helper var.
Apply:
-if [[ "$OSTYPE" == "darwin"* ]]; then - export PG_FT_BZ2="-Dbzip2=enabled" -else - export PG_FT_BZ2="-Dbzip2=disabled" -fi +case "$(uname -s)" in + Darwin*) PG_FT_BZ2="-Dbzip2=enabled" ;; + *) PG_FT_BZ2="-Dbzip2=disabled" ;; +esac
32-33: You can drop the explicit compile; install builds as needed.
meson installtriggers a build if required; the separatemeson compileis redundant.Apply:
-meson compile -C _build -meson install -C _build +meson install -C _buildbuildconfig/macdependencies/build_mac_deps.sh (3)
52-55: Prefer putting -target in CFLAGS/CXXFLAGS, not CC/CXX.Embedding flags in CC/CXX can confuse CMake/autoconf detection. Keep CC/CXX as compilers and move -target to CFLAGS/CXXFLAGS.
Apply this diff:
- export CC="clang -target x86_64-apple-macos10.11" - export CXX="clang++ -target x86_64-apple-macos10.11" + export CC="clang" + export CXX="clang++" + export CFLAGS="$CFLAGS -target x86_64-apple-macos10.11" + export CXXFLAGS="$CXXFLAGS -target x86_64-apple-macos10.11"
59-60: Propagate deployment target through CMake as well.Some projects ignore MACOSX_DEPLOYMENT_TARGET env. Set CMAKE_OSX_DEPLOYMENT_TARGET to ensure consistency.
Apply this diff:
- export PG_BASE_CMAKE_FLAGS="$PG_BASE_CMAKE_FLAGS -DCMAKE_OSX_ARCHITECTURES=x86_64" + export PG_BASE_CMAKE_FLAGS="$PG_BASE_CMAKE_FLAGS -DCMAKE_OSX_ARCHITECTURES=x86_64 -DCMAKE_OSX_DEPLOYMENT_TARGET=10.11"
65-67: Fix ShellCheck SC2155: avoid command substitution in export.Use
$PWD instead of $ (pwd) in the export to silence SC2155 and avoid subshells.Apply this diff:
- export PG_BASE_MESON_FLAGS="$PG_BASE_MESON_FLAGS --cross-file $(pwd)/macos_x86_64.ini" + export PG_BASE_MESON_FLAGS="$PG_BASE_MESON_FLAGS --cross-file ${PWD}/macos_x86_64.ini"buildconfig/manylinux-build/docker_base/libjpegturbo/build-jpeg-turbo.sh (4)
9-9: Harden curl: fail on HTTP errors and quote expansions.Prevents silent 404/5xx and handles spaces safely.
-curl -sL --retry 10 https://github.com/libjpeg-turbo/libjpeg-turbo/releases/download/${JPEG_VERSION}/${JPEG}.tar.gz > ${JPEG}.tar.gz +curl -fsSL --retry 10 --retry-all-errors "https://github.com/libjpeg-turbo/libjpeg-turbo/releases/download/${JPEG_VERSION}/${JPEG}.tar.gz" -o "${JPEG}.tar.gz"
2-2: Stricter shell flags for robustness.Catch unset vars and pipeline failures.
-set -e -x +set -Eeuo pipefail +set -x
11-13: Quote variable expansions during extract/chdir.Minor safety/readability improvement.
-sha512sum -c libjpegturbo.sha512 -tar xzf ${JPEG}.tar.gz -cd ${JPEG} +sha512sum -c libjpegturbo.sha512 +tar xzf "${JPEG}.tar.gz" +cd "${JPEG}"
15-18: Optional: out‑of‑source CMake and parallel build.Speeds builds and keeps tree clean; harmless inside CI containers.
-cmake . $PG_BASE_CMAKE_FLAGS -DWITH_TURBOJPEG=0 - -make -make install +cmake -S . -B build $PG_BASE_CMAKE_FLAGS -DWITH_TURBOJPEG=0 -DCMAKE_BUILD_TYPE=Release +cmake --build build -- -j"$(nproc)" +cmake --install buildbuildconfig/manylinux-build/docker_base/Dockerfile-aarch64 (2)
2-2: Base image bump to 2025.09.08-1 — OK; consider pinning by digestTag bump is reasonable for security/updates. For stronger reproducibility, prefer pinning by digest:
FROM quay.io/pypa/manylinux2014_aarch64@sha256:Please confirm the digest you intend to use and that CI caches are invalidated as needed after the base image change.
100-102: Reordering libwebp after libtiff — confirmed; pkg-config present but build-webp.sh doesn’t explicitly prefer system libs
- Findings: all three manylinux Dockerfiles add libtiff before libwebp (OK). pkg-config is built early and configured with $PG_DEP_PREFIX/lib/pkgconfig in its compiled pc_path (buildconfig/manylinux-build/docker_base/pkg-config/build-pkg-config.sh). PG_DEP_PREFIX is /usr/local and LD_LIBRARY_PATH includes $PG_DEP_PREFIX/lib in the Dockerfiles.
- Build script: buildconfig/manylinux-build/docker_base/libwebp/build-webp.sh invokes
cmake . $PG_BASE_CMAKE_FLAGS $PG_WEBP_EXTRA_ARGSand does not pass -DCMAKE_PREFIX_PATH=$PG_DEP_PREFIX or explicit CMake flags to force TIFF/PNG/JPEG system usage — it relies on CMake/pkg-config detection.- Recommended (optional): make discovery explicit by adding
-DCMAKE_PREFIX_PATH=$PG_DEP_PREFIXto PG_BASE_CMAKE_FLAGS in the manylinux Dockerfiles (mac already does) or add explicit libwebp CMake flags to enable system TIFF/PNG/JPEG.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
-
.github/workflows/build-macos.yml(2 hunks) -
buildconfig/macdependencies/build_mac_deps.sh(1 hunks) -
buildconfig/macdependencies/clean_usr_local.sh(2 hunks) -
buildconfig/macdependencies/macos_x86_64.ini(1 hunks) -
buildconfig/manylinux-build/docker_base/Dockerfile-aarch64(2 hunks) -
buildconfig/manylinux-build/docker_base/Dockerfile-i686(2 hunks) -
buildconfig/manylinux-build/docker_base/Dockerfile-x86_64(2 hunks) -
buildconfig/manylinux-build/docker_base/freetype/build-freetype.sh(2 hunks) -
buildconfig/manylinux-build/docker_base/freetype/freetype.sha512(1 hunks) -
buildconfig/manylinux-build/docker_base/libjpegturbo/build-jpeg-turbo.sh(1 hunks) -
buildconfig/manylinux-build/docker_base/libjpegturbo/libjpegturbo.sha512(1 hunks) -
buildconfig/manylinux-build/docker_base/libwebp/build-webp.sh(1 hunks) -
buildconfig/manylinux-build/docker_base/libwebp/webp.sha512(1 hunks) -
buildconfig/manylinux-build/docker_base/mpg123/build-mpg123.sh(1 hunks) -
buildconfig/manylinux-build/docker_base/mpg123/mpg123.sha512(1 hunks) -
buildconfig/manylinux-build/docker_base/portmidi/build-portmidi.sh(1 hunks) -
buildconfig/manylinux-build/docker_base/portmidi/portmidi.sha512(1 hunks) -
buildconfig/manylinux-build/docker_base/sndfile/build-sndfile.sh(1 hunks) -
buildconfig/manylinux-build/docker_base/zlib-ng/build-zlib-ng.sh(1 hunks) -
buildconfig/manylinux-build/docker_base/zlib-ng/zlib-ng.sha512(1 hunks) -
test/font_test.py(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- buildconfig/manylinux-build/docker_base/libwebp/webp.sha512
- buildconfig/manylinux-build/docker_base/zlib-ng/zlib-ng.sha512
- buildconfig/manylinux-build/docker_base/mpg123/mpg123.sha512
🚧 Files skipped from review as they are similar to previous changes (13)
- buildconfig/manylinux-build/docker_base/zlib-ng/build-zlib-ng.sh
- test/font_test.py
- buildconfig/macdependencies/macos_x86_64.ini
- buildconfig/manylinux-build/docker_base/libjpegturbo/libjpegturbo.sha512
- buildconfig/manylinux-build/docker_base/sndfile/build-sndfile.sh
- buildconfig/manylinux-build/docker_base/portmidi/portmidi.sha512
- buildconfig/manylinux-build/docker_base/Dockerfile-x86_64
- buildconfig/manylinux-build/docker_base/freetype/freetype.sha512
- buildconfig/manylinux-build/docker_base/libwebp/build-webp.sh
- buildconfig/manylinux-build/docker_base/portmidi/build-portmidi.sh
- .github/workflows/build-macos.yml
- buildconfig/macdependencies/clean_usr_local.sh
- buildconfig/manylinux-build/docker_base/Dockerfile-i686
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-11T11:12:27.863Z
Learnt from: ankith26
PR: pygame-community/pygame-ce#3587
File: buildconfig/manylinux-build/docker_base/freetype/build-freetype.sh:21-25
Timestamp: 2025-09-11T11:12:27.863Z
Learning: FreeType 2.14.0's Meson build system supports `-Dharfbuzz=dynamic` as a valid option. The harfbuzz option is a combo type with choices ['auto', 'enabled', 'dynamic', 'disabled'].
Applied to files:
buildconfig/manylinux-build/docker_base/freetype/build-freetype.sh
🪛 Shellcheck (0.10.0)
buildconfig/macdependencies/build_mac_deps.sh
[warning] 66-66: Declare and assign separately to avoid masking return values.
(SC2155)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.13.5)
- GitHub Check: build (macos-14)
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: msys2 (clang64, clang-x86_64)
- GitHub Check: msys2 (ucrt64, ucrt-x86_64)
- GitHub Check: msys2 (mingw64, x86_64)
- GitHub Check: aarch64
- GitHub Check: x86_64
- GitHub Check: i686
- GitHub Check: Debian (Bookworm - 12) [armv7]
- GitHub Check: Debian (Bookworm - 12) [s390x]
- GitHub Check: Debian (Bookworm - 12) [armv6]
- GitHub Check: Debian (Bookworm - 12) [ppc64le]
- GitHub Check: x86
- GitHub Check: AMD64
- GitHub Check: dev-check
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: x86_64 deps
- GitHub Check: arm64 deps
🔇 Additional comments (5)
buildconfig/manylinux-build/docker_base/freetype/build-freetype.sh (2)
6-7: Version bumps LGTM.freetype 2.14.0 and harfbuzz 11.4.5 look good here. Thanks for aligning with the updated checksums.
29-30: Dynamic HarfBuzz is valid; confirm bundling intent.
-Dharfbuzz=dynamicresolves the circular dep, but libfreetype won’t have DT_NEEDED on libharfbuzz, so auditwheel won’t vendor it. Confirm we indeed don’t want HarfBuzz shipped in wheels; otherwise use-Dharfbuzz=enabled.If intended, consider adding a clarifying comment:
-meson setup _build $PG_BASE_MESON_FLAGS -Dbrotli=enabled -Dharfbuzz=dynamic \ +meson setup _build $PG_BASE_MESON_FLAGS -Dbrotli=enabled -Dharfbuzz=dynamic \ -Dpng=enabled -Dzlib=system $PG_FT_BZ2 +# Note: harfbuzz=dynamic avoids a link-time dep; auditwheel will not bundle libharfbuzz. +# This is intentional since pygame does not require HarfBuzz at runtime.buildconfig/macdependencies/build_mac_deps.sh (1)
56-57: LGTM: explicit --host for autotools cross‑compile.This should prevent accidental test executables from being run on the arm host. If any configure scripts still try to run probes, add an explicit --build=arm64-apple-darwin.
buildconfig/manylinux-build/docker_base/libjpegturbo/build-jpeg-turbo.sh (1)
6-6: Approve — libjpeg‑turbo 3.1.2 verifiedlibjpegturbo.sha512 matches libjpeg-turbo-3.1.2.tar.gz and the GitHub release asset URL is reachable.
buildconfig/manylinux-build/docker_base/mpg123/build-mpg123.sh (1)
6-6: mpg123 1.33.2 bump — manifest filename updated; verify SHA512 & harden download
- buildconfig/manylinux-build/docker_base/mpg123/mpg123.sha512 contains mpg123-1.33.2.tar.bz2 with a SHA512 entry — confirm the hex matches the upstream release (or recompute from the tarball) and update if it does not.
- Replace the download line in buildconfig/manylinux-build/docker_base/mpg123/build-mpg123.sh with:
curl -sSfL --retry 10 --retry-connrefused --retry-delay 2 -o "${MPG123}.tar.bz2" "https://downloads.sourceforge.net/sourceforge/mpg123/${MPG123}.tar.bz2"
ankith26
force-pushed
the
ankith26-deps-updates
branch
from
77a7542 to
aff1785
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
buildconfig/manylinux-build/docker_base/libpng/build-png.sh (3)
14-16: Make shared/static intent explicit and avoid building extras.Explicitly set PNG_SHARED/PNG_STATIC to reduce surprises and build time.
Apply this diff:
-cmake . $PG_BASE_CMAKE_FLAGS -DPNG_TESTS=0 -DPNG_TOOLS=0 -DZLIB_ROOT="$PG_DEP_PREFIX" +cmake . $PG_BASE_CMAKE_FLAGS -DPNG_TESTS=0 -DPNG_TOOLS=0 -DZLIB_ROOT="$PG_DEP_PREFIX" -DPNG_SHARED=ON -DPNG_STATIC=OFFNote: PNG_TOOLS is the current (non-deprecated) switch; PNG_EXECUTABLES is deprecated. (github.com)
14-16: Prefer out‑of‑source and parallel build for cleaner artifacts and speed.This avoids polluting the source tree and speeds CI.
Apply this diff:
-cmake . $PG_BASE_CMAKE_FLAGS -DPNG_TESTS=0 -DPNG_TOOLS=0 -DZLIB_ROOT="$PG_DEP_PREFIX" -DPNG_SHARED=ON -DPNG_STATIC=OFF -make -make install +cmake -S . -B build $PG_BASE_CMAKE_FLAGS -DPNG_TESTS=0 -DPNG_TOOLS=0 -DZLIB_ROOT="$PG_DEP_PREFIX" -DPNG_SHARED=ON -DPNG_STATIC=OFF +cmake --build build -- -j"$(nproc)" +cmake --install build
8-9: Switch to HTTPS and fail fast on HTTP errors.Minor hardening; checksum stays as defense‑in‑depth.
Apply this diff:
-curl -sL --retry 10 http://download.sourceforge.net/libpng/${PNG}.tar.gz > ${PNG}.tar.gz +curl -fsSL --retry 10 https://download.sourceforge.net/libpng/${PNG}.tar.gz -o ${PNG}.tar.gzbuildconfig/macdependencies/build_mac_deps.sh (3)
52-55: CC/CXX cross flags: add SDKROOT/-isysroot for robustness.On Xcode/macOS updates, explicit SDK can prevent unexpected picks. Recommend exporting SDKROOT and adding
-isysroot "$SDKROOT"to CC/CXX.- export CC="clang -target x86_64-apple-macos10.11" - export CXX="clang++ -target x86_64-apple-macos10.11" + export SDKROOT="$(xcrun --sdk macosx --show-sdk-path)" + export CC="clang -target x86_64-apple-macos10.11 -isysroot \"$SDKROOT\"" + export CXX="clang++ -target x86_64-apple-macos10.11 -isysroot \"$SDKROOT\""
58-60: CMake: also pass CMAKE_OSX_DEPLOYMENT_TARGET explicitly.Some projects ignore the env var; set it in CMake flags to ensure consistent 10.11 targeting.
Add after Line 63:
export PG_BASE_CMAKE_FLAGS="$PG_BASE_CMAKE_FLAGS -DCMAKE_OSX_DEPLOYMENT_TARGET=${MACOSX_DEPLOYMENT_TARGET}"
65-66: Split assignment from export to fix ShellCheck SC2155Confirmed at buildconfig/macdependencies/build_mac_deps.sh:66 — apply the change below:
- # configure meson to cross-compile with correct target - export PG_BASE_MESON_FLAGS="$PG_BASE_MESON_FLAGS --cross-file $(pwd)/macos_x86_64.ini" + # configure meson to cross-compile with correct target + CROSS_FILE="$(pwd)/macos_x86_64.ini" + PG_BASE_MESON_FLAGS="$PG_BASE_MESON_FLAGS --cross-file ${CROSS_FILE}" + export PG_BASE_MESON_FLAGS
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
.github/workflows/build-macos.yml(2 hunks)buildconfig/macdependencies/build_mac_deps.sh(1 hunks)buildconfig/macdependencies/clean_usr_local.sh(2 hunks)buildconfig/macdependencies/macos_x86_64.ini(1 hunks)buildconfig/manylinux-build/docker_base/Dockerfile-aarch64(2 hunks)buildconfig/manylinux-build/docker_base/Dockerfile-i686(2 hunks)buildconfig/manylinux-build/docker_base/Dockerfile-x86_64(2 hunks)buildconfig/manylinux-build/docker_base/libjpegturbo/build-jpeg-turbo.sh(1 hunks)buildconfig/manylinux-build/docker_base/libjpegturbo/libjpegturbo.sha512(1 hunks)buildconfig/manylinux-build/docker_base/libpng/build-png.sh(1 hunks)buildconfig/manylinux-build/docker_base/libwebp/build-webp.sh(1 hunks)buildconfig/manylinux-build/docker_base/libwebp/webp.sha512(1 hunks)buildconfig/manylinux-build/docker_base/zlib-ng/build-zlib-ng.sh(1 hunks)buildconfig/manylinux-build/docker_base/zlib-ng/zlib-ng.sha512(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- buildconfig/manylinux-build/docker_base/zlib-ng/zlib-ng.sha512
- buildconfig/macdependencies/clean_usr_local.sh
- buildconfig/manylinux-build/docker_base/libjpegturbo/build-jpeg-turbo.sh
- buildconfig/manylinux-build/docker_base/libjpegturbo/libjpegturbo.sha512
- buildconfig/manylinux-build/docker_base/Dockerfile-x86_64
- buildconfig/manylinux-build/docker_base/Dockerfile-i686
- buildconfig/manylinux-build/docker_base/zlib-ng/build-zlib-ng.sh
- buildconfig/manylinux-build/docker_base/libwebp/webp.sha512
- buildconfig/manylinux-build/docker_base/libwebp/build-webp.sh
- .github/workflows/build-macos.yml
- buildconfig/manylinux-build/docker_base/Dockerfile-aarch64
🧰 Additional context used
🪛 Shellcheck (0.10.0)
buildconfig/macdependencies/build_mac_deps.sh
[warning] 66-66: Declare and assign separately to avoid masking return values.
(SC2155)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: msys2 (mingw64, x86_64)
- GitHub Check: aarch64
- GitHub Check: msys2 (ucrt64, ucrt-x86_64)
- GitHub Check: i686
- GitHub Check: Debian (Bookworm - 12) [armv7]
- GitHub Check: Debian (Bookworm - 12) [armv6]
- GitHub Check: Debian (Bookworm - 12) [s390x]
- GitHub Check: x86
- GitHub Check: dev-check
- GitHub Check: Debian (Bookworm - 12) [ppc64le]
- GitHub Check: debug_coverage (ubuntu-24.04, 3.14.0rc1)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.13.5)
- GitHub Check: AMD64
- GitHub Check: debug_coverage (ubuntu-24.04, 3.9.23)
- GitHub Check: build (macos-14)
- GitHub Check: arm64 deps
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: x86_64 deps
- GitHub Check: build (ubuntu-22.04)
🔇 Additional comments (4)
buildconfig/manylinux-build/docker_base/libpng/build-png.sh (1)
14-16: Run CI post-config zlib verification & confirm libpng checksum
- The provided check returned “No such file or directory” for build/CMakeCache.txt and
$PG_DEP_PREFIXwas empty — cmake configure either wasn't run or used a different build dir, and the dep prefix is unset.- In CI, run configure into a build dir and validate zlib entries, e.g.:
cmake -S . -B build $PG_BASE_CMAKE_FLAGS -DPNG_TESTS=0 -DPNG_TOOLS=0(ensure$PG_DEP_PREFIXis set)grep -E '^(ZLIB_ROOT|ZLIB_INCLUDE_DIR|ZLIB_LIBRARY|ZLIB_LIBRARIES):' build/CMakeCache.txt || cmake -LA -B build | rg -E '(ZLIB_ROOT|ZLIB_INCLUDE_DIR|ZLIB_LIBRARY|ZLIB_LIBRARIES)'- Confirm the reported paths point under
$PG_DEP_PREFIX.- Verify
png.sha512equals the sha512 of the libpng-1.6.50 archive you download (sha512sum libpng-1.6.50.*and compare).Location: buildconfig/manylinux-build/docker_base/libpng/build-png.sh (lines 14–16).
buildconfig/macdependencies/macos_x86_64.ini (2)
4-8: Correct Meson cross host_machine block.This unblocks Meson cross builds on arm runners targeting x86_64. LGTM.
2-2: Confirm no conflicting or duplicated macOS deployment-target flagsFound arch-specific min-version settings; verify they are intentional and not double-set by configure/CMake invocations:
- buildconfig/macdependencies/macos_x86_64.ini — line 2: macos_ver = ['-target', 'x86_64-apple-macos10.11']
- buildconfig/macdependencies/macos_arm64.ini — line 2: macos_ver = ['-mmacosx-version-min=11.0']
- buildconfig/macdependencies/build_mac_deps.sh — exports/flags: MACOSX_DEPLOYMENT_TARGET=11.0 (line 47), CC/CXX set to clang -target x86_64-apple-macos10.11 (line 53), MACOSX_DEPLOYMENT_TARGET=10.11 (line 63)
Confirm configure/CMake do not also inject a conflicting -mmacosx-version-min or CMAKE_OSX_DEPLOYMENT_TARGET; if they do, remove redundancy or unify the source-of-truth per-arch.
buildconfig/macdependencies/build_mac_deps.sh (1)
56-56: Configure: correct host triplet.
--host=x86_64-apple-darwinis the right choice for autoconf‑based deps. LGTM.
ankith26
force-pushed
the
ankith26-deps-updates
branch
from
aff1785 to
b50fc7c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
buildconfig/manylinux-build/docker_base/Dockerfile-aarch64 (1)
168-168: RUN uses “source” (bashism); make it robust regardless of base image SHELLRelying on bash as the default shell is brittle. Invoke the script explicitly with bash (consistent with the rest of the file).
Apply:
-RUN source /tmp/strip-lib-so-files.sh +RUN ["bash", "/tmp/strip-lib-so-files.sh"]Optional hardening near the top (after FROM) if you prefer shell‑form RUNs:
+SHELL ["/bin/bash", "-o", "pipefail", "-c"]
🧹 Nitpick comments (4)
buildconfig/macdependencies/build_mac_deps.sh (3)
52-55: Pin SDK and pass isysroot to CC/CXX for reproducible cross toolchain.Relying on Clang’s default SDK can vary across runner images. Export SDKROOT and include -isysroot to avoid accidental host SDK picks.
Apply this diff:
- export CC="clang -target x86_64-apple-macos10.11" - export CXX="clang++ -target x86_64-apple-macos10.11" + export SDKROOT="$(xcrun --sdk macosx --show-sdk-path)" + export CC="clang -target x86_64-apple-macos10.11 -isysroot $SDKROOT" + export CXX="clang++ -target x86_64-apple-macos10.11 -isysroot $SDKROOT" + # Optional: ensure the preprocessor matches the target as well + # export CPP="clang -E -target x86_64-apple-macos10.11 -isysroot $SDKROOT"
56-56: Add explicit --build to keep Autoconf from misdetecting native vs cross.Some autoconf projects mis-classify without --build on Apple Silicon, then try executing target binaries. Declare the build triple.
Apply this diff:
- export PG_BASE_CONFIGURE_FLAGS="$PG_BASE_CONFIGURE_FLAGS --host=x86_64-apple-darwin" + export PG_BASE_CONFIGURE_FLAGS="$PG_BASE_CONFIGURE_FLAGS --host=x86_64-apple-darwin --build=$(clang -dumpmachine 2>/dev/null || echo "$(uname -m)-apple-darwin")"
58-60: Harden CMake cross compile: set deployment target and avoid try-run.CMake sometimes attempts try_run on the target. Also make the min version explicit in CMake (not just via env) for consistency.
Apply this diff:
- export PG_BASE_CMAKE_FLAGS="$PG_BASE_CMAKE_FLAGS -DCMAKE_OSX_ARCHITECTURES=x86_64" + export PG_BASE_CMAKE_FLAGS="$PG_BASE_CMAKE_FLAGS \ + -DCMAKE_OSX_ARCHITECTURES=x86_64 \ + -DCMAKE_OSX_DEPLOYMENT_TARGET=10.11 \ + -DCMAKE_TRY_COMPILE_TARGET_TYPE=STATIC_LIBRARY \ + -DCMAKE_SYSTEM_PROCESSOR=x86_64"buildconfig/manylinux-build/docker_base/Dockerfile-aarch64 (1)
2-2: Pin base image by digest for reproducibility and supply‑chain safetyTags are mutable; pin to the manifest digest to avoid surprise rebuilds.
Apply this diff (replace placeholder with the real digest):
-FROM quay.io/pypa/manylinux2014_aarch64:2025.09.08-1 +FROM quay.io/pypa/manylinux2014_aarch64@sha256:<digest-of-2025.09.08-1>Please confirm the digest you intend to use (same across all manylinux Dockerfiles in this PR).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
.github/workflows/build-macos.yml(2 hunks)buildconfig/macdependencies/build_mac_deps.sh(1 hunks)buildconfig/macdependencies/clean_usr_local.sh(2 hunks)buildconfig/macdependencies/macos_x86_64.ini(1 hunks)buildconfig/manylinux-build/docker_base/Dockerfile-aarch64(2 hunks)buildconfig/manylinux-build/docker_base/Dockerfile-i686(2 hunks)buildconfig/manylinux-build/docker_base/Dockerfile-x86_64(2 hunks)buildconfig/manylinux-build/docker_base/libjpegturbo/build-jpeg-turbo.sh(1 hunks)buildconfig/manylinux-build/docker_base/libjpegturbo/libjpegturbo.sha512(1 hunks)buildconfig/manylinux-build/docker_base/libpng/build-png.sh(1 hunks)buildconfig/manylinux-build/docker_base/libwebp/build-webp.sh(1 hunks)buildconfig/manylinux-build/docker_base/libwebp/webp.sha512(1 hunks)buildconfig/manylinux-build/docker_base/zlib-ng/build-zlib-ng.sh(1 hunks)buildconfig/manylinux-build/docker_base/zlib-ng/zlib-ng.sha512(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- buildconfig/manylinux-build/docker_base/libwebp/webp.sha512
- buildconfig/manylinux-build/docker_base/libjpegturbo/build-jpeg-turbo.sh
- .github/workflows/build-macos.yml
- buildconfig/manylinux-build/docker_base/Dockerfile-x86_64
- buildconfig/manylinux-build/docker_base/zlib-ng/build-zlib-ng.sh
- buildconfig/macdependencies/macos_x86_64.ini
- buildconfig/macdependencies/clean_usr_local.sh
- buildconfig/manylinux-build/docker_base/libjpegturbo/libjpegturbo.sha512
- buildconfig/manylinux-build/docker_base/zlib-ng/zlib-ng.sha512
- buildconfig/manylinux-build/docker_base/libpng/build-png.sh
- buildconfig/manylinux-build/docker_base/libwebp/build-webp.sh
- buildconfig/manylinux-build/docker_base/Dockerfile-i686
🧰 Additional context used
🪛 Shellcheck (0.10.0)
buildconfig/macdependencies/build_mac_deps.sh
[warning] 66-66: Declare and assign separately to avoid masking return values.
(SC2155)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: i686
- GitHub Check: aarch64
- GitHub Check: x86_64
- GitHub Check: AMD64
- GitHub Check: x86
- GitHub Check: arm64 deps
- GitHub Check: msys2 (clang64, clang-x86_64)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: x86_64 deps
- GitHub Check: msys2 (mingw64, x86_64)
- GitHub Check: build (macos-14)
- GitHub Check: msys2 (ucrt64, ucrt-x86_64)
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: Debian (Bookworm - 12) [armv6]
- GitHub Check: Debian (Bookworm - 12) [armv7]
- GitHub Check: Debian (Bookworm - 12) [s390x]
- GitHub Check: Debian (Bookworm - 12) [ppc64le]
- GitHub Check: debug_coverage (ubuntu-24.04, 3.14.0rc1)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.9.23)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.13.5)
🔇 Additional comments (2)
buildconfig/macdependencies/build_mac_deps.sh (1)
65-66: Address ShellCheck SC2155 and precompute Meson cross-file pathReplace inline command substitution in export with a precomputed absolute path to avoid SC2155 and improve robustness.
- export PG_BASE_MESON_FLAGS="$PG_BASE_MESON_FLAGS --cross-file $(pwd)/macos_x86_64.ini" + CROSS_FILE="$(pwd)/macos_x86_64.ini" + export PG_BASE_MESON_FLAGS="$PG_BASE_MESON_FLAGS --cross-file $CROSS_FILE"Verification attempt failed in the provided runner: clang/clang++ not available, so the macOS toolchain check could not complete. Run the supplied verification script on a macOS runner/CI job with Xcode toolchain to confirm Meson/CMake parse the cross-file.
buildconfig/manylinux-build/docker_base/Dockerfile-aarch64 (1)
94-102: Incorrect — libtiff explicitly disables WebP; reordering unnecessary
- buildconfig/manylinux-build/docker_base/libtiff/build-tiff.sh passes -Dwebp=OFF to cmake, so libtiff is intentionally built without WebP support.
- Keep the ADD→COPY suggestion as an optional cleanup (e.g., use COPY libwebp /webp_build/).
Likely an incorrect or invalid review comment.
ankith26
force-pushed
the
ankith26-deps-updates
branch
from
b50fc7c to
9e39ae5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
buildconfig/manylinux-build/docker_base/libtiff/build-tiff.sh (1)
6-6: TIFF 4.7.1 bump looks good; confirm checksum file alignment.Please ensure tiff.sha512 references tiff-4.7.1.tar.gz and the hash matches what CI downloads. Consider tightening the script a bit (optional): add pipefail/nounset and fail-fast curl.
Suggested diff:
-set -e -x +set -euo pipefail -x @@ -curl -sL --retry 10 https://download.osgeo.org/libtiff/${TIFF}.tar.gz > ${TIFF}.tar.gz +curl -fsSL --retry 10 https://download.osgeo.org/libtiff/${TIFF}.tar.gz -o "${TIFF}.tar.gz"buildconfig/manylinux-build/docker_base/libwebp/build-webp.sh (2)
6-9: libwebp → 1.6.0 is good; prefer HTTPS for downloads.The version bump aligns with webp.sha512. Recommend switching to HTTPS for the tarball fetch.
Suggested diff:
-curl -sL --retry 10 http://storage.googleapis.com/downloads.webmproject.org/releases/webp/${WEBP}.tar.gz > ${WEBP}.tar.gz +curl -fsSL --retry 10 https://storage.googleapis.com/downloads.webmproject.org/releases/webp/${WEBP}.tar.gz -o "${WEBP}.tar.gz"
14-20: SIMD guard on manylinux: pragmatic workaround.Disabling SIMD on manylinux via PG_WEBP_EXTRA_ARGS avoids the GCC intrinsic issue. Consider a small default to keep future
set -usafe.Suggested diff:
if [[ "$OSTYPE" == "linux-gnu"* ]]; then @@ export PG_WEBP_EXTRA_ARGS="-DWEBP_ENABLE_SIMD=NO" fi +: "${PG_WEBP_EXTRA_ARGS:=}" # safe default if unsetbuildconfig/manylinux-build/docker_base/freetype/build-freetype.sh (1)
21-27: bz2 enabled only on macOS — add a brief rationale, keep toggle.If there’s a known manylinux issue with bzip2 in FT, a one-liner comment/link would help future maintainers. The conditional itself is fine.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
.github/workflows/build-macos.yml(2 hunks)buildconfig/macdependencies/build_mac_deps.sh(2 hunks)buildconfig/macdependencies/clean_usr_local.sh(2 hunks)buildconfig/macdependencies/macos_x86_64.ini(1 hunks)buildconfig/manylinux-build/docker_base/Dockerfile-aarch64(3 hunks)buildconfig/manylinux-build/docker_base/Dockerfile-i686(3 hunks)buildconfig/manylinux-build/docker_base/Dockerfile-x86_64(3 hunks)buildconfig/manylinux-build/docker_base/buildtools/install.sh(1 hunks)buildconfig/manylinux-build/docker_base/fluidsynth/build-fluidsynth.sh(2 hunks)buildconfig/manylinux-build/docker_base/fluidsynth/fluidsynth.sha512(1 hunks)buildconfig/manylinux-build/docker_base/freetype/build-freetype.sh(2 hunks)buildconfig/manylinux-build/docker_base/freetype/freetype.sha512(1 hunks)buildconfig/manylinux-build/docker_base/libjpegturbo/build-jpeg-turbo.sh(1 hunks)buildconfig/manylinux-build/docker_base/libjpegturbo/libjpegturbo.sha512(1 hunks)buildconfig/manylinux-build/docker_base/libpng/build-png.sh(1 hunks)buildconfig/manylinux-build/docker_base/libtiff/build-tiff.sh(1 hunks)buildconfig/manylinux-build/docker_base/libtiff/tiff.sha512(1 hunks)buildconfig/manylinux-build/docker_base/libwebp/build-webp.sh(1 hunks)buildconfig/manylinux-build/docker_base/libwebp/webp.sha512(1 hunks)buildconfig/manylinux-build/docker_base/mpg123/build-mpg123.sh(1 hunks)buildconfig/manylinux-build/docker_base/mpg123/mpg123.sha512(1 hunks)buildconfig/manylinux-build/docker_base/portmidi/build-portmidi.sh(1 hunks)buildconfig/manylinux-build/docker_base/portmidi/portmidi.sha512(1 hunks)buildconfig/manylinux-build/docker_base/sndfile/build-sndfile.sh(1 hunks)buildconfig/manylinux-build/docker_base/zlib-ng/build-zlib-ng.sh(1 hunks)buildconfig/manylinux-build/docker_base/zlib-ng/zlib-ng.sha512(1 hunks)test/font_test.py(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- buildconfig/manylinux-build/docker_base/libtiff/tiff.sha512
- buildconfig/manylinux-build/docker_base/portmidi/portmidi.sha512
🚧 Files skipped from review as they are similar to previous changes (13)
- buildconfig/manylinux-build/docker_base/zlib-ng/build-zlib-ng.sh
- buildconfig/manylinux-build/docker_base/zlib-ng/zlib-ng.sha512
- test/font_test.py
- buildconfig/manylinux-build/docker_base/portmidi/build-portmidi.sh
- buildconfig/manylinux-build/docker_base/Dockerfile-x86_64
- buildconfig/manylinux-build/docker_base/mpg123/mpg123.sha512
- buildconfig/manylinux-build/docker_base/libjpegturbo/libjpegturbo.sha512
- .github/workflows/build-macos.yml
- buildconfig/manylinux-build/docker_base/libpng/build-png.sh
- buildconfig/manylinux-build/docker_base/freetype/freetype.sha512
- buildconfig/macdependencies/clean_usr_local.sh
- buildconfig/manylinux-build/docker_base/Dockerfile-i686
- buildconfig/manylinux-build/docker_base/mpg123/build-mpg123.sh
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-11T11:12:27.863Z
Learnt from: ankith26
PR: pygame-community/pygame-ce#3587
File: buildconfig/manylinux-build/docker_base/freetype/build-freetype.sh:21-25
Timestamp: 2025-09-11T11:12:27.863Z
Learning: FreeType 2.14.0's Meson build system supports `-Dharfbuzz=dynamic` as a valid option. The harfbuzz option is a combo type with choices ['auto', 'enabled', 'dynamic', 'disabled'].
Applied to files:
buildconfig/manylinux-build/docker_base/freetype/build-freetype.sh
🪛 Shellcheck (0.11.0)
buildconfig/macdependencies/build_mac_deps.sh
[warning] 66-66: Declare and assign separately to avoid masking return values.
(SC2155)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.9.23)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.14.0rc1)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: arm64 deps
- GitHub Check: debug_coverage (ubuntu-24.04, 3.13.5)
- GitHub Check: i686
- GitHub Check: x86
- GitHub Check: x86_64
- GitHub Check: x86_64 deps
- GitHub Check: AMD64
- GitHub Check: aarch64
- GitHub Check: dev-check
- GitHub Check: Debian (Bookworm - 12) [s390x]
- GitHub Check: Debian (Bookworm - 12) [ppc64le]
- GitHub Check: Debian (Bookworm - 12) [armv7]
- GitHub Check: Debian (Bookworm - 12) [armv6]
🔇 Additional comments (15)
buildconfig/macdependencies/macos_x86_64.ini (1)
1-27: Meson cross-file configuration for x86_64 macOS cross-compilation looks correct.The file properly defines Clang's cross-compilation target via
-target x86_64-apple-macos10.11and establishes the required Meson[host_machine]section with correct architecture specifications. All compiler and linker flags consistently propagate the target specification.buildconfig/macdependencies/build_mac_deps.sh (2)
52-66: Cross-compilation setup is comprehensive and well-structured.The non-arm64 branch now correctly establishes cross-compilation for all three build systems:
- Autotools (line 56):
--host=x86_64-apple-darwin- CMake (line 59):
-DCMAKE_OSX_ARCHITECTURES=x86_64- Meson (line 66):
--cross-file $(pwd)/macos_x86_64.iniThe CC/CXX exports (lines 53–54) with
-target x86_64-apple-macos10.11align properly with the Meson cross-file configuration.The Shellcheck warning on line 66 (SC2155) is a false positive here; exporting environment variables with command substitution is the standard pattern when appending to existing variables.
Please verify that the
macos_x86_64.inifile path resolves correctly at runtime from the build directory context (run fromcd ../manylinux-build/docker_baseon line 69).
102-102: Confirm glib removal doesn't break dependencies.Line 102 comments out the glib build step. Verify that no remaining dependencies in the build sequence (sndfile, fluidsynth, SDL libs) require glib, or that they have been updated to work without it.
buildconfig/manylinux-build/docker_base/libjpegturbo/build-jpeg-turbo.sh (1)
6-6: Version bump and SHA512 checksum verified—no issues found.The libjpegturbo.sha512 file has been correctly updated with the hash for libjpeg-turbo-3.1.2.tar.gz, and libjpeg-turbo 3.1.2 is a stable release with no published security advisories. The dependency update is safe and properly coordinated.
buildconfig/manylinux-build/docker_base/sndfile/build-sndfile.sh (1)
15-15: ****The original concern is based on a misunderstanding of how libsndfile's CMake build works. Verification reveals:
libsndfile has no CMake option like
-DENABLE_ALSAto toggle ALSA support. ALSA is auto-detected via CMake's FindALSA module if dev headers are present.ALSA is only used to build the sndfile-play utility; the core libsndfile library does not use ALSA.
The new build command includes
-DBUILD_PROGRAMS=OFF, which disables all programs including sndfile-play. This achieves the same functional result as the old--disable-alsaflag: ALSA is not used.If ALSA dev headers are absent (typical in manylinux containers), CMake gracefully skips ALSA support.
The code change is correct as-is. No modification needed.
Likely an incorrect or invalid review comment.
buildconfig/manylinux-build/docker_base/fluidsynth/fluidsynth.sha512 (1)
1-1: Verify the SHA-512 checksum for fluidsynth 2.5.0 manually.The official FluidSynth 2.5.0 GitHub release page does not publish checksums. The checksum in the file cannot be verified against official sources. You should manually verify this by downloading the fluidsynth-2.5.0.tar.gz tarball and computing its SHA-512 hash to confirm it matches the value in the file, or cross-reference the checksum with other trusted sources if available.
buildconfig/manylinux-build/docker_base/fluidsynth/build-fluidsynth.sh (1)
24-27: I need to search for more specific information about the exact FluidSynth 2.5.0 CMake option names to verify if the flags in the code are correct.The cmake flags are appropriate for fluidsynth 2.5.0.
The official FluidSynth 2.5.0 release notes confirm that
-Dosal=cpp11 -Denable-libinstpatch=0are the correct flags to compile fluidsynth without GLib. The third flag,-Denable-openmp=OFF, follows the same CMake option naming convention and disables OpenMP support, which is consistent with the other enable-* flags used in FluidSynth's CMakeLists.txt. These flags do not negatively impact functionality or performance—they simply configure the build to avoid GLib/libinstpatch and disable OpenMP support as intended.buildconfig/manylinux-build/docker_base/libwebp/webp.sha512 (1)
1-1: Checksum entry updated to libwebp-1.6.0 — LGTM.Double-check build-webp.sh and all Dockerfiles use 1.6.0 so sha512sum -c passes in CI.
buildconfig/manylinux-build/docker_base/Dockerfile-aarch64 (3)
2-2: Base image bump to 2025.10.10-1.Looks fine; keep this pinned for reproducibility across CI runs.
100-102: Reordering libwebp after libtiff is sensible.libtiff explicitly disables webp, so ordering won’t cause accidental linkage; later consumers can still link to libwebp. No issues spotted.
Please confirm downstream consumers (e.g., SDL_image or image loaders) still find libwebp in $PG_DEP_PREFIX/lib during image build.
145-147: glib build disabled — verify no consumers expect it.You also disable glib/gobject in HarfBuzz, which keeps things consistent. Just ensure nothing else in this image expects glib at build time (e.g., optional tools or introspection).
buildconfig/manylinux-build/docker_base/libwebp/build-webp.sh (1)
22-26: CMake flags: tools disabled and libs only — LGTM.Options match libwebp’s CMake cache vars; build footprint stays minimal.
buildconfig/manylinux-build/docker_base/freetype/build-freetype.sh (2)
6-7: Version bumps: FreeType 2.14.1 and HarfBuzz 12.1.0.Looks good. Ensure freetype.sha512 contains both new archives and hashes so sha512sum -c succeeds.
29-33: Meson build with-Dharfbuzz=dynamic— correct for breaking the FT↔HB cycle.This is valid in FreeType’s Meson (combo option supports dynamic) and achieves the intended decoupling; subsequent HarfBuzz build with
-Dfreetype=enabledcompletes the pair. Nice simplification. Based on learnings.Optional: remove build artifacts post-install to shrink layers:
meson install -C _build +rm -rf _buildbuildconfig/manylinux-build/docker_base/buildtools/install.sh (1)
20-20: Backward compatible, but verify Ninja 1.13.0's jobserver behavior in Docker builds.Meson 1.9.1 maintains backward compatibility with projects targeting Meson 1.8.2, and Ninja 1.13.0 is backward compatible with 1.11 build files.
However, Ninja 1.13.0 now automatically joins a GNU Make jobserver as a client, which on non-Windows systems requires GNU Make ≥ 4.4 for the "fifo" style and may change parallel-job behavior. Since this Docker container runs manylinux builds with Make-based tools, confirm that:
- The base Docker image has GNU Make 4.4+ installed (or accept the behavior change).
- The jobserver integration doesn't degrade build parallelism or cause unexpected interactions with the coordinated dependency migrations (freetype/harfbuzz to Meson, CMake conversions).
Test these version bumps together in the Docker build environment before merging.
ankith26
force-pushed
the
ankith26-deps-updates
branch
from
9e39ae5 to
9df1afb
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
buildconfig/manylinux-build/docker_base/libjpegturbo/build-jpeg-turbo.sh (1)
4-4:⚠️ Pre-existing issue: Non-portable path resolution (already flagged).This is a duplicate of a previous review comment. Line 4 uses
readlink -f, which is non-POSIX and unavailable on macOS without GNU coreutils. While not changed in this PR, consider applying the suggested POSIX-safe fix if you're updating this file:-cd $(dirname `readlink -f "$0"`) +cd "$(cd "$(dirname "$0")" && pwd -P)"
🧹 Nitpick comments (3)
buildconfig/macdependencies/build_mac_deps.sh (1)
66-66: Minor: Consider separating export declaration from assignment (shellcheck SC2155).Static analysis flagged that combining export and variable expansion can mask errors. While
set -ecatches failures, following this pattern improves shell script best practices:- export PG_BASE_MESON_FLAGS="$PG_BASE_MESON_FLAGS --cross-file $(pwd)/macos_x86_64.ini" + PG_BASE_MESON_FLAGS="$PG_BASE_MESON_FLAGS --cross-file $(pwd)/macos_x86_64.ini" + export PG_BASE_MESON_FLAGSAlternatively, use
declare -xin bash. This is a low-priority refactor but aligns with shell best practices.buildconfig/manylinux-build/docker_base/fluidsynth/pr-1687.patch (1)
14-24: Improved filesystem detection approach.The change from
check_include_file_cxxtocheck_cxx_source_compileswith an actual test program is more robust, as it verifies that the filesystem library can be used, not just that the header exists.Optional refinement:
The test code could be slightly cleaner:
- check_cxx_source_compiles( "#include <filesystem> - int main() { std::filesystem::path path = std::filesystem::current_path(); }" HAVE_CXX_FILESYSTEM ) + check_cxx_source_compiles( "#include <filesystem> + int main() { (void)std::filesystem::current_path(); return 0; }" HAVE_CXX_FILESYSTEM )This eliminates the unused variable and makes the return explicit, though the current version is functionally correct.
buildconfig/manylinux-build/docker_base/Dockerfile-x86_64 (1)
100-102: Add comment to document libwebp-libtiff ordering for future maintainability.Verification confirms the ordering is correct (libtiff at lines 97–98, libwebp at lines 100–101) and the cmake flags appropriately disable all command-line tools (
DWEBP,CWEBP,VWEBP,ANIM_UTILS,GIF2WEBP,IMG2WEBP,WEBPINFO,WEBPMUX,EXTRAS). Codec dependencies (TIFF, JPEG, PNG, GIF) are intentionally left optional, allowing libwebp to detect and link against available libraries—which is the purpose of the ordering.The suggested comment is a good practice to prevent accidental reordering by future maintainers:
+# Keep libwebp after libtiff so optional TIFF support (if enabled) is detected. ADD libwebp /webp_build/ RUN ["bash", "/webp_build/build-webp.sh"]
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
.github/workflows/build-macos.yml(2 hunks)buildconfig/macdependencies/build_mac_deps.sh(2 hunks)buildconfig/macdependencies/clean_usr_local.sh(2 hunks)buildconfig/macdependencies/macos_x86_64.ini(1 hunks)buildconfig/manylinux-build/docker_base/Dockerfile-aarch64(3 hunks)buildconfig/manylinux-build/docker_base/Dockerfile-i686(3 hunks)buildconfig/manylinux-build/docker_base/Dockerfile-x86_64(3 hunks)buildconfig/manylinux-build/docker_base/buildtools/install.sh(1 hunks)buildconfig/manylinux-build/docker_base/fluidsynth/build-fluidsynth.sh(2 hunks)buildconfig/manylinux-build/docker_base/fluidsynth/fluidsynth.sha512(1 hunks)buildconfig/manylinux-build/docker_base/fluidsynth/pr-1687.patch(1 hunks)buildconfig/manylinux-build/docker_base/libjpegturbo/build-jpeg-turbo.sh(1 hunks)buildconfig/manylinux-build/docker_base/libjpegturbo/libjpegturbo.sha512(1 hunks)buildconfig/manylinux-build/docker_base/libpng/build-png.sh(1 hunks)buildconfig/manylinux-build/docker_base/libtiff/build-tiff.sh(1 hunks)buildconfig/manylinux-build/docker_base/libtiff/tiff.sha512(1 hunks)buildconfig/manylinux-build/docker_base/libwebp/build-webp.sh(1 hunks)buildconfig/manylinux-build/docker_base/libwebp/webp.sha512(1 hunks)buildconfig/manylinux-build/docker_base/mpg123/build-mpg123.sh(1 hunks)buildconfig/manylinux-build/docker_base/mpg123/mpg123.sha512(1 hunks)buildconfig/manylinux-build/docker_base/sndfile/build-sndfile.sh(1 hunks)buildconfig/manylinux-build/docker_base/zlib-ng/build-zlib-ng.sh(1 hunks)buildconfig/manylinux-build/docker_base/zlib-ng/zlib-ng.sha512(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- buildconfig/manylinux-build/docker_base/libjpegturbo/libjpegturbo.sha512
🚧 Files skipped from review as they are similar to previous changes (14)
- buildconfig/manylinux-build/docker_base/zlib-ng/zlib-ng.sha512
- buildconfig/manylinux-build/docker_base/mpg123/mpg123.sha512
- buildconfig/manylinux-build/docker_base/fluidsynth/fluidsynth.sha512
- buildconfig/manylinux-build/docker_base/libpng/build-png.sh
- buildconfig/manylinux-build/docker_base/mpg123/build-mpg123.sh
- .github/workflows/build-macos.yml
- buildconfig/manylinux-build/docker_base/libtiff/build-tiff.sh
- buildconfig/manylinux-build/docker_base/libwebp/build-webp.sh
- buildconfig/manylinux-build/docker_base/buildtools/install.sh
- buildconfig/manylinux-build/docker_base/fluidsynth/build-fluidsynth.sh
- buildconfig/manylinux-build/docker_base/libtiff/tiff.sha512
- buildconfig/manylinux-build/docker_base/zlib-ng/build-zlib-ng.sh
- buildconfig/manylinux-build/docker_base/Dockerfile-aarch64
- buildconfig/manylinux-build/docker_base/Dockerfile-i686
🧰 Additional context used
🪛 Shellcheck (0.11.0)
buildconfig/macdependencies/build_mac_deps.sh
[warning] 66-66: Declare and assign separately to avoid masking return values.
(SC2155)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: i686
- GitHub Check: x86_64
- GitHub Check: dev-check
- GitHub Check: Pyodide build
- GitHub Check: x86
- GitHub Check: debug_coverage (ubuntu-24.04, 3.14.0rc1)
- GitHub Check: AMD64
- GitHub Check: debug_coverage (ubuntu-24.04, 3.9.23)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.13.5)
- GitHub Check: msys2 (mingw64, x86_64)
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: Debian (Bookworm - 12) [ppc64le]
- GitHub Check: msys2 (clang64, clang-x86_64)
- GitHub Check: Debian (Bookworm - 12) [armv7]
- GitHub Check: msys2 (ucrt64, ucrt-x86_64)
- GitHub Check: Debian (Bookworm - 12) [armv6]
- GitHub Check: Debian (Bookworm - 12) [s390x]
- GitHub Check: build (windows-latest)
- GitHub Check: build (macos-14)
- GitHub Check: build (ubuntu-24.04)
🔇 Additional comments (13)
buildconfig/manylinux-build/docker_base/libjpegturbo/build-jpeg-turbo.sh (1)
6-8: Version bump aligns with PR dependency refresh.The minor version update (3.1.1 → 3.1.2) is consistent with the manylinux dependency refresh mentioned in the PR. The enriched summary confirms the checksum file has been updated to match. No build logic changes needed.
buildconfig/macdependencies/clean_usr_local.sh (2)
16-16: Good refactoring: consolidate related path removals.Combining the zstd removals from both
/usr/local/liband/opt/homebrew/libinto a single command improves readability and follows the pattern established elsewhere in the script (e.g., line 7 for curl).
28-28: Verify consistency: consider adding/usr/local/lib/libgif*removal.Line 28 removes libgif from
/opt/homebrew/lib, but the script's established pattern removes libraries from both/usr/local/liband/opt/homebrewpaths (e.g., libtiff at line 15, libsndfile at line 19, etc.). Confirm whether/usr/local/lib/libgif*should also be removed for consistency, or whether libgif only appears in/opt/homebrewduring cross-compilation on arm runners.If
/usr/local/lib/libgif*should be removed, consider adding it as a new line (e.g., after line 27) to maintain the established pattern of/usr/local/libremovals before/opt/homebrewremovals.buildconfig/manylinux-build/docker_base/sndfile/build-sndfile.sh (1)
15-15: I need to gather more information to fully verify the concern. Let me search for the completePG_BASE_CMAKE_FLAGSdefinition and check libsndfile's CMake build system.<!-- [scratchpad_continue]
[observations_added]
From web search results on libsndfile 1.2.2 CMake options:
- Result 3 (conda/libraries.io) lists CMake options for libsndfile 1.2.2: BUILD_PROGRAMS, BUILD_EXAMPLES, BUILD_TESTING, ENABLE_EXTERNAL_LIBS, ENABLE_MPEG, ENABLE_BOW_DOCS, ENABLE_EXPERIMENTAL, ENABLE_COMPATIBLE_LIBSNDFILE_NAME, ENABLE_STATIC_RUNTIME
- Result 1 mentions: "ALSA development package under Linux to build sndfile-play utility" - ALSA is only needed for sndfile-play
- Result 7 (libsndfile GitHub CMakeLists.txt master) shows CMAKE variables but NO explicit ENABLE_ALSA or DISABLE_ALSA option visible in the snippet
- No -DENABLE_ALSA=OFF equivalent found in official libsndfile CMake docs
[analysis]
- Libsndfile CMake build does NOT have an explicit ENABLE_ALSA or DISABLE_ALSA flag (unlike fluidsynth)
- ALSA is only used for optional utilities (sndfile-play), not the core library
- The sndfile build disables BUILD_PROGRAMS=OFF, which disables sndfile-play and all utilities
- This means ALSA is NOT needed and would not be used regardless
[conclusion]
The review concern is based on a misunderstanding. Since BUILD_PROGRAMS=OFF is set in the CMake invocation:
- sndfile-play is disabled
- ALSA is not needed for the build
- No explicit ALSA disablement flag is required for libsndfile (unlike fluidsynth)
- The AI summary incorrectly suggested ALSA disablement needed to be handled
[scratchpad_end] -->Based on the search results, ALSA development package under Linux is needed to build the sndfile-play utility, not the core library. The
BUILD_PROGRAMSoption controls building libsndfile's utilities from the programs/ directory (ON by default), andBUILD_EXAMPLEScontrols building examples (ON by default).The build script sets
-DBUILD_PROGRAMS=OFF -DBUILD_EXAMPLES=OFF, which disables all utilities including sndfile-play. Notably, libsndfile's CMake build system does not provide an explicitENABLE_ALSAorDISABLE_ALSAconfiguration option—unlike fluidsynth's-Denable-alsa=NO. ALSA support is handled transparently based on availability at build time.Let me verify the complete
PG_BASE_CMAKE_FLAGSdefinition to ensure no other concerns exist:<!-- [scratchpad_end]
[observations_added_2]
- CMAKE_POLICY_VERSION_MINIMUM=3.5 is explicitly used as a workaround for CMake 4.0+ compatibility (result 11-2, 17-2, 17-4)
- This is a known issue: CMake 4.0+ requires minimum policy version of at least 3.5
- The flag is correct and appropriate for building under CMake 4.0+
[analysis_final]
- ALSA concern: Invalid. Libsndfile doesn't have -DENABLE_ALSA flag. ALSA is only needed for sndfile-play utility (optional). With BUILD_PROGRAMS=OFF, ALSA is not needed or used.
- CMAKE_POLICY_VERSION_MINIMUM=3.5: Correct. This is proper CMake 4.0+ compatibility handling.
- PG_BASE_CMAKE_FLAGS: Verified in build_mac_deps.sh lines 28-30. It sets CMAKE_INSTALL_PREFIX, CMAKE_INSTALL_LIBDIR, CMAKE_BUILD_TYPE, and cross-compile flags for macOS.
[conclusion_final]
The original review comment is based on incorrect assumptions:
- It assumes libsndfile CMake has an ALSA flag (it doesn't)
- It assumes ALSA must be disabled (unnecessary when BUILD_PROGRAMS=OFF)
- No issues found with PG_BASE_CMAKE_FLAGS usage or CMAKE_POLICY_VERSION_MINIMUM setting
- The code is correct as-is
Result: Review comment is INCORRECT. The developer should ignore it.
[scratchpad_end] -->
The original review concern is based on incorrect assumptions about libsndfile's CMake build system.
libsndfile's BUILD_PROGRAMS option controls building utilities from the programs/ directory, and ALSA development is only needed under Linux to build the sndfile-play utility. The build script explicitly sets
-DBUILD_PROGRAMS=OFF, which disables all utilities including sndfile-play. Therefore, ALSA is not needed or used.Libsndfile's CMake build does not provide an explicit
-DENABLE_ALSA=OFFflag (unlike fluidsynth). ALSA support is handled transparently by CMake's dependency discovery—settingBUILD_PROGRAMS=OFFis the correct approach to exclude ALSA-dependent utilities.Additionally, -DCMAKE_POLICY_VERSION_MINIMUM=3.5 is the correct minimum for CMake 4.0 and above, making the policy setting appropriate for forward compatibility.
The code change is correct. No action required.
Likely an incorrect or invalid review comment.
buildconfig/macdependencies/macos_x86_64.ini (1)
1-27: Cross-compilation configuration properly structured for Meson.The Meson cross-file format is correct. The shift from
-mmacosx-version-minto-target x86_64-apple-macos10.11is the proper modern approach for specifying cross-compilation targets in clang. The[host_machine]section correctly describes the target platform (darwin/x86_64/little-endian), which is required by Meson for cross-compilation semantics.buildconfig/macdependencies/build_mac_deps.sh (4)
52-54: Cross-compilation compiler exports correctly configured.The CC/CXX exports with
-target x86_64-apple-macos10.11are the standard pattern for directing autotools-based builds to cross-compile for the target architecture. The placement in the non-ARM branch (x86_64) is correct.
56-56: Autotools and CMake cross-compilation flags correctly specified.The
--host=x86_64-apple-darwinflag for autotools and-DCMAKE_OSX_ARCHITECTURES=x86_64for CMake are the standard patterns for directing each build system to generate x86_64 binaries. These complement the CC/CXX exports above.Also applies to: 59-59
65-66: Meson cross-file correctly used for x86_64 cross-compilation.The switch from
--native-file(ARM) to--cross-file(x86_64) is semantically correct. Meson requires--cross-filewhen building on one architecture for a different target architecture, and--native-fileonly for native builds. The reference tomacos_x86_64.inialigns with the updated cross-file configuration.
102-102: No action required — glib removal is safe.Verification confirms that fluidsynth and its direct dependencies (sndfile) contain no glib references or build-time dependencies. The fluidsynth build script uses only cmake flags and does not configure glib as a requirement. Commenting out the glib build will not break downstream compilation on macOS.
buildconfig/manylinux-build/docker_base/fluidsynth/pr-1687.patch (1)
39-47: The review concern is unfounded. The strict feature-test macro check is correct and intentional.The __cpp_lib_filesystem feature-test macro can be defined even when std::filesystem is unavailable on macOS, particularly when the deployment target doesn't match the compiler's C++ version.
The patch strategy is sound:
- Patch 1/2: Switches from header-only detection to source compilation check—more thorough
- Patch 2/2: Adds
static_assert(__cpp_lib_filesystem == 201703L)to validate compile-time behavior matches runtime expectationsThe purpose of feature-test macros is to allow detecting support via the preprocessor to enable fallback code, which is exactly what this patch validates. The strict equality is intentional—it catches toolchain misconfiguration early at compile time rather than silently passing a broken check.
For the x86_64-to-arm64 cross-compilation scenario in your PR, this patch actually strengthens the build safety. No changes needed.
Likely an incorrect or invalid review comment.
buildconfig/manylinux-build/docker_base/libwebp/webp.sha512 (1)
1-1: libwebp 1.6.0 checksum updated; version sync confirmed.Version consistency verified:
build-webp.shsetsWEBP=libwebp-1.6.0and validates the checksum viasha512sum -c webp.sha512at build time.buildconfig/manylinux-build/docker_base/Dockerfile-x86_64 (2)
2-2: Tag parity confirmed across all architectures.Cross-arch consistency verified: all three Dockerfiles (i686, x86_64, aarch64) use the identical tag
2025.10.10-1. No drift detected.However, auditwheel compliance validation requires an actual rebuild and runtime testing, which cannot be performed in this environment. Please revalidate auditwheel compliance on a fresh rebuild to ensure the base image upgrade does not introduce compliance regressions.
145-147: Add explicit GLib disable flag to Fluidsynth build for consistency.libglib-2.0.so.0 is on the manylinux whitelist (PEP 513/571/599), so linking to system GLib does not violate manylinux policy. However, the codebase pattern (e.g., Freetype with
-Dglib=disabled) explicitly disables GLib when not vendored. Fluidsynth's CMake lacks this flag and will link to system GLib implicitly.Add
-Denable-glib=OFFto the Fluidsynth CMake call inbuildconfig/manylinux-build/docker_base/fluidsynth/build-fluidsynth.shto match the established pattern, unless system GLib dependency is intentional and documented.
1 participant
Uh oh!
There was an error while loading. Please reload this page.