Support macos walltime without profiling in codspeed-runner#287
Open
GuillaumeLagrange wants to merge 7 commits intomainfrom
Open
Support macos walltime without profiling in codspeed-runner#287GuillaumeLagrange wants to merge 7 commits intomainfrom
GuillaumeLagrange wants to merge 7 commits intomainfrom
Conversation
On macos a symlink breaks the existing test creating an infinite loop where `instropected_go` detects itself as the real `go` executable. Fixed by changing the behavior to what instrospected_node does.
GuillaumeLagrange
force-pushed
the
cod-2459-be-able-to-run-all-integrations-on-macos
branch
2 times, most recently
from
8a9d9bd to
407c55c
Compare
GuillaumeLagrange
force-pushed
the
cod-2459-be-able-to-run-all-integrations-on-macos
branch
4 times, most recently
from
f95953f to
bba9e90
Compare
Fixes compilation warnings in exec-harness's build script.
GuillaumeLagrange
force-pushed
the
cod-2459-be-able-to-run-all-integrations-on-macos
branch
6 times, most recently
from
c07e0c6 to
dc2a772
Compare
…tor support gates Replaces the loose `os` + `os_version` strings on `SystemInfo` with an `Os` enum. Adds `Executor::support_level(&Os)` so each executor declares its own OS matrix: `run_executor` bails on `Unsupported`, skips `setup()` on `RequiresManualSetup`, and runs it on `FullySupported`.
These tests panic on macOS, and for now are not relevant since we do not support profiling on macOS. When we do support it, we should have os-specific tests
GuillaumeLagrange
force-pushed
the
cod-2459-be-able-to-run-all-integrations-on-macos
branch
from
dc2a772 to
8b7c20c
Compare
GuillaumeLagrange
changed the title
not-matthias
requested changes
Apr 14, 2026
Member
not-matthias
left a comment
not-matthias
left a comment
There was a problem hiding this comment.
It's important to consider everything unsupported by default. e.g. if someone runs on a system we haven't considered (e.g. Windows) we shouldn't even try to continue running with best-effort.
Logic could be implemented like this:
- Tool support: Get
Osstruct, check version + distro - Executor: Check each OS, e.g. on Linux: is_debian FullySupported; else RequireManualInstallation; Mac/Windows: Unsupported
- General (check_system): We could refactor this to just iterate over all executors and see if any are supported.
| #[allow(clippy::enum_variant_names)] | ||
| MacOs { version: String }, | ||
| /// Any other os we don't explicitly target. Executors may still work but the user is responsible for installing tooling themselves. | ||
| Other { name: String, version: String }, |
Member
There was a problem hiding this comment.
Isn't this kind of dangerous? For example, does Windows fall into this category? How about NixOS?
Running on Windows should fail, while NixOS should work. We shouldn't merge both into one category.
Comment on lines
+70
to
+74
| /// Linux distributions that support auto-install of CodSpeed tooling and are explicitly targeted by our executors | ||
| SupportedLinux { | ||
| distro: SupportedLinuxDistro, | ||
| version: String, | ||
| }, |
Member
There was a problem hiding this comment.
How about renaming this to Linux, and including NixOS? We could have a is_debian_based() func on the LinuxDistro struct.
| /// - Ubuntu 22.04 x86_64 and aarch64 | ||
| /// - Debian 11 x86_64 | ||
| /// - Debian 12 x86_64 | ||
| pub fn check_system(system_info: &SystemInfo) -> Result<()> { |
Member
There was a problem hiding this comment.
We could remove this file entirely and create Os::is_supported, or maybe have a SupportLevel (supported, best-effort, unsupported)
Member
There was a problem hiding this comment.
Actually, it might be better to iterate over all executors and see if any are supported.
|
|
||
| fn support_level(&self, os: &Os) -> ExecutorSupport { | ||
| match os { | ||
| Os::SupportedLinux { .. } => ExecutorSupport::FullySupported, |
Member
There was a problem hiding this comment.
Shouldn't we check the version here since we have this download logic:
let (deb_ubuntu_version, architecture) =
match (distro, version.as_str(), system_info.arch.as_str()) {
(SupportedLinuxDistro::Ubuntu, "22.04", "x86_64")
| (SupportedLinuxDistro::Debian, "12", "x86_64") => ("22.04", "amd64"),
(SupportedLinuxDistro::Ubuntu, "24.04", "x86_64") => ("24.04", "amd64"),
(SupportedLinuxDistro::Ubuntu, "22.04", "aarch64")
| (SupportedLinuxDistro::Debian, "12", "aarch64") => ("22.04", "arm64"),
(SupportedLinuxDistro::Ubuntu, "24.04", "aarch64") => ("24.04", "arm64"),
_ => bail!("Unsupported system"),
};| /// Build an [`Os`] from the raw `(os, version)` strings reported by `sysinfo`. | ||
| /// | ||
| /// Unknown Linux-ish distro ids fall into [`Os::OtherLinux`]; only OSes we know we cannot run | ||
| /// on (today: Windows) map to [`Os::Unsupported`]. |
Member
There was a problem hiding this comment.
Os::Unsupported and Os::OtherLinux dont exist
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.
No description provided.