Skip to content

fix: valgrind-codspeed version comparison#390

Merged
not-matthias merged 3 commits into
mainfrom
cod-2782-properly-compare-codspeed-valgrind-versions-for-the-same
Jun 4, 2026
Merged

fix: valgrind-codspeed version comparison#390
not-matthias merged 3 commits into
mainfrom
cod-2782-properly-compare-codspeed-valgrind-versions-for-the-same

Conversation

@not-matthias
Copy link
Copy Markdown
Member

No description provided.

Return a ValgrindVersion { semver, codspeed_iteration } from
parse_valgrind_codspeed_version so the runner can distinguish between
codspeed iterations of the same upstream valgrind version (e.g.
3.26.0.codspeed1 vs 3.26.0.codspeed2). The legacy `.codspeed` form
(no iteration) is still accepted and yields codspeed_iteration: None.

Refs COD-2674.
@not-matthias not-matthias marked this pull request as ready for review June 4, 2026 15:12
@codspeed-hq
Copy link
Copy Markdown

codspeed-hq Bot commented Jun 4, 2026

Merging this PR will not alter performance

⚠️ Unknown Walltime execution environment detected

Using the Walltime instrument on standard Hosted Runners will lead to inconsistent data.

For the most accurate results, we recommend using CodSpeed Macro Runners: bare-metal machines fine-tuned for performance measurement consistency.

✅ 7 untouched benchmarks


Comparing cod-2782-properly-compare-codspeed-valgrind-versions-for-the-same (f8f7496) with main (873caf0)

Open in CodSpeed

@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented Jun 4, 2026

Greptile Summary

This PR fixes a version comparison bug where a stale CodSpeed-repackaged valgrind (same upstream semver but an older .codspeedN iteration) was incorrectly accepted as up-to-date. It introduces VALGRIND_CODSPEED_ITERATION as the single source of truth and plumbs it through the version string, the .deb rev suffix, and the installed-version check.

  • VALGRIND_CODSPEED_ITERATION is added to binary_pins.rs; VALGRIND_DEB_REV and VALGRIND_CODSPEED_VERSION_STRING are derived from it rather than being hardcoded, so a future repackaging only requires editing one constant.
  • parse_valgrind_codspeed_version now returns a ValgrindVersion { semver, codspeed_iteration } struct; classify_valgrind_version is extracted as a standalone, testable function that does a lexicographic tuple comparison (semver, iteration) against the pinned values — correctly rejecting older iterations at the same upstream semver.
  • Nine new unit tests cover the exact failure modes: stale iteration, legacy suffix, pinned iteration, newer iteration, and newer semver with legacy suffix.

Confidence Score: 5/5

Safe to merge — the change is a targeted, well-tested fix to the version comparison path with no side-effects on install or download logic.

The iteration constant is derived consistently across all three places it appears (deb rev, version string, comparison), the tuple comparison is lexicographically correct for the intended ordering, and nine new tests exercise every relevant boundary.

No files require special attention.

Important Files Changed

Filename Overview
src/binary_pins.rs Extracts the repackaging iteration into VALGRIND_CODSPEED_ITERATION and derives both VALGRIND_DEB_REV and VALGRIND_CODSPEED_VERSION_STRING from it, so bumping the iteration is now a single-number edit.
src/executor/valgrind/setup.rs Adds ValgrindVersion struct, refactors the parser to return semver + optional iteration, and fixes the comparison bug by using a tuple (semver, iteration) against the pinned constants. Extracts classify_valgrind_version for testability and adds comprehensive tests covering all cases.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["valgrind --version stdout"] --> B["trim to version string"]
    B --> C{contains .codspeed?}
    C -- No --> D["IncorrectVersion: not a CodSpeed build"]
    C -- Yes --> E["parse_valgrind_codspeed_version()"]
    E --> F["strip valgrind- prefix, split_once on .codspeed"]
    F -- parse fails --> G["IncorrectVersion: could not parse version"]
    F -- success --> H["ValgrindVersion with semver and codspeed_iteration"]
    H --> I["installed_iteration = codspeed_iteration.unwrap_or(0)"]
    I --> J{"semver, installed_iteration less than PINNED_VERSION, PINNED_ITERATION?"}
    J -- Yes --> K["IncorrectVersion: version too old"]
    J -- No --> L["Installed"]
Loading

Reviews (2): Last reviewed commit: "chore(valgrind): log detected valgrind v..." | Re-trigger Greptile

Comment thread src/executor/valgrind/setup.rs Outdated
not-matthias and others added 2 commits June 4, 2026 17:17
Previously only the upstream semver was compared, so a cached
3.26.0.codspeed2 build still satisfied the 3.26.0-0codspeed3 pin and
stale valgrind builds were restored from the CI cache instead of being
reinstalled. Compare the (semver, iteration) pair against the pinned
values, treating legacy `.codspeed` builds without an iteration as
iteration 0. The pinned iteration is a single constant that also forms
the .deb revision, so future bumps stay in one place.

Fixes COD-2782
Co-Authored-By: Claude <noreply@anthropic.com>
Print the installed valgrind version next to the expected pin at debug
level so stale-cache issues are visible in CI logs.
Copy link
Copy Markdown
Contributor

@GuillaumeLagrange GuillaumeLagrange left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@not-matthias not-matthias force-pushed the cod-2782-properly-compare-codspeed-valgrind-versions-for-the-same branch from 5a6af7a to f8f7496 Compare June 4, 2026 15:20
@not-matthias not-matthias merged commit f8f7496 into main Jun 4, 2026
21 checks passed
@not-matthias not-matthias deleted the cod-2782-properly-compare-codspeed-valgrind-versions-for-the-same branch June 4, 2026 15:55
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.

2 participants