AB#292772 build: resolve test toolchain and simulator instead of hard-pinning - #145
Open
eligutovsky wants to merge 3 commits into
Open
AB#292772 build: resolve test toolchain and simulator instead of hard-pinning#145eligutovsky wants to merge 3 commits into
eligutovsky wants to merge 3 commits into
Conversation
make test could not run on any machine without Xcode 15.2 and an iPhone 15
simulator on the newest installed iOS runtime:
- get-xcode-path.sh exited 1 when the pinned Xcode was absent, so `test
${DEVELOPER_DIR}` in setup failed and every target became unrunnable. It now
falls back to the active toolchain with a warning; XCODE_STRICT=1 restores the
hard failure and CI sets it.
- The destination was name-only, which xcodebuild resolves against OS:latest.
With a newer Xcode installed that runtime has no iPhone 15, so it failed with
"Unable to find a device matching the provided destination specifier" instead
of picking an older runtime. resolve-test-destination.sh now resolves a
concrete simulator UDID.
- The xcodebuild output was piped into xcbeautify unconditionally under `set -o
pipefail`, so a missing xcbeautify failed the build. It is now optional.
- brew bundle in setup was fatal, requiring Homebrew just to run tests.
- TEST_DESTINATION was exported by the Makefile but never read: the scripts used
DESTANATION from test.xcconfig. It is now an honored override, and the
misspelled variable is renamed to DEVICE_NAME.
There was a problem hiding this comment.
Pull request overview
This PR updates the repo’s build/test tooling to make make test runnable across varying local Xcode and simulator setups by resolving an available simulator destination at runtime, allowing optional log formatting, and avoiding hard failures when the pinned Xcode version isn’t installed (unless strict mode is enabled, as in CI).
Changes:
- Add a simulator destination resolver that emits a concrete simulator UDID (with
TEST_DESTINATIONoverride support). - Make
xcbeautifyoptional in test/build scripts (falls back to raw output when missing). - Update Xcode selection logic to fall back to the active toolchain locally while keeping CI strict via
XCODE_STRICT.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/test-without-building.sh | Uses resolved simulator destination + optional xcbeautify. |
| scripts/build-for-testing.sh | Uses resolved simulator destination + optional xcbeautify. |
| scripts/resolve-test-destination.sh | New resolver script that picks an available iOS simulator (or honors TEST_DESTINATION). |
| scripts/get-xcode-path.sh | Adds local fallback to active toolchain; CI can enforce strictness via XCODE_STRICT. |
| Makefile | Makes TEST_DESTINATION a real override; makes brew bundle non-fatal; adjusts setup validation. |
| configurations/test.xcconfig | Renames misspelled destination var to DEVICE_NAME. |
| .github/workflows/main.yml | Sets XCODE_STRICT=1 in CI to prevent silent fallback. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+57
to
+61
| ACTIVE_XCODE=$(xcode-select -p 2>/dev/null) | ||
|
|
||
| if [ -z "$ACTIVE_XCODE" ]; then | ||
| echo "No active Xcode found via xcode-select." 1>&2 | ||
| exit 1 |
Member
Author
There was a problem hiding this comment.
Correct, and this made the guard below it dead code. Verified: under set -e, V=$(false) aborts the script, so if [ -z "$ACTIVE_XCODE" ] never ran and a missing toolchain surfaced only as a bare exit 1.
A/B with a stubbed xcode-select that exits 1:
- before: script died right after listing available versions, no message
- after:
No active Xcode found via xcode-select.
Fixed with || true on the assignment.
Comment on lines
+52
to
+55
| if [ -n "$XCODE_STRICT" ]; then | ||
| echo "XCODE_STRICT is set, refusing to fall back." 1>&2 | ||
| exit 1 | ||
| fi |
Member
Author
There was a problem hiding this comment.
Agreed that XCODE_STRICT=0 enabling strict mode is a footgun. Went with [ -n "$XCODE_STRICT" ] && [ "$XCODE_STRICT" != "0" ] rather than a bare = 1, so 0/unset are off and anything else is on — comparing only to 1 would make XCODE_STRICT=true silently do nothing, which is the same trap in the other direction.
Verified: XCODE_STRICT=1 exits 1, XCODE_STRICT=0 falls back.
The first CI run resolved a different simulator in each script: build-for-testing: id=287A7916-2BD1-4674-8A22-38756D5658F4 test-without-building: id=F7860BC6-707F-40C6-8B2C-D38ECCD7AE42 xcodebuild adds "Clone N of <device>" simulators for parallel testing while it runs, so the device list is not stable across the two invocations and picking the newest match can land on a clone that has no built products. build-for-testing now records the destination it used under the derived data path, and test-without-building reuses it. An explicit TEST_DESTINATION still takes precedence. The resolver also skips clones outright.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (4)
scripts/test-without-building.sh:37
- After introducing
DERIVED_DATA_PATH, the xcodebuild invocation should also use it (and quote it) for-derivedDataPathto avoid word-splitting if the path contains spaces.
-destination "$DESTINATION" \
-sdk "$SDK" \
-configuration "$CONFIGURATION" \
-derivedDataPath $1 |
"${FORMATTER[@]}"
scripts/resolve-test-destination.sh:43
sort -k1,1Vuses GNU sort's version-key modifier (V), which is not supported by the default BSDsorton macOS. This can cause the resolver to fail on machines without GNU coreutils installed. Consider using a portable numeric sort for iOS version strings (e.g.-k1,1n).
print os "|" name "|" udid
}
' | sort -t'|' -k1,1V
)
scripts/build-for-testing.sh:14
- This script now writes
$1/test-destinationbut never validates that$1(derived data path) was provided. If invoked without an argument, it can end up writing to/test-destinationor failing in confusing ways. Capture and validate the argument once, and use it consistently (also quote it for-derivedDataPath).
# Resolve a simulator that exists on this machine (honors TEST_DESTINATION) and
# record it, so test-without-building runs on the device we built products for
DESTINATION=$(bash "$(dirname "$0")/resolve-test-destination.sh" "$DEVICE_NAME")
mkdir -p "$1"
echo "$DESTINATION" >"$1/test-destination"
scripts/test-without-building.sh:19
- This script reads
$1/test-destinationbut never validates that$1(derived data path) was provided. If invoked without an argument, it could accidentally read/test-destinationor fall back to a new destination unexpectedly. Capture and validate the derived data path once and use it consistently.
elif [ -f "$1/test-destination" ]; then
DESTINATION=$(cat "$1/test-destination")
else
DESTINATION=$(bash "$(dirname "$0")/resolve-test-destination.sh" "$DEVICE_NAME")
fi
eligutovsky
changed the title
- ACTIVE_XCODE=$(xcode-select -p) aborted the script under `set -e` when xcode-select failed, so the "No active Xcode found" guard below it was unreachable and the failure was reported only as a bare exit 1. Made the assignment non-fatal. - XCODE_STRICT treated any non-empty value as strict, so XCODE_STRICT=0 enabled it. Now 0 means off, matching the documented XCODE_STRICT=1. - Reworded "Failed to find Xcode X" to "Requested Xcode X not found", since the script goes on to fall back rather than failing. - setup checked DEVELOPER_DIR was non-empty; check it is a directory, restoring the guard strength while still allowing the fallback. - Quoted the derived data path passed to xcodebuild in both scripts.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (2)
scripts/test-without-building.sh:8
source $PWD/configurations/test.xcconfigis unquoted, so if the repo path contains spaces thesourcecommand will word-split and fail. Quote the path to make the script robust.
# Set the project variables
source $PWD/configurations/test.xcconfig
# Reuse the destination build-for-testing resolved. Resolving independently can
# pick a different device, because xcodebuild adds simulator clones for parallel
# testing while it runs, and there would be no built products for that device.
scripts/build-for-testing.sh:8
source $PWD/configurations/test.xcconfigis unquoted, so if the repo path contains spaces thesourcecommand will word-split and fail. Quote the path to make the script robust (similar to the quoting fixes already applied toderivedDataPath).
# Set the project variables
source $PWD/configurations/test.xcconfig
# Resolve a simulator that exists on this machine (honors TEST_DESTINATION) and
# record it, so test-without-building runs on the device we built products for
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of Changes
make testcould not run on a machine without Xcode 15.2 plus an iPhone 15 simulator onthe newest installed iOS runtime. Four independent blockers, all in the build tooling — no
SDK source is touched.
1. A missing pinned Xcode broke every target.
scripts/get-xcode-path.shexited1when
XCODE(default15.2) wasn't installed, sotest ${DEVELOPER_DIR}insetupfailed and nothing downstream could run. It now falls back to the active toolchain and says
so on stderr.
XCODE_STRICT=1restores the hard failure, and the CI workflow sets it, soCI still refuses to silently build with a different Xcode.
2. The simulator destination was unresolvable.
test.xcconfigspecifiedplatform=iOS Simulator,name=iPhone 15with no OS, and xcodebuild resolves a name-onlydestination against
OS:latest:With a newer Xcode installed,
latestis a runtime that has no iPhone 15, and xcodebuildfails rather than falling back to an older one. New
scripts/resolve-test-destination.shresolves a concrete simulator UDID: preferred device on the newest runtime that has it,
else the newest available iPhone, else any available simulator.
3.
xcbeautifywas mandatory. Its pipe ran underset -o pipefail, so a missingxcbeautifyfailed the build even though it only prettifies the log. Now optional, with anotice.
4.
brew bundleinsetupwas fatal, requiring Homebrew just to run tests. Nownon-fatal — the tools it installs are only needed by
formatand for pretty output.Also:
TEST_DESTINATIONwas exported by the Makefile but never read — the scripts sourcedDESTANATIONfromtest.xcconfig. It is now a real override(
make test TEST_DESTINATION="..."), and the misspelled variable is renamed toDEVICE_NAME, which is what it actually is now.5. The two scripts could resolve different simulators. Caught by the first CI run on
this branch, not by local testing:
xcodebuildaddsClone N of <device>simulators for parallel testing while it runs, sothe device list is not stable between the two invocations.
build-for-testingnow recordsthe destination it used under the derived data path and
test-without-buildingreuses it,so tests always run on the device the products were built for. The resolver also skips
clones. An explicit
TEST_DESTINATIONstill takes precedence.Verification
Ran the exact commands
make testruns aftersetup, on Xcode 26.6 with noxcbeautifyinstalled — the configuration that previously failed outright:
build-for-testingandtest-without-buildingboth exit 0. Resolver checked for: preferreddevice present, preferred device absent (falls back + warns), no preference, and
TEST_DESTINATIONoverride.get-xcode-path.shchecked for: missing pinned version(falls back, exit 0),
XCODE_STRICT=1(exit 1), and an installed version (resolvesnormally).
Not exercised locally: the
brew bundleline, since running it would installxcbeautifyand
swiftformaton this machine. CI will exercise it.Note:
scripts/check-headers.shfails onmasterindependently of this change (itsif [[ $? == 0 ]]aftergit grep -Lis unreachable-false underset -e, so it alwaysreports an error). Left alone — it only covers
*.{h,m,mm,swift}and excludesscripts/**, and CI does not runmake headers.CI on this branch is red for a pre-existing reason
AnalyticsHelperTestsfails with 5 tests / 1 failure in ~14s.masterfails identically(run
27026033610), as do 4 of its last 6 runs, so the red build predates this branch andis not caused by it. The 14s runtime points at the
DispatchSemaphorebarrier inAnalyticsHelper. Worth its own PR.SessionHelperTestsis separately flaky — it assertselapsedMs < 100and depends on2.0s/2.5s async delays, which is unreliable on a loaded runner. It passes consistently
locally and passed on a same-commit CI re-run.
Breaking Changes
Release Checklist
Build tooling only — no shipped code changes, so no version bump or release.
Prepare:
pod lib lintpasses — n/a, no podspec or source changeBump versions in:
n/a — no release.
Integration tests
n/a — no runtime code changed. Unit tests pass: 75 tests, 0 failures.
Release:
n/a — no release.