fix(scripts): make check-authors work on macOS bash 3.2 - #423
Merged
Conversation
The script aborted immediately on macOS with "declare: -A: invalid
option". It relied on four constructs unavailable there: associative
arrays and readarray (bash 4.0+), the -v array-key test (bash 4.2+),
and grep -oP with \K (GNU grep; /usr/bin/grep rejects -P).
Replaces them with an indexed array plus the script's existing
array_contains helper, while-read loops, and a POSIX sed expression.
Empty-array expansions now use the ${arr[@]+...} idiom already used
elsewhere, since bash 3.2 under `set -u` errors on "${empty[@]}".
The known-authors scan also excludes */target/* so it matches the
file-discovery filter and is unaffected by whether a build has run.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Marc Nuri <marc@marcnuri.com>
There was a problem hiding this comment.
🟡 Changes recommended
The known-authors scan no longer suppresses stderr, which can introduce user-visible behavioral differences versus the prior 2>/dev/null behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR updates scripts/check-authors.sh to run correctly on macOS’s default Bash 3.2 by removing Bash 4+/GNU grep dependencies, keeping make check-authors usable for contributors on macOS.
Changes:
- Replaces Bash 4 associative array usage and
[[ -v ... ]]membership checks with plain arrays + an existingarray_containshelper. - Replaces
readarraywithwhile IFS= read -rloops for Bash 3.2 compatibility. - Replaces GNU
grep -Pextraction with a portablesed -nextraction.
File summaries
| File | Description |
|---|---|
| scripts/check-authors.sh | Makes the author-checking script compatible with macOS Bash 3.2 by removing Bash 4+/GNU grep-only constructs. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The rewrite dropped the `2>/dev/null` the original grep had, so unreadable files or transient errors would leak onto the terminal mid-run. Restores it on the find invocation, which also covers the -exec'd sed since children inherit find's stderr. Matches the suppression already used by get_javadoc_authors and get_git_authors. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Marc Nuri <marc@marcnuri.com>
There was a problem hiding this comment.
🟢 Approval recommended
The changes are localized to the script, remove Bash 4+/GNU grep dependencies as intended, and I didn’t find any functional regressions in the updated parsing/loops.
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Lite
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.
Problem
make check-authorsaborts immediately on macOS:macOS ships bash 3.2, and the script relied on four constructs that aren't
available there — the
declare -Afailure was just the first one hit.declare -A KNOWN_AUTHORSreadarray[[ -v KNOWN_AUTHORS[$author] ]]grep -oP '@author\s+\K.+'-PChanges
array_containshelper (which also replaces the-vkey test).readarray→while IFS= read -rloops.grep -oP ... \K→ POSIXsed -n 's/.*@author[[:space:]][[:space:]]*//p'.${arr[@]+"${arr[@]}"}idiom the scriptalready used elsewhere — bash 3.2 under
set -uerrors on"${empty[@]}".*/target/*, matching the filter alreadyused for file discovery, so results don't change depending on whether a build
has run.
No behavioural change intended: the
sedextraction was verified byte-identicalto the original PCRE version across every Java file in the repo.
Note
The script is a manual target (CI does not run it). Now that it executes on
macOS, it reports 10 pre-existing missing
@authorentries across 8 files.Those are left untouched here — this PR only fixes the script.