Fix tapyrus version string#435
Conversation
…VERSION_STRING, FormatFullVersion() and subver string compute version using the same formula. earlier With REVISION=0, BUILD=2 under the old formula (CLIENT_VERSION = MAJOR*1M + MINOR*10k + REVISION*100 + BUILD*1): ┌───────────────────────────────────────────┬────────────────────────┐ │ Field │ Value │ ├───────────────────────────────────────────┼────────────────────────┤ │ cmake project VERSION (MAJOR.MINOR.BUILD) │ 0.7.2 │ ├───────────────────────────────────────────┼────────────────────────┤ │ CLIENT_VERSION_STRING │ 0.7.2 │ ├───────────────────────────────────────────┼────────────────────────┤ │ FormatFullVersion() │ v0.7.2 │ ├───────────────────────────────────────────┼────────────────────────┤ │ subver / getpeerinfo / getnetworkinfo │ /Tapyrus Core:0.7.0.2/ │
f142440 to
dcad7ef
Compare
azuchi
left a comment
There was a problem hiding this comment.
The core fix is sound: dropping CLIENT_VERSION_REVISION and moving CLIENT_VERSION_BUILD to the 100s slot collapses CLIENT_VERSION, CLIENT_VERSION_STRING, project(… VERSION), FormatVersion() and the P2P/RPC subver onto a single MAJOR.MINOR.BUILD formula. The FormatVersion() branch on nVersion % 100 == 0 correctly goes away with it, and the new test_version_config_consistency catches macro-vs-string drift.
Four leftover references to CLIENT_VERSION_REVISION were not updated, and each one silently breaks a full-featured build target. None of them surface in smoke-test.yml because that workflow uses -DBUILD_GUI=OFF (and -DENABLE_TESTS=OFF for Windows cross, which flips BUILD_UTILS off), so the affected files are simply excluded from the fast-test build graph. The daily-test job that would exercise them (L-Cross-W-X86-Rel-GUI+Wallet) was CANCELLED on this run, so CI hasn't observed the failure yet.
🚨 B1 — src/qt/res/tapyrus-qt-res.rc still uses CLIENT_VERSION_REVISION
#define VER_PRODUCTVERSION CLIENT_VERSION_MAJOR,CLIENT_VERSION_MINOR,CLIENT_VERSION_REVISION,CLIENT_VERSION_BUILD
#define VER_PRODUCTVERSION_STR STRINGIZE(CLIENT_VERSION_MAJOR) "." STRINGIZE(CLIENT_VERSION_MINOR) "." STRINGIZE(CLIENT_VERSION_REVISION) "." STRINGIZE(CLIENT_VERSION_BUILD)Wired via src/qt/CMakeLists.txt:273 inside the BUILD_GUI block. After the macro is dropped from tapyrus-build-config.h.in, windres sees a bare identifier where a numeric field is expected in the VERSIONINFO block, and the Windows Qt GUI cross-build fails.
Fix: mirror the change already applied to tapyrus-cli-res.rc — drop CLIENT_VERSION_REVISION and pin the 4th field of VER_PRODUCTVERSION to 0.
🚨 B2 — src/tapyrus-tx-res.rc still uses CLIENT_VERSION_REVISION
Same C-macro pattern as B1. Wired via src/CMakeLists.txt:317 inside BUILD_UTILS. Breaks the Windows tapyrus-tx.exe build whenever BUILD_UTILS is on (default in daily-test).
🚨 B3 — share/qt/Info.plist.in still references @CLIENT_VERSION_REVISION@
Three occurrences at lines 20, 23, 26 (CFBundleGetInfoString, CFBundleShortVersionString, CFBundleVersion). Consumed by add_macos_deploy_target in cmake/module/Maintenance.cmake:78 for the Tapyrus-Qt.app bundle. CMake's configure_file silently substitutes an undefined variable with an empty string, so the resulting plist contains:
<string>0.7., Copyright © 2009-… …</string>
<string>0.7.</string>
<string>0.7.</string>The macOS bundle version metadata is malformed, without any build-time error. Please update to @CLIENT_VERSION_MAJOR@.@CLIENT_VERSION_MINOR@.@CLIENT_VERSION_BUILD@.
🚨 B4 — share/setup.nsi.in still references @CLIENT_VERSION_REVISION@
!define VERSION @CLIENT_VERSION_MAJOR@.@CLIENT_VERSION_MINOR@.@CLIENT_VERSION_REVISION@
Consumed by GenerateSetupNsi.cmake:21 for the NSIS Windows installer. Same empty-string substitution → !define VERSION 0.7. and a malformed installer version. Please update to use @CLIENT_VERSION_BUILD@.
🟡 N1 — CLIENT_VERSION integer bumps from 70001 → 70200
Intentional per the PR description (BUILD 1 → 2), and the P2P subver correspondingly moves from /Tapyrus Core:0.7.0.1/ to /Tapyrus Core:0.7.2/. Worth calling out explicitly in the release notes so external tooling that parses subver knows to expect the new shape.
🟡 N2 — Test comment references a build-directory-specific path
The comment on test_version_config_consistency mentions "cmake_mac/src/tapyrus-config.h on macOS builds". The generated tapyrus-config.h isn't macOS-specific — it lives under whatever the current BASE_BUILD_DIR is. Consider phrasing it as "the generated tapyrus-config.h" to avoid future readers looking for a cmake_mac/ directory that never existed on their local build.
🟡 N3 — Add a functional test for the RPC-surfaced subver
test_version_config_consistency covers the in-process FormatSubVersion() call, but does not exercise the runtime path: init.cpp → strSubVersion → getnetworkinfo RPC. Given this PR is specifically motivated by the daemon's advertised subver diverging from the binary label, an integration-level assertion on getnetworkinfo()["subversion"] is the more meaningful regression guard. The pattern already exists in feature_uacomment.py:21, so a small extension (or a new rpc_getnetworkinfo.py) is cheap.
Suggested shape:
import re
info = self.nodes[0].getnetworkinfo()
# Enforce the new 3-part form — catches any regression to the old 4-part "0.7.0.2".
assert re.match(r'^/Tapyrus Core:\d+\.\d+\.\d+(rc\d+)?( \([^)]+\))?/$', info["subversion"]), \
f"unexpected subversion format: {info['subversion']}"
# Cross-check: the integer `version` field must decompose into the same MAJOR.MINOR.BUILD triple.
v = info["version"]
expected_prefix = f"/Tapyrus Core:{v // 1_000_000}.{(v // 10_000) % 100}.{(v // 100) % 100}"
assert info["subversion"].startswith(expected_prefix), \
f"subversion {info['subversion']!r} does not match version {v}"The regex guards the shape (rejects any drift back to the 4-part 0.7.0.2 form); the second assertion guards the internal consistency of the new slot-100 encoding itself. Not strictly a blocker, but strongly recommended for a version-string fix.
Summary
Please address B1–B4 (all four are silent full-build breakages that CI didn't get to observe). N1–N3 are recommendations. Happy to re-review once the leftover references are cleaned up.
…ION. Make windows rc file unified and build generated rather than having one static file per binary. add functional test to verify version in the rpc as suggested
Remove CLIENT_VERSION_REVISION from the version computation formula to unify all version computations.
With the old formula:
The binary and cmake labels say 0.7.2 but the P2P and RPC version string says 0.7.0.2. The mismatch comes from BUILD sitting in the FormatVersion() adding a 4th component, while cmake's VERSION line uses BUILD directly as the third component and never sees REVISION at all.
Eliminating REVISION and moving BUILD to the 100s slot made both formulas address the same field.