Skip to content

fix(ci): use portable fallbacks for bash-4 builtins and GNU sed in hook scripts#3694

Open
jiengup wants to merge 1 commit into
apache:masterfrom
jiengup:ci-mapfile
Open

fix(ci): use portable fallbacks for bash-4 builtins and GNU sed in hook scripts#3694
jiengup wants to merge 1 commit into
apache:masterfrom
jiengup:ci-mapfile

Conversation

@jiengup

@jiengup jiengup commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Closes #3693

Rationale

Several hook scripts use mapfile (bash >= 4) and sed -i without backup extension (GNU extension). On macOS stock bash 3.2 and BSD sed, these fail, blocking contributors from committing.

What changed

  • mapfile calls now guard with command -v mapfile and fall back to a while IFS= read -r loop.
  • sed -i calls is replaced by the sed -i.bak && rm pattern already established in trailing-whitespace.sh.

Local Execution

  • prek run -a

@github-actions

Copy link
Copy Markdown

Thanks for the PR. It is labeled S-waiting-on-review and queued for review.

Slash commands (own line, regular comment) move it around the queue:

  • /ready - back to S-waiting-on-review after addressing feedback
  • /author - flip to S-waiting-on-author while you finish changes
  • /request-review @user-or-team - request a reviewer

See CONTRIBUTING.md for details.

@github-actions github-actions Bot added the S-waiting-on-review PR is waiting on a reviewer label Jul 17, 2026

@hubcio hubcio left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

the -i.bak switch itself is the right call and matches the trailing-whitespace.sh precedent. main ask is the && rm -f chaining - it quietly replaces the old loud sed failure with a false "Fixed" + exit 0 (details in the line comments).

two findings sit on lines outside this diff, so noting them here:

  • scripts/verify-crates-publish.sh:313 - lc="${crate,,}" is bash-4-only, stock macOS bash 3.2 dies with bad substitution, and it fails late (after all four crates already published to the local registry). it's also a no-op since the crate names are already lowercase, so lc="$crate" fixes it for free.
  • scripts/ci/python-sdk-version-sync.sh:144 - sort -V is GNU coreutils-only, BSD sort rejects it. the --fix path hits it exactly when the two versions differ, i.e. the case the hook exists for. worth confirming on an actual mac.

with a\, \b and these two still around, #3693 ends up only partially closed for macos contributors - either keep the issue open or note a follow-up in the PR.

Comment thread scripts/ci/python-sdk-version-sync.sh
Comment thread scripts/ci/sync-python-interpreter-version.sh
Comment thread scripts/ci/sync-python-interpreter-version.sh
Comment thread scripts/ci/sync-rustc-version.sh
Comment thread scripts/ci/sync-rustc-version.sh
-e 's|^(iggy = \{ path = "[^"]+", version = "[^"]+")( \})$|\1, registry = "local-dev"\2|' \
-e 's|^(iggy-cli = \{ path = "[^"]+", version = "[^"]+")( \})$|\1, registry = "local-dev"\2|' \
Cargo.toml
Cargo.toml && rm -f Cargo.toml.bak

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

-i.bak now creates Cargo.toml.bak, but cleanup() only restores Cargo.toml from the mktemp snapshot - it never removes the .bak. if sed fails (the && skips the rm and the sanity check below exits 1) or a ctrl-c lands between sed and rm, Cargo.toml.bak survives the trap, which breaks the "leave the working tree untouched on exit, even on failure" contract at the top of this file. *.bak isn't gitignored either. one rm -f Cargo.toml.bak inside the CARGO_TOML_PATCHED branch of cleanup() covers all paths. the && itself is fine here - the sanity check right below fails closed.

Comment thread scripts/ci/edge-affected-images.sh
@github-actions github-actions Bot added S-waiting-on-author PR is waiting on author response and removed S-waiting-on-review PR is waiting on a reviewer labels Jul 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-author PR is waiting on author response

Projects

None yet

Development

Successfully merging this pull request may close these issues.

pre-commit hooks use bash-isms that block contributors on other shell (e.g, zsh)

2 participants