Skip to content

Commit e0fe030

Browse files
feat!(cargo-codspeed): allow benchmark filtering by name in cargo codspeed run
There previously was a confusion between benchmark name, aka the name the user gives to individual benchmarks, and benchmark targets. Now, benchmark target filtering is only done through `--workspace`, `--package` and `--exclude` cli flags for both `cargo codspeed build` and `cargo codspeed run`. In addition, `cargo codspeed run` now properly forwards the `benchname` positional argument to the benchmark target, in order to filter benchmarks ran. This is a breaking change, but it makes `cargo-codspeed` more in line with `cargo bench`
1 parent 8f962df commit e0fe030

File tree

3 files changed

+81
-102
lines changed

3 files changed

+81
-102
lines changed

crates/cargo-codspeed/src/app.rs

Lines changed: 23 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -21,58 +21,55 @@ struct Cli {
2121
command: Commands,
2222
}
2323

24+
const PACKAGE_HELP: &str = "Package Selection";
2425
#[derive(Args)]
2526
pub(crate) struct PackageFilters {
2627
/// Select all packages in the workspace
27-
#[arg(long)]
28+
#[arg(long, help_heading = PACKAGE_HELP)]
2829
pub(crate) workspace: bool,
29-
/// Exclude packages
30-
#[arg(long)]
30+
/// Package to exclude
31+
#[arg(long, value_name = "SPEC", help_heading = PACKAGE_HELP)]
3132
pub(crate) exclude: Vec<String>,
32-
/// Package to select (builds all workspace package by default)
33-
#[arg(short, long)]
33+
/// Package to select
34+
#[arg(short, long, value_name= "SPEC", help_heading = PACKAGE_HELP)]
3435
pub(crate) package: Vec<String>,
3536
}
3637

37-
#[derive(Args)]
38-
pub(crate) struct Filters {
39-
/// Optional list of benchmarks to build (builds all benchmarks by default)
40-
pub(crate) bench: Option<Vec<String>>,
41-
#[command(flatten)]
42-
pub(crate) package: PackageFilters,
43-
}
44-
38+
const FEATURE_HELP: &str = "Feature Selection";
39+
const COMPILATION_HELP: &str = "Compilation Options";
4540
#[derive(Subcommand)]
4641
enum Commands {
4742
/// Build the benchmarks
4843
Build {
4944
#[command(flatten)]
50-
filters: Filters,
45+
package_filters: PackageFilters,
5146

5247
/// Space or comma separated list of features to activate
53-
#[arg(short = 'F', long)]
48+
#[arg(short = 'F', long, help_heading = FEATURE_HELP)]
5449
features: Option<String>,
5550

5651
/// Activate all available features of all selected packages.
57-
#[arg(long)]
52+
#[arg(long, help_heading = FEATURE_HELP)]
5853
all_features: bool,
5954

6055
/// Do not activate the `default` feature of the selected packages.
61-
#[arg(long)]
56+
#[arg(long, help_heading = FEATURE_HELP)]
6257
no_default_features: bool,
6358

6459
/// Number of parallel jobs, defaults to # of CPUs.
65-
#[arg(short, long)]
60+
#[arg(short, long, help_heading = COMPILATION_HELP)]
6661
jobs: Option<u32>,
6762

6863
/// Build the benchmarks with the specified profile
69-
#[arg(long, default_value = "bench")]
64+
#[arg(long, default_value = "bench", help_heading = COMPILATION_HELP)]
7065
profile: String,
7166
},
7267
/// Run the previously built benchmarks
7368
Run {
69+
/// If specified, only run benches containing this string in their names
70+
bench_name: Option<String>,
7471
#[command(flatten)]
75-
filters: Filters,
72+
package_filters: PackageFilters,
7673
},
7774
}
7875

