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

Issue:
version-range: Support multiple different MSRVs in a workspace
To implement this, we need to make sure that every crate in which the command is actually run has rust-version field, if there are multiple different MSRVs in a workspace.

https://github.com/taiki-e/cargo-hack/blob/1057aec30d897460a4379bd7dcea7b268e93c475/src/rustup.rs#L43-L61

---

A concrete example of such a workspace is futures-rs:
- futures-core: 1.36 https://github.com/rust-lang/futures-rs/blob/6dc49d003cc216cc7321041dbea7e12f5fc59a1a/futures-core/Cargo.toml#L5
- futures: 1.56 https://github.com/rust-lang/futures-rs/blob/6dc49d003cc216cc7321041dbea7e12f5fc59a1a/futures/Cargo.toml#L5


Discussion / hints:
If you want, I can take this on after #196 is fixed.  They'll conflict a lot with each other and we would need this if we are to use this in cargo.
That would be great!
The most basic implementation is "pick the highest version in the workspace" (ie what will work for everything).  What cargo needs is an MSRV per package.  I can think of one of two ways to implement this and wanted to get your thoughts
- Defer evaluating `MaybeVersion::Msrv` from #202 until we get to package processing
- Instantiate a range per package

Any preferences?
> - Defer evaluating MaybeVersion::Msrv from feat: Add --rust-version flag  #202 until we get to package processing
> - Instantiate a range per package


