Merge #420

420: Allow using manifest-path instead of crate path r=foresterre a=foresterre

* Now accepts either `--manifest-path <Cargo Manifest>` or `--path <Cargo Dir>`

closes #412, replaces #419 (Context changes negatively impacted running of test cases)

Co-authored-by: Martijn Gribnau <garm@ilumeo.com>

CHANGELOG.md

@@ -13,7 +13,8 @@ the [issue tracker](https://github.com/foresterre/cargo-msrv/issues), or open a
 
 * Subcommand `cargo msrv verify` now supports setting a custom Rust version via the `--rust-version <VERSION>` argument, 
   which can be used to check for a crate's compatibility against a specific Rust version. 
-* Flag `--write-msrv` to cargo msrv (find), which upon finding the MSRV writes its value to the Cargo manifest.
+* Added flag `--write-msrv` to cargo msrv (find), which upon finding the MSRV writes its value to the Cargo manifest.
+* Added option to refer to a specific crate using its Cargo manifest (with `--manifest-path`) instead of its path (with `--path`)
 
 ### Changed
 

src/check/rustup_toolchain_check.rs

@@ -33,11 +33,9 @@ impl<'reporter, R: Reporter> Check for RustupToolchainCheck<'reporter, R> {
 
                 self.prepare(toolchain, config)?;
 
-                let outcome = self.run_check_command_via_rustup(
-                    toolchain,
-                    config.crate_path(),
-                    config.check_command(),
-                )?;
+                let path = current_dir_crate_path(config)?;
+                let outcome =
+                    self.run_check_command_via_rustup(toolchain, path, config.check_command())?;
 
                 // report outcome to UI
                 self.report_outcome(&outcome, config.no_check_feedback())?;
@@ -164,3 +162,48 @@ impl<'reporter, R: Reporter> RustupToolchainCheck<'reporter, R> {
         Ok(())
     }
 }
+
+/// If we manually specify the path to a crate (e.g. with --manifest-path or --path),
+/// we must supply the custom directory to our Command runner.
+fn current_dir_crate_path<'c>(config: &'c Config<'c>) -> TResult<Option<&'c Path>> {
+    if config.crate_path().is_some() || config.manifest_path().is_some() {
+        config.context().crate_root_path().map(Some)
+    } else {
+        Ok(None)
+    }
+}
+
+#[cfg(test)]
+mod current_dir_crate_path_tests {
+    use super::*;
+    use crate::config::ConfigBuilder;
+    use crate::Action;
+
+    #[test]
+    fn relative_manifest_path() {
+        let config = ConfigBuilder::new(Action::Verify, "")
+            .manifest_path(Some("Cargo.toml"))
+            .build();
+
+        let res = current_dir_crate_path(&config).unwrap().unwrap();
+        assert!(res.file_name().is_none())
+    }
+
+    #[test]
+    fn relative_crate_path() {
+        let config = ConfigBuilder::new(Action::Verify, "")
+            .crate_path(Some("home"))
+            .build();
+
+        let res = current_dir_crate_path(&config).unwrap().unwrap();
+        assert!(res.file_name().is_some())
+    }
+
+    #[test]
+    fn no_paths() {
+        let config = ConfigBuilder::new(Action::Verify, "").build();
+
+        let res = current_dir_crate_path(&config).unwrap();
+        assert!(res.is_none())
+    }
+}

src/cli.rs

@@ -191,6 +191,7 @@ impl<'opts> TryFrom<&'opts CargoMsrvOpts> for Config<'opts> {
 
         builder = configurators::CustomCheckCommand::configure(builder, opts)?;
         builder = configurators::PathConfig::configure(builder, opts)?;
+        builder = configurators::ManifestPathConfig::configure(builder, opts)?;
         builder = configurators::Target::configure(builder, opts)?;
         builder = configurators::MinVersion::configure(builder, opts)?;
         builder = configurators::MaxVersion::configure(builder, opts)?;

src/cli/configurators.rs

@@ -5,6 +5,7 @@ use crate::TResult;
 mod check_feedback;
 mod custom_check;
 mod ignore_lockfile;
+mod manifest_path;
 mod max_version;
 mod min_version;
 mod output_toolchain_file;
@@ -21,6 +22,7 @@ mod write_msrv;
 pub(in crate::cli) use check_feedback::CheckFeedback;
 pub(in crate::cli) use custom_check::CustomCheckCommand;
 pub(in crate::cli) use ignore_lockfile::IgnoreLockfile;
+pub(in crate::cli) use manifest_path::ManifestPathConfig;
 pub(in crate::cli) use max_version::MaxVersion;
 pub(in crate::cli) use min_version::MinVersion;
 pub(in crate::cli) use output_toolchain_file::OutputToolchainFile;

src/cli/configurators/manifest_path.rs

@@ -0,0 +1,17 @@
+use crate::cli::configurators::Configure;
+use crate::cli::CargoMsrvOpts;
+use crate::config::ConfigBuilder;
+use crate::TResult;
+
+pub(in crate::cli) struct ManifestPathConfig;
+
+impl Configure for ManifestPathConfig {
+    fn configure<'c>(
+        builder: ConfigBuilder<'c>,
+        opts: &'c CargoMsrvOpts,
+    ) -> TResult<ConfigBuilder<'c>> {
+        let path = opts.shared_opts.manifest_path.as_ref();
+
+        Ok(builder.manifest_path(path))
+    }
+}

