Skip to content

fix(valgrind): install libc6-dbg during valgrind setup #394

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
not-matthias wants to merge 1 commit into
base: main
Choose a base branch
from
+7 −1
Open

fix(valgrind): install libc6-dbg during valgrind setup #394

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion src/executor/valgrind/setup.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,13 @@ pub async fn install_valgrind(
Ok(vec!["valgrind".to_string()])
},
)
.await
.await?;

@GuillaumeLagrange GuillaumeLagrange Jun 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Part of the cache in the action is to avoid having to do sudo apt update when not required, which can take quite a lot of time.

We should cache the output of libc dbg, because it can in practice ad 30 sec to 1 min to ci runs.

apt.rs already exposes the cache helper it should work transparently


// Install libc debug symbols, as Github runners by default do not
// include them. Only install for supported systems.
apt::install(system_info, &["libc6-dbg"])?;
Comment on lines +204 to +206

@greptile-apps greptile-apps Bot Jun 5, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 libc6-dbg installed unconditionally, bypassing the cache gate

apt::install_cached exits early (return Ok(())) when valgrind is already installed, but the new apt::install call for libc6-dbg sits outside that gate and runs on every install_valgrind invocation. This means every CI run where valgrind is cached will still execute apt-get update followed by an install attempt for libc6-dbg — adding the ~10 s overhead even when nothing needs to be installed. Including libc6-dbg inside the install_packages closure (alongside the valgrind .deb) would keep it under the same caching and idempotency logic, or at minimum a dpkg -s libc6-dbg check could guard the call.

Comment on lines +204 to +206

@greptile-apps greptile-apps Bot Jun 5, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 The comment "Only install for supported systems" implies a silent no-op on incompatible systems, but apt::install actually propagates an error (bail!) when the system is incompatible. While install_valgrind is only reached on supported systems today, the comment is misleading about the runtime behaviour.

Suggested change
// Install libc debug symbols, as Github runners by default do not
// include them. Only install for supported systems.
apt::install(system_info, &["libc6-dbg"])?;
// Install libc debug symbols, as GitHub runners do not include them by
// default. `apt::install` will error on unsupported systems, but this
// function is only reachable when `is_codspeed_valgrind_installation_supported`
// is true, so only compatible Linux distros reach this point.
apt::install(system_info, &["libc6-dbg"])?;

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!


Ok(())
}

#[cfg(test)]
Expand Down