backport: bitcoin/bitcoin#34036: contrib: update macOS SDK to Xcode-26.1.1-17B100#7189
backport: bitcoin/bitcoin#34036: contrib: update macOS SDK to Xcode-26.1.1-17B100#7189knst wants to merge 8 commits intodashpay:developfrom
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
|
Guix Automation has began to build this PR tagged as v23.1.0-devpr7189.112ae2dc. A new comment will be made when the image is pushed. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR updates macOS/Xcode and SDK-related build and CI configuration: bumps Xcode version and build ID from 16.0 (16A242d) to 26.1.1 (17B100); changes default SDK download URL from the project S3 bucket to https://bitcoincore.org/depends-sources/sdks and switches some archives from .tar.gz to .tar with updated sha256 sums; gen-sdk.py now produces a plain PAX tar and filters swiftmodule/modulemap files; several host makefiles and per-host CFLAGS/CXXFLAGS were adjusted or cleared; README examples and scripts updated accordingly. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contrib/containers/guix/scripts/setup-sdk`:
- Around line 10-16: The XCODE_AR_HASH variable currently holds the SHA256 of
the original Xcode XIP, but setup uses the pre-built .tar.gz from the SDK_URL
and verifies that file against XCODE_AR_HASH; replace the current XCODE_AR_HASH
value with the actual sha256 checksum of the .tar.gz hosted at the SDK_URL
(compute the sha256sum of the .tar.gz served at depends-sources/sdks) so the
verification in setup-sdk succeeds—update the XCODE_AR_HASH constant with that
.tar.gz checksum.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
ci/test/00_setup_env.shci/test/00_setup_env_mac.shcontrib/containers/guix/scripts/setup-sdkcontrib/guix/README.mdcontrib/guix/symbol-check.pycontrib/macdeploy/README.mddepends/hosts/darwin.mk
There was a problem hiding this comment.
🧹 Nitpick comments (1)
depends/hosts/darwin.mk (1)
16-17: Note: Changed from immediate (:=) to deferred (=) assignment.The
clang_progandclangxx_progvariables now use deferred assignment (=) instead of immediate assignment (:=). This means the shell command will be evaluated each time the variable is referenced rather than once at parse time. This is typically fine for tool lookups but could have minor performance implications if referenced repeatedly. Verify this matches the upstream change.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@depends/hosts/darwin.mk` around lines 16 - 17, The change switched clang_prog and clangxx_prog from immediate assignment (:=) to deferred assignment (=), causing the shell lookup for clang/clang++ to run each time the variables are expanded; if this was accidental or causes repeated performance hits revert them back to immediate assignment (use := for clang_prog and clangxx_prog) so the command is executed once at parse time, otherwise add a comment near the clang_prog and clangxx_prog definitions documenting that the deferred assignment is intentional and why (e.g., dynamic PATH changes during the make run).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@depends/hosts/darwin.mk`:
- Around line 16-17: The change switched clang_prog and clangxx_prog from
immediate assignment (:=) to deferred assignment (=), causing the shell lookup
for clang/clang++ to run each time the variables are expanded; if this was
accidental or causes repeated performance hits revert them back to immediate
assignment (use := for clang_prog and clangxx_prog) so the command is executed
once at parse time, otherwise add a comment near the clang_prog and clangxx_prog
definitions documenting that the deferred assignment is intentional and why
(e.g., dynamic PATH changes during the make run).
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
depends/funcs.mkdepends/hosts/darwin.mkdepends/hosts/freebsd.mkdepends/hosts/linux.mkdepends/hosts/mingw32.mkdepends/hosts/netbsd.mkdepends/hosts/openbsd.mk
✅ Files skipped from review due to trivial changes (1)
- depends/hosts/openbsd.mk
|
Guix Automation has failed with an unknown error for tag v23.1.0-devpr7189.112ae2dc |
|
Guix Automation has began to build this PR tagged as v23.1.0-devpr7189.ecdb9fa5. A new comment will be made when the image is pushed. |
|
Guix Automation has failed with an unknown error for tag v23.1.0-devpr7189.ecdb9fa5 |
a89e161 contrib: update macOS SDK to Xcode-26.1.1-17B100 (fanquake) 57a778e depends: use -Xclang -fno-cxx-modules in macOS cross build (fanquake) Pull request description: Updates the macOS SDK used for Guix builds to `Xcode-26.1.1-17B100`. Closes bitcoin#34034. ACKs for top commit: hebasto: ACK a89e161. sedited: ACK a89e161 janb84: concept ACK a89e161 Tree-SHA512: 4f8f9afee6fca594a0d30fbb3c150f5ed120b40f707954678ff69951bc806acc154aed4b5986d8642160f7b37523933c87c5734f296ff881555093188e29549e
…x determinism (across distros) 3e01b5d contrib: rename gen-sdk to gen-sdk.py (fanquake) c1213a3 macdeploy: disable compression in macOS gen-sdk script (fanquake) a33d034 contrib: more selectively pick files for macOS SDK (fanquake) Pull request description: This includes three changes. The first is to more selectively pick files for inclusion into our macOS SDK tarball (skip manpages, binaries etc), which is nice because it redues the size of the tarball (from ~80mb to 20mb), and makes the size increase that happens with the next commit, less-bad. The second change removes compression of the tarball. Starting with Python 3.11, Pythons gzip might delegate to zlib. Depending on the OS, i.e Ubuntu vs Fedora, the underlying zlib implementation might differ, resulting in different output. For now, or until a better solution exists, remove compression. This results in the SDK increasing in size to ~157mb. Which is not unreasonable, to regain determinism (and would be significantly worse without the previous commit). See: https://docs.python.org/3/library/gzip.html#gzip.compress The third renames `gen-sdk` to `gen-sdk.py`, so that it will be linted, along with the rest of our Python files. Fixes bitcoin#31873. We could probably also put this into 30.x. ACKs for top commit: stickies-v: ACK 3e01b5d modulo the new .tar SDK being uploaded davidgumberg: Tested ACK 3e01b5d Tree-SHA512: 272164a98e0e6f10822870162c1b3a405693c2f64d3ed085a2d2243a48641d940704b5ef6022256915ac9cf383e87a4f8d4dc2ec4eaa9d25e2abd30f5498778b
…han setting them per-host 9954d6c depends: hard-code necessary c(xx)flags rather than setting them per-host (Cory Fields) Pull request description: The per-host flag variables hold platform-specific defaults that are ignored when flag environment variables are set, so it was wrong for them to contain -std options relied on by package definitions. Additionally, these flags (-pipe and -std=xx) will no longer be passed into the CMake build, meaning less duplication in the build summary. Pulled out of bitcoin#31920. ACKs for top commit: achow101: ACK 9954d6c ryanofsky: Code review ACK 9954d6c. No changes since last review other than improving the commit message. Change overall makes sense because it deduplicates host definitions, stops dropping `-std` flags from package builds when custom CFLAGS/CXXFLAGS environment variables are set, and stops passing duplicate flags to cmake that have no effect. theuni: ACK 9954d6c Tree-SHA512: 62a2a21b741893cf708ca71fb5f0626c30d52685c845f9016f428a5e38fc8515acd4bf2c83635d6505b63830d1c296472026ec3d341c8069f1e490a991b6b5ef
|
Guix Automation has began to build this PR tagged as v23.1.0-devpr7189.2c779e34. A new comment will be made when the image is pushed. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@depends/hosts/darwin.mk`:
- Around line 60-67: Update the two TODO comment lines that currently read
"partiall reverted" to the correct spelling "partially reverted" (these are the
TODO comments surrounding the darwin_CC definition and the linked PR
references). Edit the comment text in the same lines so both instances of
"partiall" become "partially" while leaving the rest of the comment and the
darwin_CC variable untouched.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
ci/test/00_setup_env_mac.shcontrib/containers/guix/scripts/setup-sdkcontrib/guix/README.mdcontrib/macdeploy/README.mdcontrib/macdeploy/gen-sdk.pydepends/funcs.mkdepends/hosts/darwin.mkdepends/hosts/freebsd.mkdepends/hosts/linux.mkdepends/hosts/mingw32.mkdepends/hosts/netbsd.mkdepends/hosts/openbsd.mk
🚧 Files skipped from review as they are similar to previous changes (9)
- depends/hosts/netbsd.mk
- contrib/guix/README.md
- depends/hosts/linux.mk
- contrib/containers/guix/scripts/setup-sdk
- depends/hosts/openbsd.mk
- depends/hosts/freebsd.mk
- contrib/macdeploy/README.md
- depends/hosts/mingw32.mk
- depends/funcs.mk
|
Guix Automation has failed with an unknown error for tag v23.1.0-devpr7189.2c779e34 |
|
I don't know why CI fails, but there are my hashes: |
|
🔨 Guix build started for Targets: x86_64-linux-gnu aarch64-linux-gnu riscv64-linux-gnu x86_64-w64-mingw32 x86_64-apple-darwin arm64-apple-darwin A follow-up comment with checksums will be posted when the build completes. |
|
❌ Guix build failed for tag |
|
🔨 Guix build started for Targets: x86_64-linux-gnu aarch64-linux-gnu riscv64-linux-gnu x86_64-w64-mingw32 x86_64-apple-darwin arm64-apple-darwin A follow-up comment with checksums will be posted when the build completes. |
|
❌ Guix build failed for tag |
|
🔨 Guix build started for Targets: x86_64-linux-gnu aarch64-linux-gnu riscv64-linux-gnu x86_64-w64-mingw32 x86_64-apple-darwin arm64-apple-darwin A follow-up comment with checksums will be posted when the build completes. |
|
❌ Guix build failed for tag |
|
🔨 Guix build started for Targets: x86_64-linux-gnu aarch64-linux-gnu riscv64-linux-gnu x86_64-w64-mingw32 x86_64-apple-darwin arm64-apple-darwin A follow-up comment with checksums will be posted when the build completes. |
|
❌ Guix build failed for tag |
|
🔨 Guix build started for Targets: x86_64-linux-gnu aarch64-linux-gnu riscv64-linux-gnu x86_64-w64-mingw32 x86_64-apple-darwin arm64-apple-darwin A follow-up comment with checksums will be posted when the build completes. |
|
❌ Guix build failed for tag |
|
🔨 Guix build started for Targets: x86_64-linux-gnu aarch64-linux-gnu riscv64-linux-gnu x86_64-w64-mingw32 x86_64-apple-darwin arm64-apple-darwin A follow-up comment with checksums will be posted when the build completes. |
|
❌ Guix build failed for tag |
7 participants
Issue being fixed or feature implemented
#7184 introduces a new product to maintain and take care, but without docs, without owner, without a processes to update it to new version.
Particularly, this file:
export SDK_URL=${SDK_URL:-https://s3.us-west-2.amazonaws.com/dash-depends-sources}What was done?
Partially reverted #7184 and backported 34036 instead.
How Has This Been Tested?
See CI run
Breaking Changes
Xcode 26.1 is used instead Xcode 16.
Checklist: