Skip to content

fix(arcup): a release is newer than its own pre-release - #300

Open
JspIIV wants to merge 2 commits into
circlefin:mainfrom
JspIIV:fix/version-gt-release-beats-prerelease
Open

fix(arcup): a release is newer than its own pre-release#300
JspIIV wants to merge 2 commits into
circlefin:mainfrom
JspIIV:fix/version-gt-release-beats-prerelease

Conversation

@JspIIV

@JspIIV JspIIV commented Sep 1, 2026

Copy link
Copy Markdown

What

version_gt drops the pre-release tag from both arguments before it compares anything:

ver1="${ver1%%-*}"
ver2="${ver2%%-*}"

So 0.3.0 and 0.3.0-rc.1 both reduce to 0.3.0, every major/minor/patch comparison falls through, and the function reaches its final return 1. SemVer §11 puts a release above any pre-release of the same version, so this one should be true.

Measured against the current script:

call result expected
version_gt 0.3.1-rc.1 0.3.0 true true
version_gt 0.3.0-rc.1 0.3.0 false false
version_gt 0.3.0 0.3.0-rc.1 false true
version_gt 0.3.0 0.3.0-rc.2 false true

Why it matters

It affects anyone running a pre-release of the installer itself. check_installer_up_to_date never prints the "outdated" warning once the release ships, and update_arcup refuses to move:

if ! version_gt "$remote_version" "$ARCUP_INSTALLER_VERSION"; then

so arcup --self-update from 0.3.0-rc.1 to 0.3.0 reports that it is already current. There is no way out of an rc build except reinstalling by hand.

test_version_comparison covers the two cases that already worked and not this one, which is why it went unnoticed.

Change

Keep the pre-release tags aside instead of discarding them, and when major/minor/patch are equal treat an empty tag as the higher precedence.

Ordering two pre-releases of the same version (rc.2 vs rc.10) is deliberately left alone — it needs the full SemVer identifier comparison and no caller does it, since arcup only ever compares against its own version. Happy to add it if you would rather have it complete.

ARCUP_INSTALLER_VERSION bumped to 0.2.1, per the note at the top of the script.

Tests

Two cases added to test_version_comparison. Against main the first one fails:

ok - compares prerelease installer versions
ok - same prerelease base is not newer
not ok - release is newer than its prerelease

With the change, bash arcup/test_arcup.sh passes through to archive path traversal fails.

One note on running the suite locally: test_archive_link_entries_fail fails on a Windows checkout because ln -s needs privileges there. It fails the same way on an untouched tree, so it is unrelated to this change — I could not exercise that case or the two after it.

Closes #205.

version_gt strips the pre-release tag from both arguments before comparing,
so 0.3.0 and 0.3.0-rc.1 reduce to the same numbers, fall through every
comparison and reach the final `return 1`. SemVer orders a release above any
pre-release of the same version, so the answer should be true.

The effect is on anyone running a pre-release of the installer.
check_installer_up_to_date never tells them the release shipped, and
update_arcup refuses to move:

    if ! version_gt "$remote_version" "$ARCUP_INSTALLER_VERSION"; then

so `arcup --self-update` from 0.3.0-rc.1 to 0.3.0 reports it is already
current. The existing tests cover the two cases that already worked and not
this one.

Keep the pre-release tags aside and, when major.minor.patch are equal, treat
an empty tag as the higher precedence. Ordering two pre-releases of the same
version is left alone, since no caller compares them.

Installer version bumped per the note at the top of the script.
@romac romac added pending-import Merged PR awaiting reverse-sync to upstream and removed pending-import Merged PR awaiting reverse-sync to upstream labels Sep 1, 2026
@JspIIV

JspIIV commented Sep 1, 2026

Copy link
Copy Markdown
Author

The red Rust Integration Tests job here is #298, not this change.

Same test, same signal:

SIGKILL [11.466s] (11/13) arc-test-integration::basic validators_and_full_nodes_reach_height_3
(test aborted with signal 9: SIGKILL)
Summary [70.519s] 11/13 tests run: 10 passed, 1 failed, 0 skipped

which matches the report in #298 that the job is red on unrelated branches including sync/v0_8_0.

This PR touches arcup/arcup and arcup/test_arcup.sh only, so it has no path to the consensus integration suite. Every other check is green.

Worth noting alongside #248 — the arcup shell suite is not run in CI, so the two cases added here would not be exercised there either until that lands.

@osr21

osr21 commented Sep 1, 2026

Copy link
Copy Markdown

Confirming the CI attribution from the #298 side (I filed the job-level history analysis there): the only failing check on this head is Rust Integration Tests (Public CI run 33482375784) — every other check is green — and the quoted signature (SIGKILL on validators_and_full_nodes_reach_height_3, 11/13 run) is exactly the marginal OOM pattern documented in #298, which reproduces on unrelated branches including sync/v0_8_0. A shell-only change to arcup/arcup and arcup/test_arcup.sh has no path into the consensus integration suite, so the red job carries no signal about this PR.

I also ran the parts your Windows checkout couldn't. On Linux at head 54236e5, bash arcup/test_arcup.sh passes 27/27, exit 0 — including archive link entries fail and install_binary rejects symlink, the two cases after the traversal test that ln -s privileges blocked for you. For completeness I ran the suite on untouched main too (25/25 — so the two new assertions are the only behavioral delta in the suite).

Verification of the fix itself, from sourcing version_gt directly:

  • On main, the table in the PR body reproduces verbatim — 0.3.0 vs 0.3.0-rc.1 and 0.3.0-rc.2 both come back false.
  • With the patch: 0.3.0 > 0.3.0-rc.1 ✔, v0.3.0 > v0.3.0-rc.2 ✔, and the edge semantics hold — 0.3.0-rc.1 > 0.3.0 stays false, two pre-releases of the same base are unordered in both directions (rc.2 vs rc.1 → false, as the comment documents), and identical strings stay false via the early equality return. The deliberate scope cut is sound: the only call sites (check_installer_up_to_date, update_arcup) compare a remote version against the installer's own, so full SemVer identifier ordering has no caller today.

And confirmed on the #248 point: no workflow references arcup anywhere under .github/workflows/, so the two assertions added here will not run in CI until #248 lands — this PR is a concrete example of what that gap costs, since the missing case would have been caught the day a CI-run suite covered it.

@romac

romac commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

@JspIIV Thanks! Can you please merge latest main into this branch?

@JspIIV

JspIIV commented Sep 2, 2026

Copy link
Copy Markdown
Author

Done — main merged in at c6f48d9, signed. The merge touched crates/malachite-app, crates/test/integration, deployments/ and docs/ only, so nothing in arcup/ moved.

Re-ran the shell suite on the merged head:

ok - compares prerelease installer versions
ok - same prerelease base is not newer
ok - release is newer than its prerelease
ok - release is newer than its prerelease with v prefix

The last two are the cases this PR adds; the first two are the ones that already passed.

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.

Bug: arcup SemVer comparison treats prerelease and stable versions as equal

3 participants