Skip to content

refactor(scripts): extract release helpers into common.sh - #4146

Open
SimonRastikian wants to merge 1 commit into
mainfrom
3934-1-common-helpers
Open

refactor(scripts): extract release helpers into common.sh#4146
SimonRastikian wants to merge 1 commit into
mainfrom
3934-1-common-helpers

Conversation

@SimonRastikian

Copy link
Copy Markdown
Contributor

This is a first cleanup for #3934 it's a split from #4100.

It basically moves generic functions into a file common.rs to be used in the next PRs that build the scripts.

@claude

claude Bot commented Aug 14, 2026

Copy link
Copy Markdown

Pull request overview

Extracts three generic bash helpers (die, require_cmds, and a new check_version) out of prepare-github-release.sh into a new sourced scripts/ops/common.sh, as groundwork for #3934 (split out of #4100). die gains TTY/NO_COLOR-aware red colouring, and the inline semver check is replaced by check_version, which additionally accepts a pre-release suffix. The rest of the diff condenses four explanatory comment blocks in the release script.

Changes:

  • New scripts/ops/common.sh with die, require_cmds, check_version, plus C_ERR/C_RESET guarded on [[ -t 1 && -t 2 ]] and NO_COLOR
  • prepare-github-release.sh sources it, drops its local usage/die/require_cmds copies, and collapses arg parsing to two lines
  • Version validation widened from X.Y.Z to X.Y.Z[-SUFFIX], matching .github/workflows/release.yml:39
  • Four comment blocks (changelog rationale, POSIX-tools note, ABI-check note) rewritten to one or two lines

Reviewed changes

Per-file summary
File Description
scripts/ops/common.sh New sourced helper library: colour vars, die, require_cmds, check_version
scripts/ops/prepare-github-release.sh Sources common.sh, removes the duplicated helpers and inline semver regex, condenses explanatory comments

Findings

Blocking (must fix before merge):

  • scripts/ops/prepare-github-release.sh:81 — the rewritten comment inverts the code it describes. test_abi_has_not_changed (crates/contract/tests/abi.rs:45) passes when the ABI is unchanged, which is why lines 83-85 die on success. The new text ("Test fails if ABI was not affected by the version bump") states the opposite, and "(the expected case)" is left dangling. This is worse than no comment — a reader trusting it would conclude the if branch is dead code. Either restore the previous wording or use something like:

    # A version bump must change the ABI, so this snapshot test is expected to
    # fail here; passing means the bump did not reach the contract.
  • scripts/ops/common.sh:32 — widening the accepted version to X.Y.Z[-SUFFIX] lets the script accept input its downstream steps cannot process. prepare-github-release.sh:73 and :77 both anchor on ^version = "[0-9]+\.[0-9]+\.[0-9]+" with a closing quote right after the patch digit, so neither matches version = "3.6.0-rc.1". The first rc bump succeeds (old version is plain X.Y.Z), but any subsequent run against the resulting Cargo.toml dies at line 74 with the misleading Could not find a workspace 'version = "X.Y.Z"' line. Note release.yml:34 deliberately omits the closing quote in its awk pattern for exactly this reason. Either keep the strict regex in this consumer, or align both patterns:

    OLD_VERSION=$(awk -F'"' '/^version = "[0-9]+\.[0-9]+\.[0-9]+/ {print $2; exit}' "$CARGO_TOML")
    sed -i.bak -E "s/^version = \"[0-9]+\.[0-9]+\.[0-9]+(-[a-zA-Z0-9.]+)?\"/version = \"${VERSION}\"/" "$CARGO_TOML"

    Relatedly, RELEASES.md documents only X.Y.Z releases — if release candidates are now a supported flow, that section needs a corresponding note; if they are not, the "so release candidates work too" claim on common.sh:30 overstates what this enables.

Non-blocking (nits, follow-ups, suggestions):

  • scripts/ops/prepare-github-release.sh:70 — "GNU sed/grep won't work on macOS" is not accurate; GNU tools run fine on macOS when installed. The constraint is that macOS ships BSD userland, so GNU-only flags (grep -P, bare sed -i) are unavailable — which is what the old comment said and what actually guides the next editor. The script also contains no grep at all (pre-existing in the old text), and the comment now floats two lines above the sed it constrains. Suggest attaching it to line 77 and dropping the grep mention.

  • scripts/ops/prepare-github-release.sh:61-62 — the condensed version now restates the flags visible on line 66 (--prepend, concrete SHA) while dropping the genuinely non-obvious why: git-cliff derives its PR/author metadata ref from the range head, and a literal HEAD resolves to the default branch. That rationale already lives in full at RELEASES.md:236-247, so a pointer beats a lossy paraphrase — e.g. # See RELEASES.md § Changelog conventions for why the range head is a concrete SHA. The trailing .cliffignore clause is also now context-free (the original said when to append).

  • scripts/ops/common.sh:21cmd is not declared local (carried over verbatim). Harmless in the old single-script form, but common.sh is explicitly a shared library now, so the loop variable leaks into every sourcing script's global scope. local cmd missing=0 closes it.

⚠️ Issues found

@pbeza

pbeza commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

I'm a bit hesitant to accept it if this PR doesn't actually reduce the LoC, only follow-ups do.

bash

Also, it feels like a first step toward making the Bash scripts more complex by introducing a shared “library,” which I’d prefer to avoid. We should reduce our use of Bash rather than rely on it more.

@SimonRastikian

Copy link
Copy Markdown
Contributor Author

@SimonRastikian

SimonRastikian commented Aug 14, 2026

Copy link
Copy Markdown
Contributor Author

@pbeza if you can find me a tangible solution/design I would be happier

@pbeza

pbeza commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

@pbeza if you can find me a tangible solution I would be happier

When it comes to this PR: as I mentioned, I’d be happy to accept a common Bash library if it reduced LoC in the repo. But it seems like this is mostly preparation for something bigger and Bash-heavy, which might not be accepted by the team given our preference to avoid large Bash tooling.

I think this deserves a broader async discussion, so we can continue in the Slack thread you posted. I see there’s already some discussion going on there.

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.

2 participants