Repository: taiki-e/cargo-hack (language: rust)

Issue:
Automatically install target if specified when using `--version-range`
If the `--target` flag is used when using `--version-range`, it makes sense to automatically install the target. This is a no-op if it is already installed, so the additional cost is near zero.

I think this is a reasonable addition given that `--version-range` is intended to be used in CI, and `cargo-hack` already knows what versions are being used. Without this feature, the logic for what versions need the target installed would need to be duplicated.

This would, of course, fail if the target doesn't exist in a given release. That error would presumably be passed on the same way others are.


Discussion / hints:
This seems reasonable to me!



Fix patch:
diff --git a/src/cli.rs b/src/cli.rs
index 34db5d51..3d161a6b 100644
--- a/src/cli.rs
+++ b/src/cli.rs
@@ -57,14 +57,19 @@ pub(crate) struct Args<'a> {
     /// --group-features <FEATURES>...
     pub(crate) group_features: Vec<Feature>,
 
+    // options that will be propagated to cargo
+    /// --features <FEATURES>...
+    pub(crate) features: Vec<&'a str>,
+
+    // propagated (as a part of leading_args) to cargo
     /// --no-default-features
     pub(crate) no_default_features: bool,
     /// -v, --verbose, -vv
     pub(crate) verbose: bool,
-
-    // options that will be propagated to cargo
-    /// --features <FEATURES>...
-    pub(crate) features: Vec<&'a str>,
+    // Note: specifying multiple `--target` flags requires unstable `-Zmultitarget`,
+    // so cargo-hack currently only supports a single `--target`.
+    /// --target <TRIPLE>...
+    pub(crate) target: Option<&'a str>,
 }
 
 pub(crate) fn raw() -> RawArgs {
@@ -122,6 +127,7 @@ pub(crate) fn parse_args<'a>(raw: &'a RawArgs, cargo: &Cargo, rustup: &Rustup) -
     let mut verbose = false;
     let mut no_default_features = false;
     let mut all_features = false;
+    let mut target = None;
 
     let res = (|| -> Result<()> {
         while let Some(arg) = args.next() {
@@ -192,6 +198,7 @@ pub(crate) fn parse_args<'a>(raw: &'a RawArgs, cargo: &Cargo, rustup: &Rustup) -
             parse_opt!(color, true, "--color");
             parse_opt!(version_range, false, "--version-range");
             parse_opt!(version_step, false, "--version-step");
+            parse_opt!(target, true, "--target");
 
             parse_multi_opt!(package, false, "--package");
             parse_multi_opt!(package, false, "-p");
@@ -473,14 +480,15 @@ pub(crate) fn parse_args<'a>(raw: &'a RawArgs, cargo: &Cargo, rustup: &Rustup) -
         depth,
         group_features,
 
-        no_default_features,
-        verbose,
-
         exclude_features,
         exclude_no_default_features,
         exclude_all_features,
 
         features,
+
+        no_default_features,
+        verbose,
+        target,
     })
 }
 
