Skip to content

btcctl: preserve semantic version separators - #2596

Open
pucedoteth wants to merge 2 commits into
btcsuite:masterfrom
pucedoteth:fix-btcctl-version-separators
Open

btcctl: preserve semantic version separators#2596
pucedoteth wants to merge 2 commits into
btcsuite:masterfrom
pucedoteth:fix-btcctl-version-separators

Conversation

@pucedoteth

Copy link
Copy Markdown

65db493 ("version: preserve semantic version separators", #2578) added periods to semanticAlphabet in the root version.go, because SemVer separates pre-release identifiers with periods and the old filter silently collapsed beta.rc1 into betarc1.

cmd/btcctl carries its own near-identical copy of that file, and it was not updated. The two now disagree:

version.go:14            ...uvwxyz-."
cmd/btcctl/version.go:14 ...uvwxyz-"

So btcctl still drops the separator. The upstream test, run unchanged against the btcctl copy, fails:

--- FAIL: TestNormalizeVerString
    version_test.go:16: unexpected normalized version: got "betarc1", want "beta.rc1"

This is reachable rather than latent. normalizeVerString is applied to appBuild, which the comment above it documents as settable at build time:

// appBuild is defined as a variable so it can be overridden during the build
// process with '-ldflags "-X main.appBuild foo' if needed.

SemVer build metadata is dot-separated (exp.sha.5114f85 is an example from the spec itself), so a btcctl built with -ldflags "-X main.appBuild=exp.sha.5114f85" reports 0.23.1-beta+expsha5114f85 — the dots stripped and the metadata no longer round-trips. btcd built the same way reports it correctly.

The change is the one-character fix from 65db493 applied to the btcctl copy, plus that commit's test mirrored into cmd/btcctl, with a build metadata case added for the appBuild path.

go build ./..., go test ./cmd/..., gofmt and go vet are all clean.

I left the stale appMinor/appPatch in the btcctl copy alone (0.23.1 there vs 0.26.2 at the root) since bumping those is a release decision, not part of this fix.


Disclosure: written with AI assistance; the analysis, fix and test were verified locally.

The root version.go gained periods in semanticAlphabet in 65db493, but
cmd/btcctl carries its own copy of that code and was left behind, so
btcctl still collapses beta.rc1 into betarc1.

appBuild is settable at build time via '-ldflags "-X main.appBuild ..."'
and SemVer build metadata is dot-separated, so a build stamped with
exp.sha.5114f85 reports expsha5114f85.

Mirror the upstream test, adding a build metadata case.
Every case asserted normalizeVerString(x) == x, so an identity function --
that is, dropping the filter entirely -- satisfied the whole table. Make it
table-driven with explicit in/want and add a case whose input contains a
character outside the semantic alphabet, so the test fails if the filter
stops filtering.

Verified by neutralising the alphabet check in place: the new case fails
with got "beta!rc1", want "betarc1", while the dotted cases still fail if
'.' is removed from the alphabet.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@pucedoteth

Copy link
Copy Markdown
Author

Pushed a test-only follow-up.

The original table asserted normalizeVerString(x) == x for every case, which an identity function satisfies — so it could not have failed if the filter were dropped. (Thanks to @Lrifton92 for spotting this on the equivalent btcwallet PR.) The table is now in/want-based with a case whose input contains a character outside the semantic alphabet.

Verified by neutralising the alphabet check in place (if true || strings.ContainsRune(...), which still compiles): the new case fails with got "beta!rc1", want "betarc1". The dotted cases still fail in the other direction if . is removed from the alphabet, so the test now pins both ends.

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