I was originally trying to implement this with the former approach (evaluating MSRV after `determine_package_list`, which creates a list of packages to actually run command on). The patch can be found [here](https://github.com/taiki-e/cargo-hack/commit/1d647e877e030d5e56e7202187e23ed5ad35d894) (feel free to copy it if needed).

That patch is incomplete because there are the following two main problems, but I'm still thinking it is reasonable to take that approach.

1\. [`--version-step` is inaccurate](https://github.com/taiki-e/cargo-hack/commit/1d647e877e030d5e56e7202187e23ed5ad35d894#diff-42cb6807ad74b3e201c5a7ca98b911c5fa08380e942be6e4ac5807f8377f87fcR75-R81).
2\. [Build number counter is inaccurate](https://github.com/taiki-e/cargo-hack/commit/1d647e877e030d5e56e7202187e23ed5ad35d894#diff-42cb6807ad74b3e201c5a7ca98b911c5fa08380e942be6e4ac5807f8377f87fcR109-R110).

I think 1\. is a problem that does not exist in the "Instantiate a range per package" approach, but "Instantiate a range per package" approach has a (perhaps more complex) problem that may increase the number of rust versions to be used (e.g., if step is 2, crate `a`'s  MSRV is 63, crate `b`'s MSRV is 64, the range is [63, 65, 67, ...] for crate `a` and [64, 66, 68, ...] for crate `b`).

Personally, I think that if there is a crate with an MSRV greater than the lowest one, it is okay to just add that version to the range (e.g., if step is 2, crate `a`'s  MSRV is 63, crate `b`'s MSRV is 64, the range is [63, *64*, 65, 67, ..]).

Alternatively, it is also okay to implement the minimum ones and postpone `--version-step` support.

I think 2\. can be addressed by evaluating MSRV (at least partially) inside `determine_package_list`.

Fix patch:
diff --git a/src/main.rs b/src/main.rs
index 32628052..e0ed744f 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -62,70 +62,78 @@ fn try_main() -> Result<()> {
             return Ok(());
         }
 
+        let packages = determine_package_list(cx)?;
         let mut progress = Progress::default();
-        let packages = determine_package_list(cx, &mut progress)?;
-
         let mut keep_going = KeepGoing::default();
         if let Some(range) = cx.version_range {
-            if range == VersionRange::msrv() {
-                let mut versions = BTreeMap::new();
-                for (id, kind) in packages {
-                    let v =
-                        cx.rust_version(id).map(str::parse::<Version>).transpose()?.ok_or_else(
-                            || format_err!("no rust-version field in Cargo.toml is specified"),
-                        )?;
-                    versions.entry(v.strip_patch()).or_insert_with(Vec::new).push((id, kind));
+            let mut versions = BTreeMap::new();
+            let steps = rustup::version_range(range, cx.version_step, &packages, cx)?;
+            for pkg in packages {
+                let msrv = cx
+                    .rust_version(pkg.id)
+                    .map(str::parse::<Version>)
+                    .transpose()?
+                    .map(Version::strip_patch);
+                if range == VersionRange::msrv() {
+                    let msrv = msrv.ok_or_else(|| {
+                        format_err!("no rust-version field in Cargo.toml is specified")
+                    })?;
+                    versions.entry(msrv).or_insert_with(Vec::new).push(pkg);
+                } else {
+                    let mut seen = false;
+                    for cargo_version in &steps {
+                        if msrv.is_some() && Some(*cargo_version) < msrv {
+                            continue;
+                        }
+                        if !seen {
+                            if Some(*cargo_version) != msrv {
+                                if let Some(msrv) = msrv {
+                                    versions.entry(msrv).or_insert_with(Vec::new).push(pkg.clone());
+                                }
+                            }
+                            seen = true;
+                        }
+                        versions.entry(*cargo_version).or_insert_with(Vec::new).push(pkg.clone());
+                    }
+                    if !seen {
+                        let package = cx.packages(pkg.id);
+                        let name = &package.name;
+                        let msrv = msrv.expect("always `seen` if no msrv");
+                        warn!("skipping {name}, rust-version ({msrv}) is not in specified range ({range})");
+                    }
                 }
+            }
 
-                // First, generate the lockfile using the oldest cargo specified.
-                // https://github.com/taiki-e/cargo-hack/issues/105
-                let mut generate_lockfile = true;
-                // Workaround for spurious "failed to select a version" error.
-                // (This does not work around the underlying cargo bug: https://github.com/rust-lang/cargo/issues/10623)
-                let mut regenerate_lockfile_on_51_or_up = false;
-                for (cargo_version, packages) in versions {
-                    versioned_cargo_exec_on_packages(
-                        cx,
-                        &packages,
-                        cargo_version.minor,
-                        &mut progress,
-                        &mut keep_going,
-                        &mut generate_lockfile,
-                        &mut regenerate_lockfile_on_51_or_up,
-                    )?;
-                }
-            } else {
-                let range = rustup::version_range(range, cx.version_step, &packages, cx)?;
-
-                let total = progress.total;
-                progress.total = 0;
-                for cargo_version in &range {
-                    if cx.target.is_empty() || *cargo_version >= 64 {
-                        progress.total += total;
+            for (cargo_version, packages) in &versions {
+                for package in packages {
+                    if cx.target.is_empty() || cargo_version.minor >= 64 {
+                        progress.total += package.feature_count;
                     } else {
-                        progress.total += total * cx.target.len();
+                        progress.total += package.feature_count * cx.target.len();
                     }
                 }
+            }
 
-                // First, generate the lockfile using the oldest cargo specified.
-                // https://github.com/taiki-e/cargo-hack/issues/105
-                let mut generate_lockfile = true;
-                // Workaround for spurious "failed to select a version" error.
-                // (This does not work around the underlying cargo bug: https://github.com/rust-lang/cargo/issues/10623)
-                let mut regenerate_lockfile_on_51_or_up = false;
-                for cargo_version in range {
-                    versioned_cargo_exec_on_packages(
-                        cx,
-                        &packages,
-                        cargo_version,
-                        &mut progress,
-                        &mut keep_going,
-                        &mut generate_lockfile,
-                        &mut regenerate_lockfile_on_51_or_up,
-                    )?;
-                }
+            // First, generate the lockfile using the oldest cargo specified.
+            // https://github.com/taiki-e/cargo-hack/issues/105
+            let mut generate_lockfile = true;
+            // Workaround for spurious "failed to select a version" error.
+            // (This does not work around the underlying cargo bug: https://github.com/rust-lang/cargo/issues/10623)
+            let mut regenerate_lockfile_on_51_or_up = false;
+            for (cargo_version, packages) in versions {
+                versioned_cargo_exec_on_packages(
+                    cx,
+                    &packages,
+                    cargo_version.minor,
+                    &mut progress,
+                    &mut keep_going,
+                    &mut generate_lockfile,
+                    &mut regenerate_lockfile_on_51_or_up,
+                )?;
             }
         } else {
+            let total = packages.iter().map(|p| p.feature_count).sum();
+            progress.total = total;
             default_cargo_exec_on_packages(cx, &packages, &mut progress, &mut keep_going)?;
         }
         if keep_going.count > 0 {
@@ -142,6 +150,7 @@ struct Progress {
     count: usize,
 }
 
+#[derive(Clone)]
 enum Kind<'a> {
     Normal,
     Each { features: Vec<&'a Feature> },
@@ -150,18 +159,18 @@ enum Kind<'a> {
 
 fn determine_kind<'a>(
     cx: &'a Context,
-    id: &PackageId,
-    progress: &mut Progress,
+    id: &'a PackageId,
     multiple_packages: bool,
-) -> Option<Kind<'a>> {
+) -> Option<PackageRuns<'a>> {
     assert!(cx.subcommand.is_some());
     if cx.ignore_private && cx.is_private(id) {
         info!("skipped running on private package `{}`", cx.name_verbose(id));
         return None;
     }
     if !cx.each_feature && !cx.feature_powerset {
-        progress.total += 1;
-        return Some(Kind::Normal);
+        let feature_count = 1;
+        let kind = Kind::Normal;
+        return Some(PackageRuns { id, kind, feature_count });
     }
 
     let package = cx.packages(id);
@@ -220,15 +229,17 @@ fn determine_kind<'a>(
             || !cx.include_features.is_empty())
             && features.is_empty()
         {
-            progress.total += 1;
-            Some(Kind::Normal)
+            let feature_count = 1;
+            let kind = Kind::Normal;
+            Some(PackageRuns { id, kind, feature_count })
         } else {
-            progress.total += features.len()
+            let feature_count = features.len()
                 + !cx.exclude_no_default_features as usize
                 + (!cx.exclude_all_features
                     && pkg_features.normal().len() + pkg_features.optional_deps().len() > 1)
                     as usize;
-            Some(Kind::Each { features })
+            let kind = Kind::Each { features };
+            Some(PackageRuns { id, kind, feature_count })
         }
     } else if cx.feature_powerset {
         let features =
@@ -238,25 +249,31 @@ fn determine_kind<'a>(
             || !cx.include_features.is_empty())
             && features.is_empty()
         {
-            progress.total += 1;
-            Some(Kind::Normal)
+            let feature_count = 1;
+            let kind = Kind::Normal;
+            Some(PackageRuns { id, kind, feature_count })
         } else {
-            progress.total += features.len()
+            let feature_count = features.len()
                 + !cx.exclude_no_default_features as usize
                 + (!cx.exclude_all_features
                     && pkg_features.normal().len() + pkg_features.optional_deps().len() > 1)
                     as usize;
-            Some(Kind::Powerset { features })
+            let kind = Kind::Powerset { features };
+            Some(PackageRuns { id, kind, feature_count })
         }
     } else {
         unreachable!()
     }
 }
 
-fn determine_package_list<'a>(
-    cx: &'a Context,
-    progress: &mut Progress,
-) -> Result<Vec<(&'a PackageId, Kind<'a>)>> {
+#[derive(Clone)]
+struct PackageRuns<'a> {
+    id: &'a PackageId,
+    kind: Kind<'a>,
+    feature_count: usize,
+}
+
+fn determine_package_list(cx: &Context) -> Result<Vec<PackageRuns<'_>>> {
     Ok(if cx.workspace {
         for spec in &cx.exclude {
             if !cx.workspace_members().any(|id| cx.packages(id).name == *spec) {
@@ -270,9 +287,7 @@ fn determine_package_list<'a>(
         let multiple_packages = cx.workspace_members().len().saturating_sub(cx.exclude.len()) > 1;
         cx.workspace_members()
             .filter(|id| !cx.exclude.contains(&cx.packages(id).name))
-            .filter_map(|id| {
-                determine_kind(cx, id, progress, multiple_packages).map(|kind| (id, kind))
-            })
+            .filter_map(|id| determine_kind(cx, id, multiple_packages))
             .collect()
     } else if !cx.package.is_empty() {
         if let Some(spec) = cx
@@ -286,32 +301,24 @@ fn determine_package_list<'a>(
         let multiple_packages = cx.package.len() > 1;
         cx.workspace_members()
             .filter(|id| cx.package.contains(&cx.packages(id).name))
-            .filter_map(|id| {
-                determine_kind(cx, id, progress, multiple_packages).map(|kind| (id, kind))
-            })
+            .filter_map(|id| determine_kind(cx, id, multiple_packages))
             .collect()
     } else if cx.current_package().is_none() {
         let multiple_packages = cx.workspace_members().len() > 1;
-        cx.workspace_members()
-            .filter_map(|id| {
-                determine_kind(cx, id, progress, multiple_packages).map(|kind| (id, kind))
-            })
-            .collect()
+        cx.workspace_members().filter_map(|id| determine_kind(cx, id, multiple_packages)).collect()
     } else {
         let current_package = &cx.packages(cx.current_package().unwrap()).name;
         let multiple_packages = false;
         cx.workspace_members()
             .find(|id| cx.packages(id).name == *current_package)
-            .and_then(|id| {
-                determine_kind(cx, id, progress, multiple_packages).map(|kind| vec![(id, kind)])
-            })
+            .and_then(|id| determine_kind(cx, id, multiple_packages).map(|p| vec![p]))
             .unwrap_or_default()
     })
 }
 
 fn versioned_cargo_exec_on_packages(
     cx: &Context,
-    packages: &[(&PackageId, Kind<'_>)],
+    packages: &[PackageRuns<'_>],
     cargo_version: u32,
     progress: &mut Progress,
     keep_going: &mut KeepGoing,
@@ -362,7 +369,7 @@ fn versioned_cargo_exec_on_packages(
 
 fn default_cargo_exec_on_packages(
     cx: &Context,
-    packages: &[(&PackageId, Kind<'_>)],
+    packages: &[PackageRuns<'_>],
     progress: &mut Progress,
     keep_going: &mut KeepGoing,
 ) -> Result<()> {
@@ -373,7 +380,7 @@ fn default_cargo_exec_on_packages(
 
 fn exec_on_packages(
     cx: &Context,
-    packages: &[(&PackageId, Kind<'_>)],
+    packages: &[PackageRuns<'_>],
     mut line: ProcessBuilder<'_>,
     progress: &mut Progress,
     keep_going: &mut KeepGoing,
@@ -387,14 +394,14 @@ fn exec_on_packages(
         }
         packages
             .iter()
-            .try_for_each(|(id, kind)| exec_on_package(cx, id, kind, &line, progress, keep_going))
+            .try_for_each(|pkg| exec_on_package(cx, pkg.id, &pkg.kind, &line, progress, keep_going))
     } else {
         cx.target.iter().try_for_each(|target| {
             let mut line = line.clone();
             line.arg("--target");
             line.arg(target);
-            packages.iter().try_for_each(|(id, kind)| {
-                exec_on_package(cx, id, kind, &line, progress, keep_going)
+            packages.iter().try_for_each(|pkg| {
+                exec_on_package(cx, pkg.id, &pkg.kind, &line, progress, keep_going)
             })
         })
     }
diff --git a/src/rustup.rs b/src/rustup.rs
index 378cea43..abe96832 100644
--- a/src/rustup.rs
+++ b/src/rustup.rs
@@ -5,9 +5,8 @@ use anyhow::{bail, format_err, Result};
 use crate::{
     cargo,
     context::Context,
-    metadata::PackageId,
     version::{MaybeVersion, Version, VersionRange},
-    Kind,
+    PackageRuns,
 };
 
 pub(crate) struct Rustup {
@@ -28,9 +27,9 @@ impl Rustup {
 pub(crate) fn version_range(
     range: VersionRange,
     step: u16,
-    packages: &[(&PackageId, Kind<'_>)],
+    packages: &[PackageRuns<'_>],
     cx: &Context,
-) -> Result<Vec<u32>> {
+) -> Result<Vec<Version>> {
     let check = |version: &Version| {
         if version.major != 1 {
             bail!("major version must be 1");
@@ -61,23 +60,30 @@ pub(crate) fn version_range(
         if let Some(rust_version) = rust_version {
             Ok(rust_version)
         } else {
-            let mut version = None;
-            for (id, _) in packages {
-                let v = cx.rust_version(id);
-                if v.is_none() || v == version {
-                    // no-op
-                } else if version.is_none() {
-                    version = v;
-                } else {
-                    bail!("automatic detection of the lower bound of the version range is not yet supported when the minimum supported Rust version of the crates in the workspace do not match")
-                }
+            let mut lowest_msrv = None;
+            for pkg in packages {
+                let pkg_msrv = cx
+                    .rust_version(pkg.id)
+                    .map(str::parse::<Version>)
+                    .transpose()?
+                    .map(Version::strip_patch);
+                lowest_msrv = match (lowest_msrv, pkg_msrv) {
+                    (Some(workspace), Some(pkg)) => {
+                        if workspace < pkg {
+                            Some(workspace)
+                        } else {
+                            Some(pkg)
+                        }
+                    }
+                    (Some(msrv), None) | (None, Some(msrv)) => Some(msrv),
+                    (None, None) => None,
+                };
             }
-            let version = match version {
-                Some(v) => v.parse()?,
-                None => bail!("no rust-version field in Cargo.toml is specified"),
+            let Some(lowest_msrv) = lowest_msrv else {
+                bail!("no rust-version field in Cargo.toml is specified")
             };
-            rust_version = Some(version);
-            Ok(version)
+            rust_version = Some(lowest_msrv);
+            Ok(lowest_msrv)
         }
     };
 
@@ -105,8 +111,10 @@ pub(crate) fn version_range(
         MaybeVersion::Stable => get_stable_version()?,
     };
 
-    let versions: Vec<_> =
-        (start_inclusive.minor..=end_inclusive.minor).step_by(step as _).collect();
+    let versions: Vec<_> = (start_inclusive.minor..=end_inclusive.minor)
+        .step_by(step as _)
+        .map(|minor| Version { major: 1, minor, patch: None })
+        .collect();
     if versions.is_empty() {
         bail!("specified version range `{range}` is empty");
     }


Test patch:
diff --git a/tests/test.rs b/tests/test.rs
index 241c38f2..ee48b490 100644
--- a/tests/test.rs
+++ b/tests/test.rs
@@ -1353,11 +1353,30 @@ fn version_range() {
             running `rustup run 1.64 cargo check` on member2 (4/4)
             ",
         );
+    // Skips `real` because it isn't in range
     cargo_hack(["check", "--version-range", "..=1.64", "--workspace"])
-        .assert_failure("rust-version")
+        .assert_failure("rust-version") // warn
         .stderr_contains(
             "
-            automatic detection of the lower bound of the version range is not yet supported when the minimum supported Rust version of the crates in the workspace do not match
+            warning: skipping real, rust-version (1.65) is not in specified range (..=1.64)
+            running `rustup run 1.63 cargo check` on member1 (1/5)
+            running `rustup run 1.63 cargo check` on member2 (2/5)
+            running `rustup run 1.64 cargo check` on member1 (3/5)
+            running `rustup run 1.64 cargo check` on member2 (4/5)
+            running `rustup run 1.64 cargo check` on member3 (5/5)
+            ",
+        );
+    cargo_hack(["check", "--version-range", "..=1.66", "--version-step", "2", "--workspace"])
+        .assert_success("rust-version")
+        .stderr_contains(
+            "
+            running `rustup run 1.63 cargo check` on member1 (1/7)
+            running `rustup run 1.63 cargo check` on member2 (2/7)
+            running `rustup run 1.64 cargo check` on member3 (3/7)
+            running `rustup run 1.65 cargo check` on member1 (4/7)
+            running `rustup run 1.65 cargo check` on member2 (5/7)
+            running `rustup run 1.65 cargo check` on member3 (6/7)
+            running `rustup run 1.65 cargo check` on real (7/7)
             ",
         );
 }