src/cli/shared_opts.rs

@@ -2,17 +2,23 @@ use crate::config::{OutputFormat, TracingTargetOption};
 
 use crate::log_level::LogLevel;
 use clap::AppSettings;
+use clap::ArgGroup;
 use clap::Args;
 use std::path::PathBuf;
 
 // Cli Options shared between subcommands
 #[derive(Debug, Args)]
 #[clap(setting = AppSettings::DeriveDisplayOrder)]
+#[clap(group(ArgGroup::new("paths").args(&["path", "manifest-path"])))]
 pub struct SharedOpts {
     /// Path to cargo project directory
-    #[clap(long, value_name = "DIR", global = true)]
+    #[clap(long, value_name = "Crate Directory", global = true)]
     pub path: Option<PathBuf>,
 
+    /// Path to cargo manifest file
+    #[clap(long, value_name = "Cargo Manifest", global = true)]
+    pub manifest_path: Option<PathBuf>,
+
     #[clap(flatten)]
     pub user_output_opts: UserOutputOpts,
 

src/config.rs

@@ -215,6 +215,7 @@ pub struct Config<'a> {
     target: String,
     check_command: Vec<&'a str>,
     crate_path: Option<PathBuf>,
+    manifest_path: Option<PathBuf>,
     include_all_patch_releases: bool,
     minimum_version: Option<bare_version::BareVersion>,
     maximum_version: Option<bare_version::BareVersion>,
@@ -239,6 +240,7 @@ impl<'a> Config<'a> {
             target: target.into(),
             check_command: vec!["cargo", "check"],
             crate_path: None,
+            manifest_path: None,
             include_all_patch_releases: false,
             minimum_version: None,
             maximum_version: None,
@@ -272,10 +274,16 @@ impl<'a> Config<'a> {
         self.check_command.join(" ")
     }
 
+    /// Should not be used directly. Use the context instead.
     pub fn crate_path(&self) -> Option<&Path> {
         self.crate_path.as_deref()
     }
 
+    /// Should not be used directly. Use the context instead.
+    pub fn manifest_path(&self) -> Option<&Path> {
+        self.manifest_path.as_deref()
+    }
+
     pub fn include_all_patch_releases(&self) -> bool {
         self.include_all_patch_releases
     }
@@ -379,6 +387,11 @@ impl<'a> ConfigBuilder<'a> {
         self
     }
 
+    pub fn manifest_path<P: AsRef<Path>>(mut self, path: Option<P>) -> Self {
+        self.inner.manifest_path = path.map(|p| PathBuf::from(p.as_ref()));
+        self
+    }
+
     pub fn get_crate_path(&self) -> Option<&Path> {
         self.inner.crate_path.as_deref()
     }

src/ctx.rs

@@ -1,6 +1,7 @@
 use crate::error::IoErrorSource;
 use crate::{CargoMSRVError, Config, TResult};
 use once_cell::unsync::OnceCell;
+use std::ffi::OsStr;
 use std::path::{Path, PathBuf};
 
 /// Context which provides a way to lazily initialize values.
@@ -60,19 +61,64 @@ impl LazyContext {
 
 #[derive(Debug, Clone)]
 pub struct ContextValues {
-    path: Option<PathBuf>,
+    path: GivenPath,
 }
 
 impl ContextValues {
     pub fn from_config(config: &Config) -> Self {
+        let path = if let Some(p) = config.crate_path() {
+            GivenPath::new_crate_root(p)
+        } else if let Some(p) = config.manifest_path() {
+            GivenPath::new_manifest(p)
+        } else {
+            GivenPath::new_none()
+        };
+
         Self {
             // Cloning the crate path here saves us a lot of trouble.
             // However, the real problem is that the Config is supplied through the whole program,
             // and that the context is held by the config, while it also requires values from the
             // config to determine its path.
             // Here we choose to be pragmatic, but hopefully one day we'll get to refactoring the
             // Config and LazyContext.
-            path: config.crate_path().map(|it| it.to_path_buf()),
+            path,
+        }
+    }
+}
+
+#[derive(Debug, Clone)]
+enum GivenPath {
+    CrateRoot(PathBuf),
+    Manifest(PathBuf),
+    None,
+}
+
+impl GivenPath {
+    pub fn new_crate_root<P: AsRef<Path>>(path: P) -> Self {
+        Self::CrateRoot(path.as_ref().to_path_buf())
+    }
+
+    pub fn new_manifest<P: AsRef<Path>>(path: P) -> Self {
+        Self::Manifest(path.as_ref().to_path_buf())
+    }
+
+    pub fn new_none() -> Self {
+        Self::None
+    }
+
+    pub fn as_crate_root(&self) -> TResult<PathBuf> {
+        match self {
+            Self::CrateRoot(p) => Ok(p.clone()),
+            // When in a crate root, an argument "Cargo.toml" will fail, because it doesn't have a
+            // parent, when the path is relative.
+            Self::Manifest(p) if p.file_name() == Some(OsStr::new("Cargo.toml")) => {
+                Ok(Path::new(".").to_path_buf())
+            }
+            Self::Manifest(p) => Ok(p.parent().unwrap().to_path_buf()),
+            Self::None => std::env::current_dir().map_err(|error| CargoMSRVError::Io {
+                error,
+                source: IoErrorSource::CurrentDir,
+            }),
         }
     }
 }
@@ -86,18 +132,9 @@ struct GlobalContext {
 
 impl GlobalContext {
     pub fn crate_root_path(&self) -> TResult<&Path> {
-        fn crate_root(path: Option<&Path>) -> TResult<PathBuf> {
-            path.map(|p| Ok(p.to_path_buf())).unwrap_or_else(|| {
-                std::env::current_dir().map_err(|error| CargoMSRVError::Io {
-                    error,
-                    source: IoErrorSource::CurrentDir,
-                })
-            })
-        }
-
         let path = self
             .crate_root_path
-            .get_or_try_init(|| crate_root(self.values.path.as_deref()))?;
+            .get_or_try_init(|| self.values.path.as_crate_root())?;
 
         Ok(path)
     }

tests/verify_msrv.rs

@@ -195,3 +195,23 @@ fn verify_with_rust_version_opt() {
 
     assert!(result.is_ok());
 }
+
+#[test]
+fn manifest_path() {
+    let version = "1.36.0";
+    let folder = fixtures_path().join(version).join("Cargo.toml");
+    let with_args = vec![
+        "cargo",
+        "msrv",
+        "--manifest-path",
+        folder.to_str().unwrap(),
+        "verify",
+    ];
+
+    let result = run_verify(
+        with_args,
+        vec![Release::new_stable(semver::Version::new(1, 36, 0))],
+    );
+
+    assert!(result.is_ok());
+}