Conversation
`SupportedOs::from_os` read `sysinfo::System::os_version()` before looking at the OS at all and bailed with "Failed to get OS version" when it was absent. That value comes from `VERSION_ID` in `/etc/os-release`, which rolling releases do not ship: on Arch (`ID=archarm`, `BUILD_ID=rolling`) every command aborted before it could determine anything about the host. The version only matters for the distributions we publish packages for, and those all expose one — the rest are already handled by `is_supported()`. So on Linux fall back to `"unknown"` with a `debug!` rather than failing, and keep the hard error on macOS, where the version is always available and is what we report to the API. Refs COD-3072 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VZZqxhWtwHhurnV2hsJ48H
`install_valgrind` went straight to `apt::install_cached`, which resolves the `valgrind-codspeed` deb for the host through `get_codspeed_valgrind_target`. That mapping only covers the Debian/Ubuntu versions we publish packages for, so on anything else — Arch and other rolling releases, non-apt distributions — it bailed with a bare "Unsupported system" and the run stopped at setup. Yet `ValgrindExecutor::support_level` already reports `RequiresManualInstallation` for those hosts, so the executor was advertising a path that setup refused to take. Mirror that support level in the setup: when no package exists for the host, return early if a valgrind installation is already present, and otherwise fail with an error that says CodSpeed publishes nothing for this distribution and points at valgrind-codspeed for a manual install. The libc debug symbol check is already skipped on non-apt systems, so an existing build is enough. Refs COD-3072 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Greptile SummaryThis PR allows Linux distributions without a reported OS version to initialize using an
Confidence Score: 4/5The PR appears safe to merge, with a non-blocking diagnostic gap for manual installations on unsupported Ubuntu or Debian versions. The changed setup branch correctly avoids automatic package installation where no package exists, but its error message cannot guide users whose compatible Valgrind installation fails only because libc debug symbols are missing. Files Needing Attention: src/executor/valgrind/setup.rs
|
| Filename | Overview |
|---|---|
| src/system/os.rs | Linux now falls back to an unknown version while preserving strict macOS detection; no concrete compatibility defect was identified. |
| src/executor/valgrind/setup.rs | Manual installations are now accepted outside the package matrix, but the failure guidance does not mention the libc debug symbols required on apt-compatible hosts. |
Prompt To Fix All With AI
### Issue 1
src/executor/valgrind/setup.rs:260-264
**Manual requirement omitted**
On unsupported Ubuntu or Debian versions, `is_valgrind_installed` also requires resolvable libc debug symbols, but this error only instructs users to install `valgrind-codspeed`. A user who follows that instruction can receive the same error again without learning that the debug-symbol package is the remaining requirement.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "fix(valgrind): accept a manual installat..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Merging this PR will not alter performance
|
On the systems we publish no valgrind package for (rolling releases, non-apt distributions, ...), the setup used to give up immediately and ask for a manual installation. Try a best-effort source build instead: check the build toolchain, clone the sources, compile them and install system-wide, only falling back to the manual instructions when any of those steps fails. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…all path `is_valgrind_installed` answered two questions at once: whether a usable valgrind-codspeed is on PATH, and whether the system libc has separate debug symbols. The second belongs to the apt path alone, where the setup cache restores package files without touching dpkg's database and `dpkg -s libc6-dbg` therefore lies (e5587f4). Everywhere else it was neutralised by an `apt::is_system_compatible` early return. But that early return only covers non-apt systems, so on Ubuntu and Debian the probe leaked into the two callers that install nothing: accepting a manual installation, and verifying a source build. Neither installs `libc6-dbg`, so both were rejected on the very distributions where the check applies. The source-build fallback could therefore never succeed on Ubuntu, and a user who followed the resulting "install it manually" instruction was refused again, for a reason the error never named. Split the function in two and apply the probe at the one call site whose decision it belongs to, the `apt::install_cached` idempotency closure, where `apt::is_system_compatible` holds by construction and the early return was dead weight. Missing symbols now warn rather than block, reusing the wording the perf profiler already has for them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The source-build fallback ran as soon as it could, which spends minutes compiling and then installs system-wide under sudo without the user having agreed to either. Ask first, mirroring the confirmation the walltime executor uses before installing bash: an interactive run gets a `[Y/n]` prompt on stderr, `CODSPEED_VALGRIND_BUILD_FROM_SOURCE` answers it without asking, and a run with no terminal builds anyway, since nobody is there to answer and failing the run outright is the worse outcome. The prompt is wrapped in `suspend_progress_bar` so a spinner cannot overwrite the question, as the sudo password prompt already does. Declining is an outcome, not an error: the three ways this fallback can end without a usable valgrind — declined, failed, built but unusable — now warn and fall through to the same instruction to install manually, instead of being reported as "building it from source failed". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
moha-bekh
changed the title
GuillaumeLagrange
requested changes
Sep 9, 2026
GuillaumeLagrange
left a comment
GuillaumeLagrange
left a comment
Contributor
There was a problem hiding this comment.
olgtm, nothing that big to change!
Rolling releases expose no `VERSION_ID`, and the `"unknown"` placeholder that stood in for it leaked into every consumer of `SupportedOs::version()`: `Display` printed "arch unknown", and nothing in the type said the string could be a stand-in rather than a version. Make the absence part of the type instead. `LinuxDistribution::Other` carries an `Option<String>` and `version()` returns an `Option`, so `Display` drops the version rather than printing a placeholder. `"unknown"` now survives only in `SupportedOsSerde`, where the wire format requires a `String`. A distribution reporting no version falls into `Other` even when its id is `ubuntu` or `debian`. That is correct by construction: every distribution we ship packages for reports a version, so one that does not is not among them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`install_valgrind` re-derived what `ValgrindExecutor::support_level` already knows, deciding for itself whether a package exists for the host before doing anything. Match on the support level in `ValgrindExecutor::setup` instead, and give each branch its own function: `install_valgrind` is the apt path alone, and `try_install_from_source` handles the hosts we publish nothing for. Simplify the source build the same change touches: - clone into a `tempfile::TempDir` rather than a fixed path under the system temporary directory, so no leftover checkout has to be removed by hand and the sources are wiped once the build is done; - on an incomplete toolchain, name the missing executables and stop there. The per-distribution table of install commands was a second list to keep in step with reality, for an edge case that already carries a lot of code; - drop the `is_wanted` env-var tests, which only restated the two-line match they covered. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`codspeed setup` skipped `RequiresManualInstallation` executors outright, so the source-build fallback the valgrind executor now carries was only reachable from `codspeed run`/`exec`. On Arch, the one command a user runs to prepare their machine told them to install the tooling by hand while the runner was willing to build it for them. Run the setup for those executors too, and treat a failure as the executor having tried rather than as fatal: it warns, names the executor and the OS, and the remaining executors are still set up. That distinction matters for the walltime executor, whose setup apt-installs and so fails on a non-apt host — under the old branch it never ran, and running it unguarded would abort the whole command. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Member
Author
|
@GuillaumeLagrange ready for another pass — all threads addressed, and the manual Arch run is done ( |
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
2 participants
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.
Refs COD-3072
The CLI aborted on Arch before doing anything:
SupportedOs::from_osfailed when/etc/os-releasehad noVERSION_ID, which rolling releases do not ship. Fixing that surfaced the next failure at setup, whereinstall_valgrindwent straight to thevalgrind-codspeeddeb we only publish for some Debian/Ubuntu versions.Changes
Optionrather than an error (macOS keeps the hard error, the version is always there and we report it to the API).ValgrindExecutor::setup()matches on its ownsupport_level: deb install, source build, or bail.CODSPEED_VALGRIND_BUILD_FROM_SOURCEskips the prompt), and otherwise says what to install by hand.codspeed setupno longer skipsRequiresManualInstallationexecutors; a failing one warns and the others still run.Blocked on
VALGRIND_CODSPEED_BRANCHpoints at a feature branch until CodSpeedHQ/valgrind-codspeed#40 lands. Merging before that would have released runners cloning it.Test plan
cargo test --lib.VERSION_ID):status,setupandexecin the three modes pass,7c4b2aa^still reproduces the failure, and the no-package path warns and carries on.make,make installunder sudo, thensetup,statusandexecagainst the freshly builtvalgrind-3.26.0.codspeed7.🤖 Generated with Claude Code