@@ -85,7 +82,7 @@ pub fn run(args: impl Iterator<Item = OsString>) -> Result<()> {
8582

8683
let res = match cli.command {
8784
Commands::Build {
88-
filters,
85+
package_filters,
8986
features,
9087
all_features,
9188
jobs,
@@ -113,15 +110,18 @@ pub fn run(args: impl Iterator<Item = OsString>) -> Result<()> {
113110
features.map(|f| f.split([' ', ',']).map(|s| s.to_string()).collect_vec());
114111
build_benches(
115112
&metadata,
116-
filters,
113+
package_filters,
117114
features,
118115
profile,
119116
cli.quiet,
120117
measurement_mode,
121118
passthrough_flags,
122119
)
123120
}
124-
Commands::Run { filters } => run_benches(&metadata, filters, measurement_mode),
121+
Commands::Run {
122+
bench_name,
123+
package_filters,
124+
} => run_benches(&metadata, bench_name, package_filters, measurement_mode),
125125
};
126126

127127
if let Err(e) = res {

crates/cargo-codspeed/src/build.rs

Lines changed: 21 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
use crate::{
2-
app::{Filters, PackageFilters},
2+
app::PackageFilters,
33
helpers::{clear_dir, get_codspeed_target_dir},
44
measurement_mode::MeasurementMode,
55
prelude::*,
@@ -8,7 +8,7 @@ use cargo_metadata::{camino::Utf8PathBuf, Message, Metadata, TargetKind};
88
use std::process::{exit, Command, Stdio};
99

1010
struct BuildOptions<'a> {
11-
filters: Filters,
11+
package_filters: PackageFilters,
1212
features: &'a Option<Vec<String>>,
1313
profile: &'a str,
1414
passthrough_flags: &'a Vec<String>,
@@ -49,6 +49,19 @@ impl BuildOptions<'_> {
4949
);
5050

5151
let mut built_benches = Vec::new();
52+
53+
let package_names = self
54+
.package_filters
55+
.packages_from_flags(metadata)
56+
.map_err(|e| {
57+
// Avoid leaving an orphan cargo process, even if something went wrong
58+
cargo.wait().expect("Could not get cargo's exist status");
59+
e
60+
})?
61+
.into_iter()
62+
.map(|t| t.name.clone())
63+
.collect::<Vec<_>>();
64+
5265
for message in Message::parse_stream(reader) {
5366
match message.expect("Failed to parse message") {
5467
// Those messages will include build errors and warnings even if stderr also contain some of them
@@ -66,19 +79,14 @@ impl BuildOptions<'_> {
6679
.find(|p| p.id == artifact.package_id)
6780
.expect("Could not find package");
6881

69-
let bench_name = artifact.target.name;
82+
let bench_target_name = artifact.target.name;
7083

71-
let add_bench_to_codspeed_dir = match &self.filters.bench {
72-
Some(allowed_bench_names) => allowed_bench_names
73-
.iter()
74-
.any(|allowed_bench_name| bench_name.contains(allowed_bench_name)),
75-
None => true,
76-
};
84+
let add_bench_to_codspeed_dir = package_names.iter().contains(&package.name);
7785

7886
if add_bench_to_codspeed_dir {
7987
built_benches.push(BuiltBench {
8088
package: package.name.clone(),
81-
bench: bench_name,
89+
bench: bench_target_name,
8290
executable_path: artifact
8391
.executable
8492
.expect("Unexpected missing executable path"),
@@ -171,7 +179,7 @@ impl BuildOptions<'_> {
171179

172180
cargo.arg("--profile").arg(self.profile);
173181

174-
self.filters.package.add_cargo_args(&mut cargo);
182+
self.package_filters.add_cargo_args(&mut cargo);
175183

176184
cargo
177185
}
@@ -199,15 +207,15 @@ impl PackageFilters {
199207

200208
pub fn build_benches(
201209
metadata: &Metadata,
202-
filters: Filters,
210+
package_filters: PackageFilters,
203211
features: Option<Vec<String>>,
204212
profile: String,
205213
quiet: bool,
206214
measurement_mode: MeasurementMode,
207215
passthrough_flags: Vec<String>,
208216
) -> Result<()> {
209217
let built_benches = BuildOptions {
210-
filters,
218+
package_filters,
211219
features: &features,
212220
profile: &profile,
213221
passthrough_flags: &passthrough_flags,

crates/cargo-codspeed/src/run.rs

Lines changed: 37 additions & 66 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,5 @@
11
use crate::{
2-
app::{Filters, PackageFilters},
3-
helpers::get_codspeed_target_dir,
4-
measurement_mode::MeasurementMode,
2+
app::PackageFilters, helpers::get_codspeed_target_dir, measurement_mode::MeasurementMode,
53
prelude::*,
64
};
75
use anyhow::Context;
@@ -22,13 +20,37 @@ struct BenchToRun {
2220
package_name: String,
2321
}
2422

25-
impl Filters {
23+
impl PackageFilters {
24+
/// Logic taken from [cargo::ops::Packages](https://docs.rs/cargo/0.85.0/src/cargo/ops/cargo_compile/packages.rs.html#34-42)
25+
pub(crate) fn packages_from_flags<'a>(
26+
&self,
27+
metadata: &'a Metadata,
28+
) -> Result<Vec<&'a Package>> {
29+
Ok(
30+
match (self.workspace, self.exclude.len(), self.package.len()) {
31+
(false, 0, 0) => metadata.workspace_default_packages(),
32+
(false, 0, _) => metadata
33+
.workspace_packages()
34+
.into_iter()
35+
.filter(|p| self.package.contains(&p.name))
36+
.collect(),
37+
(false, _, _) => bail!("--exclude can only be used with --workspace"),
38+
(true, 0, _) => metadata.workspace_packages(),
39+
(true, _, _) => metadata
40+
.workspace_packages()
41+
.into_iter()
42+
.filter(|p| !self.exclude.contains(&p.name))
43+
.collect(),
44+
},
45+
)
46+
}
47+
2648
fn benches_to_run(
2749
&self,
2850
codspeed_target_dir: PathBuf,
2951
metadata: &Metadata,
3052
) -> Result<Vec<BenchToRun>> {
31-
let packages = self.package.packages(metadata)?;
53+
let packages = self.packages_from_flags(metadata)?;
3254

3355
let mut benches = vec![];
3456
for package in packages {
@@ -63,80 +85,25 @@ impl Filters {
6385
}
6486
}
6587

66-
impl PackageFilters {
67-
/// Logic taken from [cargo::ops::Packages](https://docs.rs/cargo/0.85.0/src/cargo/ops/cargo_compile/packages.rs.html#34-42)
68-
fn packages<'a>(&self, metadata: &'a Metadata) -> Result<Vec<&'a Package>> {
69-
Ok(
70-
match (self.workspace, self.exclude.len(), self.package.len()) {
71-
(false, 0, 0) => metadata.workspace_default_packages(),
72-
(false, 0, _) => metadata
73-
.workspace_packages()
74-
.into_iter()
75-
.filter(|p| {
76-
self.package
77-
.iter()
78-
.any(|allowed_package| p.name.contains(allowed_package))
79-
})
80-
.collect(),
81-
(false, _, _) => bail!("--exclude can only be used with --workspace"),
82-
(true, 0, _) => metadata.workspace_packages(),
83-
(true, _, _) => metadata
84-
.workspace_packages()
85-
.into_iter()
86-
.filter(|p| {
87-
!self
88-
.exclude
89-
.iter()
90-
.any(|denied_package| p.name.contains(denied_package))
91-
})
92-
.collect(),
93-
},
94-
)
95-
}
96-
}
97-
9888
pub fn run_benches(
9989
metadata: &Metadata,
100-
filters: Filters,
90+
bench_name_filter: Option<String>,
91+
package_filters: PackageFilters,
10192
measurement_mode: MeasurementMode,
10293
) -> Result<()> {
10394
let codspeed_target_dir = get_codspeed_target_dir(metadata, measurement_mode);
10495
let workspace_root = metadata.workspace_root.as_std_path();
10596
if measurement_mode == MeasurementMode::Walltime {
10697
WalltimeResults::clear(workspace_root)?;
10798
}
108-
let benches = filters.benches_to_run(codspeed_target_dir, metadata)?;
99+
let benches = package_filters.benches_to_run(codspeed_target_dir, metadata)?;
109100
if benches.is_empty() {
110101
bail!("No benchmarks found. Run `cargo codspeed build` first.");
111102
}
112103

113-
let mut to_run = vec![];
114-
if let Some(allowed_bench_names) = filters.bench {
115-
// Make sure all benchmarks are found
116-
let mut not_found = vec![];
117-
for allowed_bench_name in allowed_bench_names.iter() {
118-
let bench = benches
119-
.iter()
120-
.find(|b| b.bench_name.contains(allowed_bench_name));
121-
122-
if let Some(bench) = bench {
123-
to_run.push(bench);
124-
} else {
125-
not_found.push(allowed_bench_name);
126-
}
127-
}
104+
eprintln!("Collected {} benchmark suite(s) to run", benches.len());
128105

129-
if !not_found.is_empty() {
130-
bail!(
131-
"The following benchmarks to run were not found: {}",
132-
not_found.iter().join(", ")
133-
);
134-
}
135-
} else {
136-
to_run = benches.iter().collect();
137-
}
138-
eprintln!("Collected {} benchmark suite(s) to run", to_run.len());
139-
for bench in to_run.iter() {
106+
for bench in benches.iter() {
140107
let bench_name = &bench.bench_name;
141108
// workspace_root is needed since file! returns the path relatively to the workspace root
142109
// while CARGO_MANIFEST_DIR returns the path to the sub package
@@ -151,6 +118,10 @@ pub fn run_benches(
151118
command.arg("--bench"); // Walltime targets need this additional argument (inherited from running them with `cargo bench`)
152119
}
153120

121+
if let Some(bench_name_filter) = bench_name_filter.as_ref() {
122+
command.arg(bench_name_filter);
123+
}
124+
154125
command
155126
.status()
156127
.map_err(|e| anyhow!("failed to execute the benchmark process: {}", e))
@@ -177,7 +148,7 @@ pub fn run_benches(
177148
})?;
178149
eprintln!("Done running {bench_name}");
179150
}
180-
eprintln!("Finished running {} benchmark suite(s)", to_run.len());
151+
eprintln!("Finished running {} benchmark suite(s)", benches.len());
181152

182153
if measurement_mode == MeasurementMode::Walltime {
183154
aggregate_raw_walltime_data(workspace_root)?;

0 commit comments

Comments
 (0)