From a00961269107703772c4e8f071f0accbe0f1a7e5 Mon Sep 17 00:00:00 2001 From: Stypox Date: Wed, 9 Apr 2025 10:34:03 +0200 Subject: [PATCH] Allow building Miri with --features from miri-script Otherwise there was no way to pass e.g. `--features tracing` just to the `cargo` commands issued on the root repository: CARGO_EXTRA_FLAGS applies the flags to the "cargo-miri" crate, too, which does not make sense for crate-specific features. Fix install_to_sysroot doing path concatenation twice. Since the second concatenation was against an absolute path, it didn't do anything. This also makes the interface of install_to_sysroot() similar to that of cargo_cmd(). Implement --features for clippy, also fix not passing features to one of the cargo invocations for test --- src/tools/miri/miri-script/src/commands.rs | 83 +++++++++++--------- src/tools/miri/miri-script/src/coverage.rs | 7 +- src/tools/miri/miri-script/src/main.rs | 38 +++++++-- src/tools/miri/miri-script/src/util.rs | 90 +++++++++++++++++----- 4 files changed, 153 insertions(+), 65 deletions(-) diff --git a/src/tools/miri/miri-script/src/commands.rs b/src/tools/miri/miri-script/src/commands.rs index 86362145d475..d18e9c7791fc 100644 --- a/src/tools/miri/miri-script/src/commands.rs +++ b/src/tools/miri/miri-script/src/commands.rs @@ -32,6 +32,7 @@ impl MiriEnv { &mut self, quiet: bool, target: Option>, + features: &[String], ) -> Result { if let Some(miri_sysroot) = self.sh.var_os("MIRI_SYSROOT") { // Sysroot already set, use that. @@ -39,8 +40,8 @@ impl MiriEnv { } // Make sure everything is built. Also Miri itself. - self.build(".", &[], quiet)?; - self.build("cargo-miri", &[], quiet)?; + self.build(".", features, &[], quiet)?; + self.build("cargo-miri", &[], &[], quiet)?; let target_flag = if let Some(target) = &target { vec![OsStr::new("--target"), target.as_ref()] @@ -58,7 +59,7 @@ impl MiriEnv { } let mut cmd = self - .cargo_cmd("cargo-miri", "run") + .cargo_cmd("cargo-miri", "run", &[]) .arg("--quiet") .arg("--") .args(&["miri", "setup", "--print-sysroot"]) @@ -90,7 +91,9 @@ impl Command { Self::fmt(vec![])?; } if auto_clippy { - Self::clippy(vec![])?; + // no features for auto actions, see + // https://github.com/rust-lang/miri/pull/4396#discussion_r2149654845 + Self::clippy(vec![], vec![])?; } Ok(()) @@ -175,16 +178,16 @@ impl Command { } // Then run the actual command. match self { - Command::Install { flags } => Self::install(flags), - Command::Build { flags } => Self::build(flags), - Command::Check { flags } => Self::check(flags), - Command::Test { bless, flags, target, coverage } => - Self::test(bless, flags, target, coverage), - Command::Run { dep, verbose, target, edition, flags } => - Self::run(dep, verbose, target, edition, flags), - Command::Doc { flags } => Self::doc(flags), + Command::Install { features, flags } => Self::install(features, flags), + Command::Build { features, flags } => Self::build(features, flags), + Command::Check { features, flags } => Self::check(features, flags), + Command::Test { bless, target, coverage, features, flags } => + Self::test(bless, target, coverage, features, flags), + Command::Run { dep, verbose, target, edition, features, flags } => + Self::run(dep, verbose, target, edition, features, flags), + Command::Doc { features, flags } => Self::doc(features, flags), Command::Fmt { flags } => Self::fmt(flags), - Command::Clippy { flags } => Self::clippy(flags), + Command::Clippy { features, flags } => Self::clippy(features, flags), Command::Bench { target, no_install, save_baseline, load_baseline, benches } => Self::bench(target, no_install, save_baseline, load_baseline, benches), Command::Toolchain { flags } => Self::toolchain(flags), @@ -494,7 +497,7 @@ impl Command { if !no_install { // Make sure we have an up-to-date Miri installed and selected the right toolchain. - Self::install(vec![])?; + Self::install(vec![], vec![])?; } let results_json_dir = if save_baseline.is_some() || load_baseline.is_some() { Some(TempDir::new()?) @@ -601,47 +604,48 @@ impl Command { Ok(()) } - fn install(flags: Vec) -> Result<()> { + fn install(features: Vec, flags: Vec) -> Result<()> { let e = MiriEnv::new()?; - e.install_to_sysroot(e.miri_dir.clone(), &flags)?; - e.install_to_sysroot(path!(e.miri_dir / "cargo-miri"), &flags)?; + e.install_to_sysroot(".", &features, &flags)?; + e.install_to_sysroot("cargo-miri", &[], &flags)?; Ok(()) } - fn build(flags: Vec) -> Result<()> { + fn build(features: Vec, flags: Vec) -> Result<()> { let e = MiriEnv::new()?; - e.build(".", &flags, /* quiet */ false)?; - e.build("cargo-miri", &flags, /* quiet */ false)?; + e.build(".", &features, &flags, /* quiet */ false)?; + e.build("cargo-miri", &[], &flags, /* quiet */ false)?; Ok(()) } - fn check(flags: Vec) -> Result<()> { + fn check(features: Vec, flags: Vec) -> Result<()> { let e = MiriEnv::new()?; - e.check(".", &flags)?; - e.check("cargo-miri", &flags)?; + e.check(".", &features, &flags)?; + e.check("cargo-miri", &[], &flags)?; Ok(()) } - fn doc(flags: Vec) -> Result<()> { + fn doc(features: Vec, flags: Vec) -> Result<()> { let e = MiriEnv::new()?; - e.doc(".", &flags)?; - e.doc("cargo-miri", &flags)?; + e.doc(".", &features, &flags)?; + e.doc("cargo-miri", &[], &flags)?; Ok(()) } - fn clippy(flags: Vec) -> Result<()> { + fn clippy(features: Vec, flags: Vec) -> Result<()> { let e = MiriEnv::new()?; - e.clippy(".", &flags)?; - e.clippy("cargo-miri", &flags)?; - e.clippy("miri-script", &flags)?; + e.clippy(".", &features, &flags)?; + e.clippy("cargo-miri", &[], &flags)?; + e.clippy("miri-script", &[], &flags)?; Ok(()) } fn test( bless: bool, - mut flags: Vec, target: Option, coverage: bool, + features: Vec, + mut flags: Vec, ) -> Result<()> { let mut e = MiriEnv::new()?; @@ -652,7 +656,7 @@ impl Command { } // Prepare a sysroot. (Also builds cargo-miri, which we need.) - e.build_miri_sysroot(/* quiet */ false, target.as_deref())?; + e.build_miri_sysroot(/* quiet */ false, target.as_deref(), &features)?; // Forward information to test harness. if bless { @@ -672,10 +676,10 @@ impl Command { // Then test, and let caller control flags. // Only in root project as `cargo-miri` has no tests. - e.test(".", &flags)?; + e.test(".", &features, &flags)?; if let Some(coverage) = &coverage { - coverage.show_coverage_report(&e)?; + coverage.show_coverage_report(&e, &features)?; } Ok(()) @@ -686,14 +690,17 @@ impl Command { verbose: bool, target: Option, edition: Option, + features: Vec, flags: Vec, ) -> Result<()> { let mut e = MiriEnv::new()?; // Preparation: get a sysroot, and get the miri binary. - let miri_sysroot = e.build_miri_sysroot(/* quiet */ !verbose, target.as_deref())?; - let miri_bin = - e.build_get_binary(".").context("failed to get filename of miri executable")?; + let miri_sysroot = + e.build_miri_sysroot(/* quiet */ !verbose, target.as_deref(), &features)?; + let miri_bin = e + .build_get_binary(".", &features) + .context("failed to get filename of miri executable")?; // More flags that we will pass before `flags` // (because `flags` may contain `--`). @@ -718,7 +725,7 @@ impl Command { // The basic command that executes the Miri driver. let mut cmd = if dep { // We invoke the test suite as that has all the logic for running with dependencies. - e.cargo_cmd(".", "test") + e.cargo_cmd(".", "test", &features) .args(&["--test", "ui"]) .args(quiet_flag) .arg("--") diff --git a/src/tools/miri/miri-script/src/coverage.rs b/src/tools/miri/miri-script/src/coverage.rs index 8cafcea0d16f..cdf2bbb93304 100644 --- a/src/tools/miri/miri-script/src/coverage.rs +++ b/src/tools/miri/miri-script/src/coverage.rs @@ -49,7 +49,7 @@ impl CoverageReport { /// show_coverage_report will print coverage information using the artifact /// files in `self.path`. - pub fn show_coverage_report(&self, e: &MiriEnv) -> Result<()> { + pub fn show_coverage_report(&self, e: &MiriEnv, features: &[String]) -> Result<()> { let profraw_files = self.profraw_files()?; let profdata_bin = path!(e.libdir / ".." / "bin" / "llvm-profdata"); @@ -63,8 +63,9 @@ impl CoverageReport { // Create the coverage report. let cov_bin = path!(e.libdir / ".." / "bin" / "llvm-cov"); - let miri_bin = - e.build_get_binary(".").context("failed to get filename of miri executable")?; + let miri_bin = e + .build_get_binary(".", features) + .context("failed to get filename of miri executable")?; cmd!( e.sh, "{cov_bin} report --instr-profile={merged_file} --object {miri_bin} --sources src/" diff --git a/src/tools/miri/miri-script/src/main.rs b/src/tools/miri/miri-script/src/main.rs index 6aab2f79bd78..673d658cf1d5 100644 --- a/src/tools/miri/miri-script/src/main.rs +++ b/src/tools/miri/miri-script/src/main.rs @@ -14,24 +14,40 @@ pub enum Command { /// Sets up the rpath such that the installed binary should work in any /// working directory. Install { + /// Pass features to cargo invocations on the "miri" crate in the root. This option does + /// **not** apply to other crates, so e.g. these features won't be used on "cargo-miri". + #[arg(long, value_delimiter = ',', action = clap::ArgAction::Append)] + features: Vec, /// Flags that are passed through to `cargo install`. #[arg(trailing_var_arg = true, allow_hyphen_values = true)] flags: Vec, }, /// Build Miri. Build { + /// Pass features to cargo invocations on the "miri" crate in the root. This option does + /// **not** apply to other crates, so e.g. these features won't be used on "cargo-miri". + #[arg(long, value_delimiter = ',', action = clap::ArgAction::Append)] + features: Vec, /// Flags that are passed through to `cargo build`. #[arg(trailing_var_arg = true, allow_hyphen_values = true)] flags: Vec, }, /// Check Miri. Check { + /// Pass features to cargo invocations on the "miri" crate in the root. This option does + /// **not** apply to other crates, so e.g. these features won't be used on "cargo-miri". + #[arg(long, value_delimiter = ',', action = clap::ArgAction::Append)] + features: Vec, /// Flags that are passed through to `cargo check`. #[arg(trailing_var_arg = true, allow_hyphen_values = true)] flags: Vec, }, /// Check Miri with Clippy. Clippy { + /// Pass features to cargo invocations on the "miri" crate in the root. This option does + /// **not** apply to other crates, so e.g. these features won't be used on "cargo-miri". + #[arg(long, value_delimiter = ',', action = clap::ArgAction::Append)] + features: Vec, /// Flags that are passed through to `cargo clippy`. #[arg(trailing_var_arg = true, allow_hyphen_values = true)] flags: Vec, @@ -47,6 +63,10 @@ pub enum Command { /// Produce coverage report. #[arg(long)] coverage: bool, + /// Pass features to cargo invocations on the "miri" crate in the root. This option does + /// **not** apply to other crates, so e.g. these features won't be used on "cargo-miri". + #[arg(long, value_delimiter = ',', action = clap::ArgAction::Append)] + features: Vec, /// Flags that are passed through to the test harness. #[arg(trailing_var_arg = true, allow_hyphen_values = true)] flags: Vec, @@ -67,6 +87,10 @@ pub enum Command { /// The Rust edition. #[arg(long)] edition: Option, + /// Pass features to cargo invocations on the "miri" crate in the root. This option does + /// **not** apply to other crates, so e.g. these features won't be used on "cargo-miri". + #[arg(long, value_delimiter = ',', action = clap::ArgAction::Append)] + features: Vec, /// Flags that are passed through to `miri`. /// /// The flags set in `MIRIFLAGS` are added in front of these flags. @@ -75,6 +99,10 @@ pub enum Command { }, /// Build documentation. Doc { + /// Pass features to cargo invocations on the "miri" crate in the root. This option does + /// **not** apply to other crates, so e.g. these features won't be used on "cargo-miri". + #[arg(long, value_delimiter = ',', action = clap::ArgAction::Append)] + features: Vec, /// Flags that are passed through to `cargo doc`. #[arg(trailing_var_arg = true, allow_hyphen_values = true)] flags: Vec, @@ -144,13 +172,13 @@ impl Command { } match self { - Self::Install { flags } - | Self::Build { flags } - | Self::Check { flags } - | Self::Doc { flags } + Self::Install { flags, .. } + | Self::Build { flags, .. } + | Self::Check { flags, .. } + | Self::Doc { flags, .. } | Self::Fmt { flags } | Self::Toolchain { flags } - | Self::Clippy { flags } + | Self::Clippy { flags, .. } | Self::Run { flags, .. } | Self::Test { flags, .. } => { flags.extend(remainder); diff --git a/src/tools/miri/miri-script/src/util.rs b/src/tools/miri/miri-script/src/util.rs index 5c2a055990fd..c100cf195ba4 100644 --- a/src/tools/miri/miri-script/src/util.rs +++ b/src/tools/miri/miri-script/src/util.rs @@ -26,6 +26,12 @@ pub fn flagsplit(flags: &str) -> Vec { flags.split(' ').map(str::trim).filter(|s| !s.is_empty()).map(str::to_string).collect() } +/// Turns a list of features into a list of arguments to pass to cargo invocations. +/// Each feature will go in its own argument, e.g. "--features feat1 --features feat2". +fn features_to_args(features: &[String]) -> impl IntoIterator { + features.iter().flat_map(|feat| ["--features", feat]) +} + /// Some extra state we track for building Miri, such as the right RUSTFLAGS. pub struct MiriEnv { /// miri_dir is the root of the miri repository checkout we are working in. @@ -116,44 +122,70 @@ impl MiriEnv { Ok(MiriEnv { miri_dir, toolchain, sh, sysroot, cargo_extra_flags, libdir }) } - pub fn cargo_cmd(&self, crate_dir: impl AsRef, cmd: &str) -> Cmd<'_> { + /// Make sure the `features` you pass here exist for the specified `crate_dir`. For example, the + /// "--features" parameter of [crate::Command]s is intended only for the "miri" root crate. + pub fn cargo_cmd( + &self, + crate_dir: impl AsRef, + cmd: &str, + features: &[String], + ) -> Cmd<'_> { let MiriEnv { toolchain, cargo_extra_flags, .. } = self; let manifest_path = path!(self.miri_dir / crate_dir.as_ref() / "Cargo.toml"); + let features = features_to_args(features); cmd!( self.sh, - "cargo +{toolchain} {cmd} {cargo_extra_flags...} --manifest-path {manifest_path}" + "cargo +{toolchain} {cmd} {cargo_extra_flags...} --manifest-path {manifest_path} {features...}" ) } + /// Make sure the `features` you pass here exist for the specified `crate_dir`. For example, the + /// "--features" parameter of [crate::Command]s is intended only for the "miri" root crate. pub fn install_to_sysroot( &self, - path: impl AsRef, + crate_dir: impl AsRef, + features: &[String], args: impl IntoIterator>, ) -> Result<()> { let MiriEnv { sysroot, toolchain, cargo_extra_flags, .. } = self; - let path = path!(self.miri_dir / path.as_ref()); + let path = path!(self.miri_dir / crate_dir.as_ref()); + let features = features_to_args(features); // Install binaries to the miri toolchain's `sysroot` so they do not interact with other toolchains. // (Not using `cargo_cmd` as `install` is special and doesn't use `--manifest-path`.) - cmd!(self.sh, "cargo +{toolchain} install {cargo_extra_flags...} --path {path} --force --root {sysroot} {args...}").run()?; + cmd!(self.sh, "cargo +{toolchain} install {cargo_extra_flags...} --path {path} --force --root {sysroot} {features...} {args...}").run()?; Ok(()) } - pub fn build(&self, crate_dir: impl AsRef, args: &[String], quiet: bool) -> Result<()> { + pub fn build( + &self, + crate_dir: impl AsRef, + features: &[String], + args: &[String], + quiet: bool, + ) -> Result<()> { let quiet_flag = if quiet { Some("--quiet") } else { None }; // We build all targets, since building *just* the bin target doesnot include // `dev-dependencies` and that changes feature resolution. This also gets us more // parallelism in `./miri test` as we build Miri and its tests together. - let mut cmd = - self.cargo_cmd(crate_dir, "build").args(&["--all-targets"]).args(quiet_flag).args(args); + let mut cmd = self + .cargo_cmd(crate_dir, "build", features) + .args(&["--all-targets"]) + .args(quiet_flag) + .args(args); cmd.set_quiet(quiet); cmd.run()?; Ok(()) } /// Returns the path to the main crate binary. Assumes that `build` has been called before. - pub fn build_get_binary(&self, crate_dir: impl AsRef) -> Result { - let cmd = - self.cargo_cmd(crate_dir, "build").args(&["--all-targets", "--message-format=json"]); + pub fn build_get_binary( + &self, + crate_dir: impl AsRef, + features: &[String], + ) -> Result { + let cmd = self + .cargo_cmd(crate_dir, "build", features) + .args(&["--all-targets", "--message-format=json"]); let output = cmd.output()?; let mut bin = None; for line in output.stdout.lines() { @@ -174,23 +206,43 @@ impl MiriEnv { bin.ok_or_else(|| anyhow!("found no binary in cargo output")) } - pub fn check(&self, crate_dir: impl AsRef, args: &[String]) -> Result<()> { - self.cargo_cmd(crate_dir, "check").arg("--all-targets").args(args).run()?; + pub fn check( + &self, + crate_dir: impl AsRef, + features: &[String], + args: &[String], + ) -> Result<()> { + self.cargo_cmd(crate_dir, "check", features).arg("--all-targets").args(args).run()?; Ok(()) } - pub fn doc(&self, crate_dir: impl AsRef, args: &[String]) -> Result<()> { - self.cargo_cmd(crate_dir, "doc").args(args).run()?; + pub fn doc( + &self, + crate_dir: impl AsRef, + features: &[String], + args: &[String], + ) -> Result<()> { + self.cargo_cmd(crate_dir, "doc", features).args(args).run()?; Ok(()) } - pub fn clippy(&self, crate_dir: impl AsRef, args: &[String]) -> Result<()> { - self.cargo_cmd(crate_dir, "clippy").arg("--all-targets").args(args).run()?; + pub fn clippy( + &self, + crate_dir: impl AsRef, + features: &[String], + args: &[String], + ) -> Result<()> { + self.cargo_cmd(crate_dir, "clippy", features).arg("--all-targets").args(args).run()?; Ok(()) } - pub fn test(&self, crate_dir: impl AsRef, args: &[String]) -> Result<()> { - self.cargo_cmd(crate_dir, "test").args(args).run()?; + pub fn test( + &self, + crate_dir: impl AsRef, + features: &[String], + args: &[String], + ) -> Result<()> { + self.cargo_cmd(crate_dir, "test", features).args(args).run()?; Ok(()) }