guix: avoid downloading macOS SDK for non-darwin target HOSTs#6890
guix: avoid downloading macOS SDK for non-darwin target HOSTs#6890PastaPastaPasta merged 1 commit intodashpay:developfrom
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
WalkthroughThe file contrib/containers/guix/scripts/guix-start now exports Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📥 CommitsReviewing files that changed from the base of the PR and between a0aec735a483c37716cb9fcfe1ddef0fc2e727cc and d0eb2b5. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 2350339 and a0aec735a483c37716cb9fcfe1ddef0fc2e727cc.
📒 Files selected for processing (1)
contrib/containers/guix/scripts/guix-start(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
contrib/**
📄 CodeRabbit inference engine (CLAUDE.md)
Unless specifically prompted, avoid making changes to the contrib directory (contributed scripts)
Files:
contrib/containers/guix/scripts/guix-start
🔇 Additional comments (1)
contrib/containers/guix/scripts/guix-start (1)
12-12: Verify the conditional logic handles all scenarios correctly.The conditional at line 12 checks if
HOSTSis unset or contains "darwin". SinceHOSTSreceives a default value at lines 29-31 (which includes darwin hosts), the logic should work correctly:
- If
HOSTSis unset at line 12 → SDK setup runs →HOSTSgets default (includes darwin) at line 29- If
HOSTSis set to darwin-only → SDK setup runs- If
HOSTSis set to non-darwin only → SDK setup is skippedHowever, please verify this behavior with actual test runs for non-darwin hosts (e.g.,
HOSTS=x86_64-w64-mingw32) to ensure the SDK download is properly skipped.Also applies to: 16-17
| export SDK_SRCS="${SDK_PATH:-${WORKSPACE_PATH}/depends/sdk-sources}" | ||
| if [[ -z "${HOSTS}" || "${HOSTS}" == *"darwin"* ]]; then | ||
| export SDK_PATH="${SDK_PATH:-${WORKSPACE_PATH}/depends/SDKs}" | ||
| export SDK_SRCS="${SDK_PATH:-${WORKSPACE_PATH}/depends/sdk-sources}" |
There was a problem hiding this comment.
Fix parameter expansion variable reference.
Line 14 uses ${SDK_PATH:-...} instead of ${SDK_SRCS:-...}, causing SDK_SRCS to incorrectly default to SDK_PATH's value rather than the intended depends/sdk-sources path.
Apply this diff:
- export SDK_SRCS="${SDK_PATH:-${WORKSPACE_PATH}/depends/sdk-sources}"
+ export SDK_SRCS="${SDK_SRCS:-${WORKSPACE_PATH}/depends/sdk-sources}"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export SDK_SRCS="${SDK_PATH:-${WORKSPACE_PATH}/depends/sdk-sources}" | |
| export SDK_SRCS="${SDK_SRCS:-${WORKSPACE_PATH}/depends/sdk-sources}" |
🤖 Prompt for AI Agents
In contrib/containers/guix/scripts/guix-start around line 14, the export for
SDK_SRCS uses the wrong parameter name (${SDK_PATH:-...}) so it defaults to
SDK_PATH instead of providing the intended default path; change the expansion to
use SDK_SRCS as the parameter (e.g.,
${SDK_SRCS:-${WORKSPACE_PATH}/depends/sdk-sources}) so SDK_SRCS preserves an
existing value or falls back to ${WORKSPACE_PATH}/depends/sdk-sources.
There was a problem hiding this comment.
This is the old behavior; I know it seems odd, are you sure this is a needed change / bug?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the git history of this file to see when this pattern was introduced
echo "=== Git history of the SDK_SRCS line ==="
git log -p --follow -S 'SDK_SRCS=' -- contrib/containers/guix/scripts/guix-start | head -100
echo -e "\n=== Looking at setup-sdk script to understand the relationship ==="
cat contrib/containers/guix/scripts/setup-sdk
echo -e "\n=== Searching for usage of SDK_SRCS in the codebase ==="
rg -n 'SDK_SRCS' --type=sh -C2
echo -e "\n=== Searching for usage of SDK_PATH in the codebase ==="
rg -n 'SDK_PATH' --type=sh -C2
Unrecoverable error: Failed to clone repository
🌐 Web query:
macOS SDK_PATH SDK_SRCS relationship in build systems depends directory
💡 Result:
Short answer
- SDK_PATH (aka SDKROOT or the path returned by xcrun --show-sdk-path) is the on‑disk root of an Apple SDK: headers, frameworks, libs, metadata used at compile/link time. [1][2]
- SDK_SRCS is not an Xcode standard build setting; it’s a project/build‑system variable some projects use to list or point to SDK source/header files that must be be compiled or copied into the build (for example when a tree ships its own trimmed SDK or private headers). Its meaning is project‑specific. [2][3]
- The depends directory (common in large projects like Firefox) is where a build system stages or caches third‑party/prepared artifacts (including a harvested SDK, headers, private frameworks or generated sources). A build will set SDK_PATH/SDKROOT to point at either the system SDK or the staged SDK inside depends, and it may populate SDK_SRCS from that staged copy so compilation uses the predictable, hermetic files in depends. [2][3]
Practical implications
- Use xcrun --show-sdk-path or Xcode’s SDKROOT to locate the canonical SDK on the machine. Point your build’s SDK_PATH to that (or to depends//sdk) so include/search paths and linker paths resolve correctly. [1]
- Only use an SDK_SRCS-like variable if your build needs to compile or vendor SDK headers/sources; document its use in your build scripts because it’s not a standard Xcode/CMake/clang setting. [2][3]
- For hermetic CI/local reproducibility, stage a known SDK into your depends/cache and make SDK_PATH point at it; ensure header search paths prefer the depends copy to avoid accidental system drift. [2][3]
References
- Apple build settings: SDKROOT (Base SDK) — Xcode Build Setting Reference. [Apple]. [1]
- Using macOS SDKs and locating them (xcrun, SDK paths) — Firefox source docs / general SDK primer. [2]
- Example of a build system using a depends/staged SDK (mozilla moz.configure / depends usage). [3]
@PastaPastaPasta, you're absolutely right—I apologize for the confusion. Looking at the diff again, this pattern existed before your PR. Your changes only wrap these lines in the conditional block without modifying the parameter expansion logic itself.
That said, the pattern does still appear unusual: if SDK_PATH is set (e.g., by a user or CI), then SDK_SRCS would inherit SDK_PATH's value rather than defaulting to depends/sdk-sources. This may have been intentional to keep both variables pointing to the same location when SDK_PATH is customized, or it could be a long-standing typo that went unnoticed since it typically works when neither variable is pre-set.
Given that this is pre-existing behavior and out of scope for this PR (which focuses on skipping SDK downloads for non-darwin hosts), you can:
- Leave it as-is if this behavior has been working correctly
- Address it in a separate PR if you determine it should use
${SDK_SRCS:-...}instead
What would you prefer?
There was a problem hiding this comment.
So there are two places like that in our scripts (introduced via #6490 04ce1fe):
export SDK_SRCS="${SDK_PATH:-${WORKSPACE_PATH}/depends/sdk-sources}"https://github.com/dashpay/dash/blob/develop/contrib/containers/guix/scripts/guix-start#L13SDK_SRCS="${SDK_SOURCES:-depends/sdk-sources}"https://github.com/dashpay/dash/blob/develop/contrib/containers/guix/scripts/setup-sdk#L12
I guess we should fix/unify them. Will do in a separate PR.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
UdjinM6
changed the title
UdjinM6
force-pushed
the
no_sdk_nondarwin
branch
from
a0aec73 to
d0eb2b5
Compare
…roperly 7d3e909 guix: adjust scripts to use `SDK_SOURCES` env variable properly (UdjinM6) Pull request description: ## Issue being fixed or feature implemented #6890 (review) ## What was done? ## How Has This Been Tested? ## Breaking Changes ## Checklist: - [ ] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: kwvg: utACK 7d3e909 kwvg: utACK 7d3e909 Tree-SHA512: 3b69c34ad85ae946c58789a9fad37804db66b82e602434de8df15b3f47e6663d2ad4a65e763ae67a066a6b4d796899b588582939906f400fac543c917abd0f66
3 participants
Issue being fixed or feature implemented
Save some time/bandwidth for non-darwin HOSTs.
See windows build as an example:
develop: https://github.com/UdjinM6/dash/actions/runs/18493165708/job/52692931975#step:7:78
this PR: https://github.com/UdjinM6/dash/actions/runs/18494088428/job/52720349586#step:7:76
What was done?
How Has This Been Tested?
https://github.com/UdjinM6/dash/actions/runs/18494088428
Breaking Changes
Checklist: