Repository navigation
fix(apple): fall back to the default Xcode past a dangling selection link - #4146
Conversation
…link xcode-select -p and xcrun skip a /var/db/xcode_select_link whose target was deleted and use /Applications/Xcode.app. #3995 replaced the xcrun probe with a file reader that failed with apple_toolchain_unreadable instead, which blocked iOS input on hosts with a stale link from an uninstalled Xcode. Any other unreadable link still fails. Fixes #4145
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Size Report
Startup median (7 runs, lower is better):
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 3 files
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
Apex SentinelVerdict: Since the last review: 4 new · 0 still open · 4 no longer raised 4 finding(s): 1 minor, 3 nit. Verdict: approve. reached max rounds (1). 🟡 Minor (1)
🔵 Nit (3)
Reply under an inline comment with |
![apex-sentinel-staging[bot]](https://avatars.githubusercontent.com/in/5196457?s=60&v=4){kind=small}
There was a problem hiding this comment.
Apex Sentinel: 3 new findings on 031c78f. The verdict and the full list are in the summary comment.
|
I read the change at 031c78f, and the fix itself looks right. The fallback past a dangling selection link works as described, and permission errors still fail loudly. CI is green with 19 checks and none failing, and the index-recovery-wiring flake you reported does not touch the toolchain identity resolver. There are no conflicts. One open thread, the fixture default dir below, should be fixed before merge; the other notes are optional. Not blocking, and you can take or leave these: the fixture at apple-toolchain-fixtures.ts#L5 redeclares the default developer dir as a literal, so exporting These review threads still apply: the fixture default dir thread is a real gap (with I did not run the tests, including the root-user and EACCES cases, and I did not reproduce the flake. I did no live run, since a dangling |
…ds behind it Report danglingSelectionLink and an xcode-select hint when the fallback default Xcode is missing too. Share one ENOENT/unreadable mapping for both selection-link reads. Keep the fixture hermetic against the host's default Xcode, and raise EACCES through the fixture so the case holds as root.
|
Addressed the review in b08ccb2:
Validation: |
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
There was a problem hiding this comment.
Apex Sentinel: 4 new findings on b08ccb2. The verdict and the full list are in the summary comment.
|
Thanks for the update. At b08ccb2 the earlier findings are fixed: the tests no longer read the host Xcode, and the EACCES case no longer depends on chmod or on running as non-root. One open thread, on the dangling-link hint wording below, still applies and should be settled before merge. I did not run the tests. I traced the mocked fs calls against the resolver at this commit. I also did not run against a real dangling /var/db/xcode_select_link, which needs root. Not blocking, and fine to take or leave: the dangling-link hint at https://github.com/callstack/agent-device/blob/b08ccb2/packages/apple-runner/src/runner-toolchain-identity.ts#L52 says no Xcode is installed at the default path, but it is chosen from the link state alone. If the default Xcode exists but lacks the requested SDK platform, or its version.plist or SDKSettings.json is unreadable, the message tells the user the wrong thing. Also, Could we drop the diagnostic layer this change added ( Of the other open threads, the one on the hint wording (#4146 (comment)) still applies, along with the matching wording thread (#4146 (comment)) and the same-name nit (#4146 (comment)). These are fixed at this head, so please resolve them: the fixture reading the host Xcode (#4146 (comment)), the EACCES test under root (#4146 (comment)), the missing dangling-link hint (#4146 (comment)) and the swallowed realPath errors (#4146 (comment)). These do not apply, because the fallback to the Xcode that Smoke Tests, Repo Guards & Compatibility and Coverage are still running on this commit, and the earlier commit passed all 19 checks. No conflicts. Once those three jobs pass, nothing else stands in the way of merging. |
The dangling-link hint asserted a cause the resolver never checked. Return the developer dir as a string again and add the xcode-select -s step to the hint that every toolchain-read failure carries.
|
Addressed in 2d51640: I removed the dangling-link diagnostic layer as suggested. Validation: |
There was a problem hiding this comment.
Apex Sentinel: 3 new findings on 2d51640. The verdict and the full list are in the summary comment.
|
I found no problems in 8b13b82. The findings from the earlier review (#4146 (comment)) are fixed: the hint no longer asserts a cause, and the dangling-link field and name collision are gone. CI is green: 18 checks, none failing. There are no conflicts. The earlier unrelated provider-revyl flakes in your local check:affected run did not show up on this head. I did not run the tests locally. I traced the fixture's fs mocks against the resolver at this head and relied on the green checks. I also did not see a live run with a real dangling /var/db/xcode_select_link, which needs root, as the PR body says. The one changed branch, the ENOENT mapping, is exercised by the fixture with a real ENOENT from realpathSync. I did not verify Apple's behavior for ELOOP or ENOTDIR on the link target, so those stay a hard failure by your choice. The bot threads are settled, so you can resolve them. These are fixed at this head: #4146 (comment), #4146 (comment), #4146 (comment), #4146 (comment) and #4146 (comment). These are accepted as intended behavior: #4146 (comment) (skipping a dangling link matches xcode-select -p), #4146 (comment) (DEFAULT_DEVELOPER_DIR is an exported constant, like XCODE_SELECT_LINK_PATH) and #4146 (comment) (naming the link was the diagnostic layer the maintainer asked to drop). Nothing else stands in the way. This is ready to merge once the maintainer is satisfied the open conversation is settled. |
thymikee
added
the
ready-for-human
Summary
Fixes #4145. In 0.21.34+, iOS input fails with
apple_toolchain_unreadablewhen/var/db/xcode_select_linkpoints at a deleted Xcode, even thoughxcode-select -pandxcrunresolve/Applications/Xcode.app.How it regressed: #3995 stopped running
xcrunto build the runner cache key and reads Xcode's files instead. To do that it copied Apple's developer-dir resolution:DEVELOPER_DIR, then the selection link, then the default Xcode. One rule did not match Apple: a dangling link was treated as a hard failure. Apple's tools skip it and use the default Xcode. A test (a selection link whose Xcode is gone fails …) locked in that assumption, so nothing caught it.Fix: if the link's target is missing (
ENOENT), the link is treated as absent, just like a missing link, and resolution uses the default Xcode. Any other unreadable link, such asEACCESon the link or its target, still fails. That keeps the #3995 guarantee: an unreadable selection never silently stands in for another Xcode.Prevention:
xcode-select -p, because the cache key must name the Xcode that builds the runner.Validation
pnpm check:affected --run --head 031c78fc13: all runnable checks passed,exactCommit=true(pre-squash head, same tree as the pushed commit).COMMAND_FAILED, dangling selection link) and passes with the fix.realPathswallowedEACCES. Both are fixed: that case is dropped and anEACCESnegative is added.packages/apple-runner/src/__tests__/index-recovery-wiring.test.tsfails 39 tests together in about 1 of 6 isolated runs at base32bf98def5./var/db/xcode_select_linkneeds root. The normal path (link resolves) is unchanged.