fix: unbreak main, a compile error and an empty xcodebuild argument - #2435
Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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.
mainis red on two independent counts. This fixes both. They are separate commits and can be split if you would rather.1. The build break, which is mine
fix(tabs): restore the quinaryLabel spelling that Xcode 26.4 compiles#2434shippedColor(nsColor: .quinaryLabelColor)and CI stopped at:The two spellings swap places between toolchains. The Xcode 27 beta I develop against marks
quinaryLabeldeprecated and points atquinaryLabelColor; Xcode 26.4.1, which CI builds with, rejectsquinaryLabelColoroutright as renamed toquinaryLabel. I took the beta's hint on a line I was already moving, and only CI has the toolchain that disagrees. Reverted to the spelling that was there before, with a comment so the next person does not take the same hint.It is one line and one root cause; the three reported failures are the file, its compile batch and the project.
2. The quarantine break, which predates it
ci: stop an empty test quarantine passing an empty argument to xcodebuildThe "Run unit tests" step has failed on every
mainrun since3cc00c0a1, with no test executing:scripts/ci/quarantine_args.pyends withprint("\n".join(args)). With no quarantine entries that isprint(""), which is a blank line. The workflow reads that stdout one line per argument, so the blank line becomes a literal empty argument and xcodebuild reads an empty argument as a build action.3cc00c0a1emptied.github/macos-test-quarantine.txt. The file's own header says "The list is empty. Keep it that way", so the empty list is the goal rather than an edge case, and the run history matches exactly:036945d51cut the list to 6 entries and passed; every run from3cc00c0a1onward failed.Measured on the runner's own shell,
/bin/bash 3.2.57:An empty array was never the problem; one empty string was.
Two changes, because the producer should not emit it and the consumer should not accept it:
quarantine_args.pyprints nothing when there is nothing to skip.The guard the script exists for is untouched: an entry that would skip nothing still fails the job with its hint, verified.
Test
scripts/ci/test_quarantine_args.py, run by a new Repo Hygiene step next to the existingtest_update_registry.py. Six cases: an empty list and a whitespace-only file print nothing at all, no emitted line is ever blank, a whole-suite entry still works, and both "would skip nothing" refusals still fail the job.It is a real regression test. Against the unfixed script it fails with
AssertionError: '\n'.Repo Hygiene is the right home: the macOS suite cannot catch this, because the script runs before any test does and when it gets this wrong no test runs at all.
3. The guard that had never once failed a job
ci: fail the test step when the quarantine or shard lister refuses to runFound while verifying the fix above, and it is the worse bug of the two.
The comment over the unit test step says the script "refuses to run if any entry would skip nothing". It does refuse: it prints its diagnostic and exits 1. But it is invoked inside a process substitution,
done < <(scripts/ci/quarantine-args.sh ...), and bash discards a process substitution's exit status. Measured:So an inert quarantine entry printed a warning nobody reads, the array came back empty, and the step ran the whole suite unskipped and reported success. The refusal that the script exists for, and that the comment promises, has never failed a job.
Both loops now redirect to a file on their own line, so
bash -esees the failure. Measured end to end on the new block:Two smaller things in the same lines:
[ "${#ONLY_ARGS[@]}" -gt 0 ]neighbour is blind to this shape: a blank line makes the count 1, so it reads as protection and is not. Measured on bash 3.2.57. That path cannot fire the bug today, becauselist-tests.shprints the runnable set rather than the quarantined one, so emptying its quarantine maximises output rather than emptying it. The guard is there so it stays that way."${SKIP_ARGS[@]}"is now${SKIP_ARGS[@]+"${SKIP_ARGS[@]}"}. The array is genuinely empty for the first time, and on bash 3.2 underset -uthe plain form is an "unbound variable" error. This step runsbash -etoday, but sibling steps in the same file already useset -euo pipefail.Verification
verify.sh buildPASSverify.sh test EditorTabStripChromeTests EditorTabStripSurfacesTestsPASS, 21 of 21actionlintclean on both changed workflows, which is what Repo Hygiene runs and is the only thing that checks an inlinerun:blockpython3 scripts/ci/test_quarantine_args.pypasses here and fails against the version onmain\n, and the read loop builds one empty argumentI could not build against Xcode 26.4.1: only the beta is installed here, which is the whole reason the first bug reached CI. CI is the check for that one.
No CHANGELOG entry: neither change alters anything a user sees.
quinaryLabelandquinaryLabelColorare the same colour.