Skip to content

fix(bench): support macos bash in benchmark script#371

Merged
jdx merged 1 commit intomainfrom
codex/bench-bash3-compat
Apr 28, 2026
Merged

fix(bench): support macos bash in benchmark script#371
jdx merged 1 commit intomainfrom
codex/bench-bash3-compat

Conversation

@jdx
Copy link
Copy Markdown
Contributor

@jdx jdx commented Apr 28, 2026

Summary

  • replace the benchmark script's Bash 4 associative-array command map with a Bash 3-compatible case helper
  • leave the benchmark task tool providers unchanged, including aqua:pnpm/pnpm@latest

Why

macOS still ships Bash 3.2 by default, so benchmarks/bench.sh failed on declare -A before it could run the hyperfine matrix. This keeps the same per-scenario commands while making the dispatch portable to the default macOS shell.

Validation

  • bash -n benchmarks/bench.sh
  • git diff --check

I also confirmed the earlier one-run smoke got past pnpm 11 provisioning when using npm:pnpm@11, but reverted that provider change per follow-up because aqua:pnpm/pnpm@latest is expected to work again later.

This PR was generated by Codex.


Note

Low Risk
Low risk: this is a refactor of benchmark command dispatch to avoid Bash 4-only associative arrays, with no production/runtime impact beyond the benchmarking harness.

Overview
Makes benchmarks/bench.sh compatible with macOS’s default Bash (3.2) by replacing the Bash 4 declare -A scenario/tool command map with a cmd_template case helper.

Updates run_bench and run_bench_preinstall to fetch per-scenario commands via cmd_template, keeping the underlying benchmark commands and hermetic env flags effectively the same while improving portability.

Reviewed by Cursor Bugbot for commit e51dc9a. Bugbot is set up for automated code reviews on this repo. Configure here.

@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented Apr 28, 2026

Greptile Summary

This PR replaces the declare -A CMDS associative-array lookup in benchmarks/bench.sh with a cmd_template() function backed by a case statement, making the script compatible with the Bash 3.2 that ships on macOS. All 30 scenario:tool command mappings have been checked against the original and are faithfully reproduced, including the warm/cold and GVS-on/off distinctions.

Confidence Score: 5/5

Safe to merge — a mechanical, correct substitution with no logic changes.

No functional or logic issues found. All scenario:tool pairs are correctly translated from the associative array to the case statement, the local cmd_tpl split avoids masking subshell exit codes (a mild best-practice improvement), and the missing * catch-all is handled by the existing empty-string guard already present in run_bench and run_bench_preinstall.

No files require special attention.

Important Files Changed

Filename Overview
benchmarks/bench.sh Replaces Bash 4 associative array (declare -A CMDS) with a Bash 3-compatible cmd_template() case function; all scenario:tool mappings are faithfully preserved and the local split correctly avoids masking subshell exit codes.

Reviews (1): Last reviewed commit: "fix(bench): support macos bash in benchm..." | Re-trigger Greptile

@jdx jdx merged commit 661ad98 into main Apr 28, 2026
19 checks passed
@jdx jdx deleted the codex/bench-bash3-compat branch April 28, 2026 21:56
jdx added a commit that referenced this pull request Apr 28, 2026
## Summary

- replace the benchmark script's Bash 4 associative-array command map
with a Bash 3-compatible `case` helper
- leave the benchmark task tool providers unchanged, including
`aqua:pnpm/pnpm@latest`

## Why

macOS still ships Bash 3.2 by default, so `benchmarks/bench.sh` failed
on `declare -A` before it could run the hyperfine matrix. This keeps the
same per-scenario commands while making the dispatch portable to the
default macOS shell.

## Validation

- `bash -n benchmarks/bench.sh`
- `git diff --check`

I also confirmed the earlier one-run smoke got past pnpm 11 provisioning
when using `npm:pnpm@11`, but reverted that provider change per
follow-up because `aqua:pnpm/pnpm@latest` is expected to work again
later.

*This PR was generated by Codex.*

<!-- CURSOR_SUMMARY -->
---

> [!NOTE]
> **Low Risk**
> Low risk: this is a refactor of benchmark command dispatch to avoid
Bash 4-only associative arrays, with no production/runtime impact beyond
the benchmarking harness.
> 
> **Overview**
> Makes `benchmarks/bench.sh` compatible with macOS’s default Bash (3.2)
by **replacing the Bash 4 `declare -A` scenario/tool command map** with
a `cmd_template` `case` helper.
> 
> Updates `run_bench` and `run_bench_preinstall` to fetch per-scenario
commands via `cmd_template`, keeping the underlying benchmark commands
and hermetic env flags effectively the same while improving portability.
> 
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit
e51dc9a. Bugbot is set up for automated
code reviews on this repo. Configure
[here](https://www.cursor.com/dashboard/bugbot).</sup>
<!-- /CURSOR_SUMMARY -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant