Conversation
is_newer_version() pushed every version segment through bash base-10
arithmetic. A segment like "13+dlc1" made bash resolve `dlc1` as an
identifier, which aborts the script under `set -u`:
$ is_newer_version "0.5.14" "0.5.13+dlc1"
utils.sh: line 82: dlc1: unbound variable
check-upstream-releases.sh runs each framework in a `set -euo pipefail`
subshell, so this killed the framework's whole update block and reported
only a bare exit code. The `+dlc<n>` suffix is a live convention here —
sglang/ec2-amzn2023.yml and sglang/sagemaker-amzn2023.yml carry
0.5.14+dlc1 today — so the nightly tracker was one config reorder away
from breaking. Without `set -u` the failure is quieter and worse: bash
resolves the unknown identifier to 0 and the comparison is silently wrong.
The comparison also stopped after three segments, so a four-segment
upstream release read as identical to its predecessor and never raised
an update PR.
Extract release_segments(), which keeps the leading digits of each
segment and stops at the first suffix, so "+dlc1", ".post1" and ".dev361"
no longer reach the arithmetic. Compare across every segment present,
padding the shorter side. Malformed input now returns 2 with a message
on stderr instead of aborting; callers testing `if ! is_newer_version`
treat that as "not newer" and skip, which is the safe direction.
Add utils_test.sh, a dependency-free bash runner covering the two
regressions plus the existing numeric, padding and leading-zero
behavior. It aborts on the first assertion against the old
implementation.
Closes aws#6516
Signed-off-by: Aditya Jain <adityaj0@uci.edu>
Author
|
@sallyseok can you review this? |
Contributor
|
This PR has been marked stale due to 30 days of inactivity. Please comment or remove the stale label to keep it open. It will be closed in 5 days if no further activity occurs. |
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
1 participant
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.
Purpose
Fixes #6516.
is_newer_version()inscripts/ci/autocurrency/utils.shpushed every version segment through bash base-10 arithmetic. A segment like13+dlc1makes bash parse10#13 + dlc1and try to resolvedlc1as a variable, which aborts under theset -uthatutils.shsets at the top:check-upstream-releases.shruns each framework inside aset -euo pipefailsubshell, so this kills that framework's entire update block and surfaces only::warning::sglang: Processing failed with exit code 1.The
+dlc<n>suffix is a live convention here — documented incheck_framework_version_currency.pyasframework_version == "<upstream_release>[+dlc<n>]", and present today insglang/ec2-amzn2023.ymlandsglang/sagemaker-amzn2023.ymlas0.5.14+dlc1. The trackedconfig_files[0]entries are plain numeric right now, so the nightly job survives on luck; a config reorder or a+dlcrebuild on a tracked config breaks it.Separately, the loop was fixed at
for i in 0 1 2, so1.0.0.1compared equal to1.0.0and no update PR would ever be raised.Changes:
release_segments()keeps the leading digits of each segment and stops at the first suffix, so+dlc1,.post1and.dev361never reach the arithmetic.2with a stderr message instead of aborting. Callers useif ! is_newer_version ..., so that reads as "not newer" and skips — the safe direction.Test Plan
New
scripts/ci/autocurrency/utils_test.sh— plain bash, no dependencies, runnable directly:25 assertions covering both regressions plus the existing numeric, padding and leading-zero behaviour. There was no shell unit-test harness under
scripts/ci/, so this adds a minimal one rather than pulling in bats.Lint/format checks run on the changed files:
Test Result
Against the pre-fix implementation the suite aborts on the first
+dlc1assertion, confirming these are genuine regression tests rather than tests written to pass.Behaviour table:
is_newer_version 0.5.14 0.5.13+dlc1unbound variable)0— neweris_newer_version 0.5.13 0.5.13+dlc11— not neweris_newer_version 1.0.0.1 1.0.01— not newer (wrong)0— neweris_newer_version 0.17.0 0.16.00— newer0— neweris_newer_version notaversion 0.17.00— newer (wrong)2+ stderrbash -npasses on both shell files.Toggle if you are merging into main Branch
PR Checklist
pre-commit run --all-fileslocally before creating this PR.Not fully run — disclosing rather than checking the box.
pre-commitcould not install its hook environments in my sandbox (URLError: CERTIFICATE_VERIFY_FAILEDfetching the hook repos). What I ran instead, directly:bash -non both changed shell files — passNot run, and worth a maintainer's eye on CI: shfmt (the formatting hook that applies to these files), typos, gitleaks. I matched the existing 2-space indentation and comment-banner style of
utils.sh, but shfmt has not verified that.