fix(gates): drop mapfile so the hardcoded-defaults gate runs on macOS - #729
Open
ayaangazali wants to merge 1 commit into
Open
fix(gates): drop mapfile so the hardcoded-defaults gate runs on macOS#729ayaangazali wants to merge 1 commit into
ayaangazali wants to merge 1 commit into
Conversation
`mapfile` is bash 4. macOS ships bash 3.2 as /bin/bash, so the gate exits 127
with `mapfile: command not found` before it reads a single file. It only ever
passes because the centralization job runs on ubuntu-22.04.
Two sibling scripts already state this constraint and avoid the same builtins
for it:
bindings/swift/scripts/sync-dist-repo.sh:178
Built with read loops rather than `mapfile`: macOS ships bash 3.2, which
has no mapfile, and this script must run on a stock macOS release runner.
scripts/build/build-core-android.sh:65
...array (`declare -A`) so this script works on macOS' default /bin/bash 3.2
This gate is the one place that breaks the rule, and it breaks it exactly when
a contributor on a Mac tries to reproduce a red gate locally: instead of the
list of offending files they get a bash error.
Replaced with the read loop the sibling scripts use. `FILES` stays an array, so
`${FILES[@]}` / `${#FILES[@]}` and `--list` are unchanged.
Contributor
Author
|
Heads up on the red Both are red on main too, at I opened #736 with the evidence and a one-line guard. Nothing to do on this PR; every other check here is green. |
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.
What is wrong
check_no_hardcoded_defaults.shcannot run on macOS. On a stock Mac:mapfileis bash 4. macOS ships bash 3.2 as/bin/bash(3.2.57 here), and the shebang is#!/usr/bin/env bash, so it resolves to that unless someone happens to have a newer bash earlier on PATH. The gate exits 127 before reading a single file, and it only ever passes becausepr-build.yml'scentralizationjob runs onubuntu-22.04.Why this is a rule and not my preference
Two sibling scripts already state the constraint and avoid these builtins because of it:
bindings/swift/scripts/sync-dist-repo.sh:178scripts/build/build-core-android.sh:65I swept the tree for the bash 4 builtins (
mapfile,readarray,declare -A,${x^^},${x,,}) and this line is the only violation left. The other two hits are those comments.It bites at the worst moment too: a contributor on a Mac reproducing a red gate locally gets a bash error instead of the list of offending files.
What this does
Replaces
mapfile -t FILESwith the read loop the sibling scripts use.FILESstays an array, so${FILES[@]},${#FILES[@]}, the empty-scope guard and--listall behave as before. 8 insertions, 1 deletion.Verification
On this Mac,
/bin/bash= 3.2.57, no Homebrew bash installed.Before: exit 127,
mapfile: command not found.After:
I checked the array is complete rather than assuming it, since a read loop can silently drop a final line without a trailing newline. Running the find pipeline standalone gives exactly
781, matching the gate's count, and--listprints 781 paths plus itsscanned 781 filesline.No test added: there is no harness for these gate scripts, and the check that matters is the one above, running it under the bash version that used to fail.
Summary by CodeRabbit