Skip to content

guix: avoid downloading macOS SDK for non-darwin target HOSTs #6890

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
PastaPastaPasta merged 1 commit into from
Oct 19, 2025
Merged

guix: avoid downloading macOS SDK for non-darwin target HOSTs #6890

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions contrib/containers/guix/scripts/guix-start
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,12 @@ if [[ ! -d "${WORKSPACE_PATH}" || ! "${WORKSPACE_PATH}" = /* || ! -f "${WORKSPAC
exit 1
fi

export SDK_PATH="${SDK_PATH:-${WORKSPACE_PATH}/depends/SDKs}"
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}"
Copy link

@coderabbitai coderabbitai bot Oct 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

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.

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

Copy link
Member

@PastaPastaPasta PastaPastaPasta Oct 14, 2025

Choose a reason for hiding this comment

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

This is the old behavior; I know it seems odd, are you sure this is a needed change / bug?

Copy link

@coderabbitai coderabbitai bot Oct 14, 2025

Choose a reason for hiding this comment

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

🧩 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

  1. Apple build settings: SDKROOT (Base SDK) — Xcode Build Setting Reference. [Apple]. [1]
  2. Using macOS SDKs and locating them (xcrun, SDK paths) — Firefox source docs / general SDK primer. [2]
  3. 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:

  1. Leave it as-is if this behavior has been working correctly
  2. Address it in a separate PR if you determine it should use ${SDK_SRCS:-...} instead

What would you prefer?

Copy link
Author

@UdjinM6 UdjinM6 Oct 14, 2025

Choose a reason for hiding this comment

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

So there are two places like that in our scripts (introduced via #6490 04ce1fe):

I guess we should fix/unify them. Will do in a separate PR.

Copy link

@coderabbitai coderabbitai bot Oct 14, 2025

Choose a reason for hiding this comment

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

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!


./contrib/containers/guix/scripts/setup-sdk
./contrib/containers/guix/scripts/setup-sdk
fi

# Add safe.directory option only when WORKSPACE_PATH was specified via cmd-line arguments (happens in CI)
if [[ -n "${1}" ]]; then
Expand Down
Loading