diff --git a/src/main.rs b/src/main.rs
index 76842448..5581da96 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -57,7 +57,7 @@ fn exec_on_workspace(cx: &Context<'_>) -> Result<()> {
     let restore = Restore::new(cx);
     if let Some(range) = &cx.version_range {
         progress.total *= range.len();
-        let mut line = ProcessBuilder::new("cargo".as_ref());
+        let mut line = ProcessBuilder::new("cargo");
         if cx.verbose {
             line.display_manifest_path();
         }
@@ -65,7 +65,7 @@ fn exec_on_workspace(cx: &Context<'_>) -> Result<()> {
             // First, generate the lockfile using the oldest cargo specified.
             // https://github.com/taiki-e/cargo-hack/issues/105
             let toolchain = &range[0];
-            rustup::install_toolchain(toolchain)?;
+            rustup::install_toolchain(toolchain, cx.target, true)?;
             let mut line = line.clone();
             line.leading_arg(toolchain);
             line.args(&["generate-lockfile"]);
@@ -82,8 +82,10 @@ fn exec_on_workspace(cx: &Context<'_>) -> Result<()> {
             line.exec_with_output()?;
         }
 
-        range.iter().try_for_each(|toolchain| {
-            rustup::install_toolchain(toolchain)?;
+        range.iter().enumerate().try_for_each(|(i, toolchain)| {
+            if i != 0 {
+                rustup::install_toolchain(toolchain, cx.target, true)?;
+            }
             let mut line = line.clone();
             line.leading_arg(toolchain);
             line.with_args(cx);
diff --git a/src/process.rs b/src/process.rs
index d5727d25..9d15364c 100644
--- a/src/process.rs
+++ b/src/process.rs
@@ -5,6 +5,7 @@ use std::{
     fmt,
     path::Path,
     process::{Command, ExitStatus, Output},
+    rc::Rc,
     str,
 };
 
@@ -18,7 +19,7 @@ use crate::{Context, PackageId, Result};
 pub(crate) struct ProcessBuilder<'a> {
     // $program $leading_args $propagated_leading_args $args $propagated_trailing_args
     /// The program to execute.
-    program: &'a OsStr,
+    program: Rc<OsStr>,
     /// A list of arguments to pass to the program (until '--').
     propagated_leading_args: &'a [&'a str],
     /// A list of arguments to pass to the program (after '--').
@@ -41,9 +42,9 @@ pub(crate) struct ProcessBuilder<'a> {
 
 impl<'a> ProcessBuilder<'a> {
     /// Creates a new `ProcessBuilder`.
-    pub(crate) fn new(program: &'a OsStr) -> Self {
+    pub(crate) fn new(program: impl AsRef<OsStr>) -> Self {
         Self {
-            program,
+            program: program.as_ref().into(),
             propagated_leading_args: &[],
             trailing_args: &[],
             leading_args: Vec::new(),
diff --git a/src/rustup.rs b/src/rustup.rs
index 2e020ef3..24e99046 100644
--- a/src/rustup.rs
+++ b/src/rustup.rs
@@ -43,8 +43,8 @@ pub(crate) fn version_range(range: &str, step: Option<&str>) -> Result<Vec<Strin
 
     let end = match split.next() {
         Some("") | None => {
-            install_toolchain("stable")?;
-            cargo::minor_version(ProcessBuilder::new("cargo".as_ref()).args(&["+stable"]))?
+            install_toolchain("stable", None, false)?;
+            cargo::minor_version(ProcessBuilder::new("cargo").args(&["+stable"]))?
         }
         Some(end) => {
             let end = parse_version(end)?;
@@ -66,20 +66,34 @@ pub(crate) fn version_range(range: &str, step: Option<&str>) -> Result<Vec<Strin
     Ok(versions)
 }
 
-pub(crate) fn install_toolchain(mut toolchain: &str) -> Result<()> {
+pub(crate) fn install_toolchain(
+    mut toolchain: &str,
+    target: Option<&str>,
+    print_output: bool,
+) -> Result<()> {
     if toolchain.starts_with('+') {
         toolchain = &toolchain[1..];
     }
     // In Github Actions and Azure Pipelines, --no-self-update is necessary
     // because the windows environment cannot self-update rustup.exe.
-    rustup()
-        .args(&["toolchain", "install", toolchain, "--no-self-update"])
-        .exec_with_output()
-        .map(drop)
+    let mut cmd = rustup();
+    cmd.args(&["toolchain", "install", toolchain, "--no-self-update"]);
+    if let Some(target) = target {
+        cmd.args(&["--target", target]);
+    }
+
+    if print_output {
+        // The toolchain installation can take some time, so we'll show users
+        // the progress.
+        cmd.exec()
+    } else {
+        // However, in certain situations, it may be preferable not to display it.
+        cmd.exec_with_output().map(drop)
+    }
 }
 
 fn rustup<'a>() -> ProcessBuilder<'a> {
-    ProcessBuilder::new("rustup".as_ref())
+    ProcessBuilder::new("rustup")
 }
 
 fn minor_version() -> Result<u32> {


Test patch:
diff --git a/tests/auxiliary/mod.rs b/tests/auxiliary/mod.rs
index b77c64f9..4e7c154f 100644
--- a/tests/auxiliary/mod.rs
+++ b/tests/auxiliary/mod.rs
@@ -32,7 +32,7 @@ pub fn cargo_hack<O: AsRef<OsStr>>(args: impl AsRef<[O]>) -> Command {
 
 #[ext(CommandExt)]
 impl Command {
-    pub fn test_dir(&mut self, path: &str) -> &mut Self {
+    pub fn test_dir(&mut self, path: impl AsRef<Path>) -> &mut Self {
         self.current_dir(Path::new(env!("CARGO_MANIFEST_DIR")).join(path))
     }
 
@@ -88,8 +88,8 @@ fn line_separated(lines: &str, f: impl FnMut(&str)) {
 impl AssertOutput {
     /// Receives a line(`\n`)-separated list of patterns and asserts whether stderr contains each pattern.
     #[rustversion::attr(since(1.46), track_caller)]
-    pub fn stderr_contains(&self, pats: &str) -> &Self {
-        line_separated(pats, |pat| {
+    pub fn stderr_contains(&self, pats: impl AsRef<str>) -> &Self {
+        line_separated(pats.as_ref(), |pat| {
             if !self.stderr.contains(pat) {
                 panic!(
                     "assertion failed: `self.stderr.contains(..)`:\n\nEXPECTED:\n{0}\n{1}\n{0}\n\nACTUAL:\n{0}\n{2}\n{0}\n",
@@ -104,8 +104,8 @@ impl AssertOutput {
 
     /// Receives a line(`\n`)-separated list of patterns and asserts whether stdout contains each pattern.
     #[rustversion::attr(since(1.46), track_caller)]
-    pub fn stderr_not_contains(&self, pats: &str) -> &Self {
-        line_separated(pats, |pat| {
+    pub fn stderr_not_contains(&self, pats: impl AsRef<str>) -> &Self {
+        line_separated(pats.as_ref(), |pat| {
             if self.stderr.contains(pat) {
                 panic!(
                     "assertion failed: `!self.stderr.contains(..)`:\n\nEXPECTED:\n{0}\n{1}\n{0}\n\nACTUAL:\n{0}\n{2}\n{0}\n",
@@ -120,8 +120,8 @@ impl AssertOutput {
 
     /// Receives a line(`\n`)-separated list of patterns and asserts whether stdout contains each pattern.
     #[rustversion::attr(since(1.46), track_caller)]
-    pub fn stdout_contains(&self, pats: &str) -> &Self {
-        line_separated(pats, |pat| {
+    pub fn stdout_contains(&self, pats: impl AsRef<str>) -> &Self {
+        line_separated(pats.as_ref(), |pat| {
             if !self.stdout.contains(pat) {
                 panic!(
                     "assertion failed: `self.stdout.contains(..)`:\n\nEXPECTED:\n{0}\n{1}\n{0}\n\nACTUAL:\n{0}\n{2}\n{0}\n",
@@ -136,8 +136,8 @@ impl AssertOutput {
 
     /// Receives a line(`\n`)-separated list of patterns and asserts whether stdout contains each pattern.
     #[rustversion::attr(since(1.46), track_caller)]
-    pub fn stdout_not_contains(&self, pats: &str) -> &Self {
-        line_separated(pats, |pat| {
+    pub fn stdout_not_contains(&self, pats: impl AsRef<str>) -> &Self {
+        line_separated(pats.as_ref(), |pat| {
             if self.stdout.contains(pat) {
                 panic!(
                     "assertion failed: `!self.stdout.contains(..)`:\n\nEXPECTED:\n{0}\n{1}\n{0}\n\nACTUAL:\n{0}\n{2}\n{0}\n",
@@ -150,3 +150,30 @@ impl AssertOutput {
         self
     }
 }
+
+pub fn target_triple() -> &'static str {
+    if cfg!(not(target_arch = "x86_64")) {
+        panic!("non x86_64 arch")
+    }
+    if cfg!(target_os = "linux") {
+        if cfg!(target_env = "gnu") {
+            "x86_64-unknown-linux-gnu"
+        } else if cfg!(target_env = "musl") {
+            "x86_64-unknown-linux-musl"
+        } else {
+            panic!("non gnu/musl linux")
+        }
+    } else if cfg!(target_os = "macos") {
+        "x86_64-apple-darwin"
+    } else if cfg!(target_os = "windows") {
+        if cfg!(target_env = "gnu") {
+            "x86_64-pc-windows-gnu"
+        } else if cfg!(target_env = "msvc") {
+            "x86_64-pc-windows-msvc"
+        } else {
+            unreachable!()
+        }
+    } else {
+        panic!("non linux/macos/windows os")
+    }
+}
diff --git a/tests/test.rs b/tests/test.rs
index 1615024d..4e293fe4 100644
--- a/tests/test.rs
+++ b/tests/test.rs
@@ -2,7 +2,7 @@
 
 mod auxiliary;
 
-use auxiliary::{cargo_bin_exe, cargo_hack, CommandExt, SEPARATOR};
+use auxiliary::{cargo_bin_exe, cargo_hack, target_triple, CommandExt, SEPARATOR};
 
 #[test]
 fn failures() {
@@ -48,7 +48,7 @@ fn multi_arg() {
         cargo_hack(["check", flag, flag])
             .test_dir("tests/fixtures/real")
             .assert_failure()
-            .stderr_contains(&format!(
+            .stderr_contains(format!(
                 "The argument '{}' was provided more than once, but cannot be used multiple times",
                 flag.split('=').next().unwrap()
             ));
@@ -64,7 +64,7 @@ fn removed_flags() {
         cargo_hack(["check", flag])
             .test_dir("tests/fixtures/real")
             .assert_failure()
-            .stderr_contains(&format!("{} was removed, use {} instead", flag, alt));
+            .stderr_contains(format!("{} was removed, use {} instead", flag, alt));
     }
 }
 
@@ -320,7 +320,7 @@ fn no_dev_deps_failure() {
         cargo_hack(["check", "--no-dev-deps", flag])
             .test_dir("tests/fixtures/real")
             .assert_failure()
-            .stderr_contains(&format!("--no-dev-deps may not be used together with {}", flag));
+            .stderr_contains(format!("--no-dev-deps may not be used together with {}", flag));
     }
 
     // with subcommands requires dev-deps
@@ -328,7 +328,7 @@ fn no_dev_deps_failure() {
         cargo_hack([subcommand, "--no-dev-deps"])
             .test_dir("tests/fixtures/real")
             .assert_failure()
-            .stderr_contains(&format!(
+            .stderr_contains(format!(
                 "--no-dev-deps may not be used together with {} subcommand",
                 subcommand
             ));
@@ -344,7 +344,7 @@ fn remove_dev_deps_failure() {
         cargo_hack(["check", "--remove-dev-deps", flag])
             .test_dir("tests/fixtures/real")
             .assert_failure()
-            .stderr_contains(&format!("--remove-dev-deps may not be used together with {}", flag));
+            .stderr_contains(format!("--remove-dev-deps may not be used together with {}", flag));
     }
 
     // with subcommands requires dev-deps
@@ -352,7 +352,7 @@ fn remove_dev_deps_failure() {
         cargo_hack([subcommand, "--remove-dev-deps"])
             .test_dir("tests/fixtures/real")
             .assert_failure()
-            .stderr_contains(&format!(
+            .stderr_contains(format!(
                 "--remove-dev-deps may not be used together with {} subcommand",
                 subcommand
             ));
@@ -1338,7 +1338,7 @@ fn verbose() {
     cargo_hack(["check", "--verbose"])
         .test_dir("tests/fixtures/virtual")
         .assert_success()
-        .stderr_contains(&format!(
+        .stderr_contains(format!(
             "
             running `cargo check --manifest-path member1{0}Cargo.toml`
             running `cargo check --manifest-path member2{0}Cargo.toml`
@@ -1382,6 +1382,12 @@ fn propagate() {
         .assert_success()
         .stderr_contains("`cargo check --color auto`");
 
+    // --target
+    cargo_hack(["check", "--target", target_triple()])
+        .test_dir("tests/fixtures/real")
+        .assert_success()
+        .stderr_contains(format!("`cargo check --target {}`", target_triple()));
+
     // --verbose does not be propagated
     cargo_hack(["check", "--verbose"])
         .test_dir("tests/fixtures/real")
@@ -1416,6 +1422,50 @@ fn default_feature_behavior() {
         .stdout_not_contains("has default feature!");
 }
 
+// It seems rustup is not installed in the docker image provided by cross.
+#[cfg_attr(any(not(target_arch = "x86_64"), target_env = "musl"), ignore)]
+#[test]
+fn version_range() {
+    cargo_hack(["check", "--version-range", "1.36..1.37"])
+        .test_dir("tests/fixtures/run/real")
+        .assert_success()
+        .stderr_contains(
+            "
+            running `cargo +1.36 check` on real (1/2)
+            running `cargo +1.37 check` on real (2/2)
+            ",
+        );
+
+    cargo_hack(["check", "--version-range", "1.36..1.37", "--target", target_triple()])
+        .test_dir("tests/fixtures/run/real")
+        .assert_success()
+        .stderr_contains(format!(
+            "
+            running `cargo +1.36 check --target {0}` on real (1/2)
+            running `cargo +1.37 check --target {0}` on real (2/2)
+            ",
+            target_triple()
+        ));
+
+    if cfg!(target_os = "linux") {
+        cargo_hack([
+            "check",
+            "--version-range",
+            "1.36..1.37",
+            "--target",
+            "x86_64-unknown-linux-musl",
+        ])
+        .test_dir("tests/fixtures/run/real")
+        .assert_success()
+        .stderr_contains(
+            "
+            running `cargo +1.36 check --target x86_64-unknown-linux-musl` on real (1/2)
+            running `cargo +1.37 check --target x86_64-unknown-linux-musl` on real (2/2)
+            ",
+        );
+    }
+}
+
 // It seems rustup is not installed in the docker image provided by cross.
 #[cfg_attr(any(not(target_arch = "x86_64"), target_env = "musl"), ignore)]
 #[test]