fix: correct faulty substitutions in update-version.sh - #909
Open
arcusbuilds wants to merge 6 commits into
Open
Conversation
The script runs under 'set -u', so the documented fallback invocation
'bash update-version.sh <version>' aborted with 'RAPIDS_RUN_CONTEXT:
unbound variable' before any substitution ran. Use ${VAR:-} so the
documented default-to-main behavior actually happens.
The minor-version group was a single [[:digit:]] rather than [[:digit:]]\+, so bumping 26.06 -> 26.08 rewrote only 'RAPIDS_VER=26.0' and left the trailing digit, producing 'RAPIDS_VER=26.086'. That value is live on release/26.08 today. The other version-matching seds in this file already use the one-or-more form, apart from the SECURITY.md line just below, which has the same bug and is removed in a later commit.
's|@.*|@ref|' is greedy to end-of-line, so rewriting a ref also deletes whatever follows it on that line. Cutting release/26.06 (935ed5a) stripped the trailing '# zizmor: ignore[unpinned-uses]' comments from all three shared-workflows 'uses:' lines and broke pre-commit (rapidsai#877). rapidsai#877 fixed the fallout by adding .github/zizmor.yml but left the sed alone, so cutting release/26.08 (d711b42) stripped the two remaining comments again. Those comments are redundant today, since .github/zizmor.yml exempts 'rapidsai/shared-workflows/*' from unpinned-uses. But the sed still silently drops any trailing comment on a line it rewrites, which is churn in every release diff and a trap for any future comment. Anchor the match to the ref token with '@[^[:space:]]\+' and drop the 'g' flag, since there is one ref per line. Refs with no trailing comment, such as the one in trigger-breaking-change-alert.yaml, are still rewritten.
SECURITY.md carries no version string, so this sed has matched nothing since it was added in rapidsai#877. It also had the same single-digit-minor bug as the CONTRIBUTING.md line. Removing rather than repairing it, since there is nothing in that file to keep in sync.
arcusbuilds
force-pushed
the
fix/update-version-sed-907
branch
from
August 9, 2026 11:07
ef9be3b to
24833e3
Compare
Author
|
@jameslamb @raydouglass could one of you take a look when you get a chance? One open question from the description: I deleted the SECURITY.md call since it has never matched anything, but happy to restore it, if you'd rather keep it |
msarahan
requested review from
msarahan
and removed request for
KyleFromNVIDIA
August 18, 2026 23:05
msarahan
approved these changes
Aug 18, 2026
msarahan
left a comment
Contributor
There was a problem hiding this comment.
approved, but one question about quantifier
Co-authored-by: Mike Sarahan <msarahan@gmail.com>
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.
Closes #907
What the script does
ci/release/update-version.shbumps the RAPIDS version across the repo when a release branch is cut. It is a flat list ofsed_runnercalls, one per file, each rewriting a version string in place.The problems
Running it on
maintoday produces two bad substitutions and one that has never matched anything. A fourth bug stops it running at all through the no-context invocation both of its documented interfaces allow.1.
CONTRIBUTING.mdgains a stray digitThis is the reported bug. The minor-version group was a single
[[:digit:]]instead of[[:digit:]]\+, so bumping26.06to26.08rewrote onlyRAPIDS_VER=26.0and left the trailing6behind:That value is live on
release/26.08. Any minor with two or more digits breaks the same way: the pattern eats one digit of the old minor and leaves the rest appended, so the next bump from today'smainwould giveRAPIDS_VER=26.106. The other version-matchingsed_runnercalls already use the one-or-more form, apart from theSECURITY.mdline in (3), which has the identical bug.2. Trailing comments get deleted from
shared-workflowsrefss|@.*|@REF|is greedy to end of line, so rewriting a ref also deletes whatever follows it. Cuttingrelease/26.06in 935ed5a stripped# zizmor: ignore[unpinned-uses]from all threeshared-workflowsuses:lines and broke pre-commit, which is what #877 was cleaning up.#877 fixed the fallout by adding
.github/zizmor.ymland left the sed alone, so cuttingrelease/26.08in d711b42 stripped the two remaining comments again. Those two are redundant now that the zizmor config exemptsrapidsai/shared-workflows/*fromunpinned-uses, so this is no longer a CI break. But the sed will still drop any trailing comment on any line it rewrites.3. The
SECURITY.mdcall does nothingSECURITY.mdhas no version string in it, sos|[[:digit:]]\+\.[[:digit:]]-cuda|...|has matched nothing since #877 added it. It carries the same missing\+as (1), so it could not have matched06-cudaeven if a version were there.4. The no-context invocation aborts
The header documents a primary interface with an optional
--run-contextflag and a fallback interface with an optionalRAPIDS_RUN_CONTEXTvariable, and says it defaults tomainwhen neither is given. That default path reads the variable before checking whether it exists, soset -ukills the script before any substitution runs.The fix
Four one-line changes, one commit each:
\+so theCONTRIBUTING.mdpattern matches a full minor version.@[^[:space:]]\+and drop thegflag, since there is one ref per line. Refs with no trailing comment, such as the one intrigger-breaking-change-alert.yaml, are still rewritten.SECURITY.mdcall. Happy to restore it as[[:digit:]]\+\.[[:digit:]]\+-cudainstead if you would rather keep it as future-proofing.${RAPIDS_RUN_CONTEXT:-}so the documented default actually happens. This one is fixed first, because it is what makes the other three reproducible locally.Testing
Same approach as #877:
Both context values were exercised through both interfaces, plus the bare default. CLI precedence over the env var still holds, and invalid values exit 1 without touching the tree. Running twice gives the same diff as running once. A release run followed by a main run returns the three workflow refs and the
DockerfileRAPIDS_BRANCHto@main, comments intact; the doc bumps correctly persist.I also ran the two new patterns against fixtures outside the repo. For (1):
26.06,26.6,26.10,26.100,9.1,26.06.00, and a bareRAPIDS_VER=with no digits. The new pattern is right on all seven; the old one mangles four of them (26.06,26.10,26.100,26.06.00) and no-ops identically on the rest. For (2): a ref with a trailing comment, one without, a comment containing its own@(left alone, which is why theggoes), a 40-character SHA pin (still rewritten), and ashared-actionsline (untouched, since the address is/shared-workflows/).pre-commit run --all-filespasses. With aGH_TOKENin the environment,zizmoradditionally resolves tags and reports 12 pre-existing mediumref-version-mismatchfindings onactions/*hash pins. Those are all present onmainand none are in a file this PR touches.Only GNU sed was exercised.
\+is already the idiom throughout the file and the bracket expressions are POSIX, sosed -i.bakportability should be unchanged.One thing I noticed
Unrelated to the fix: docs on
mainare still at26.06(CONTRIBUTING.md,dockerhub-readme.md,cuvs-bench/README.md,tests/container-canary/README.md) while the Dockerfiles are at26.10, so the 26.08 and 26.10 burndowns look like they skipped them. I kept this PR to the script. Happy to add the doc bump here or in a follow-up.