From 8dcc4666c5da3f427e8c56849d4f8bedca9859f0 Mon Sep 17 00:00:00 2001 From: Martijn Gribnau Date: Sun, 24 Jul 2022 16:44:06 +0200 Subject: [PATCH 1/4] Allow using manifest-path instead of crate path --- src/cli.rs | 1 + src/cli/configurators.rs | 2 + src/cli/configurators/manifest_path.rs | 17 ++++++++ src/cli/shared_opts.rs | 8 +++- src/config.rs | 11 ++++++ src/ctx.rs | 55 ++++++++++++++++++++------ 6 files changed, 81 insertions(+), 13 deletions(-) create mode 100644 src/cli/configurators/manifest_path.rs diff --git a/src/cli.rs b/src/cli.rs index 1642c085..0b5c299e 100644 --- a/src/cli.rs +++ b/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)?; diff --git a/src/cli/configurators.rs b/src/cli/configurators.rs index 306eebec..6eb5a8f4 100644 --- a/src/cli/configurators.rs +++ b/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; diff --git a/src/cli/configurators/manifest_path.rs b/src/cli/configurators/manifest_path.rs new file mode 100644 index 00000000..76770326 --- /dev/null +++ b/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> { + let path = opts.shared_opts.manifest_path.as_ref(); + + Ok(builder.manifest_path(path)) + } +} diff --git a/src/cli/shared_opts.rs b/src/cli/shared_opts.rs index 15fabd52..522edce2 100644 --- a/src/cli/shared_opts.rs +++ b/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, + /// Path to cargo manifest file + #[clap(long, value_name = "Cargo Manifest", global = true)] + pub manifest_path: Option, + #[clap(flatten)] pub user_output_opts: UserOutputOpts, diff --git a/src/config.rs b/src/config.rs index 5009092d..1e347d0a 100644 --- a/src/config.rs +++ b/src/config.rs @@ -215,6 +215,7 @@ pub struct Config<'a> { target: String, check_command: Vec<&'a str>, crate_path: Option, + manifest_path: Option, include_all_patch_releases: bool, minimum_version: Option, maximum_version: Option, @@ -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, @@ -276,6 +278,10 @@ impl<'a> Config<'a> { self.crate_path.as_deref() } + 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 +385,11 @@ impl<'a> ConfigBuilder<'a> { self } + pub fn manifest_path>(mut self, path: Option

) -> 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() } diff --git a/src/ctx.rs b/src/ctx.rs index 823b111d..231ccbf4 100644 --- a/src/ctx.rs +++ b/src/ctx.rs @@ -60,11 +60,19 @@ impl LazyContext { #[derive(Debug, Clone)] pub struct ContextValues { - path: Option, + 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, @@ -72,7 +80,39 @@ impl ContextValues { // 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>(path: P) -> Self { + Self::CrateRoot(path.as_ref().to_path_buf()) + } + + pub fn new_manifest>(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 { + match self { + Self::CrateRoot(p) => Ok(p.clone()), + 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 +126,9 @@ struct GlobalContext { impl GlobalContext { pub fn crate_root_path(&self) -> TResult<&Path> { - fn crate_root(path: Option<&Path>) -> TResult { - 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) } From 47ec905c4c53e713cd99e36fa506107fa1c0e9e6 Mon Sep 17 00:00:00 2001 From: Martijn Gribnau Date: Sun, 24 Jul 2022 16:58:05 +0200 Subject: [PATCH 2/4] Update changelog for --manifest-path --- CHANGELOG.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cfe8b535..28804cfd 100644 --- a/CHANGELOG.md +++ b/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 ` 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 From 20a6cf3020bad45418699c728ed928f2cd680834 Mon Sep 17 00:00:00 2001 From: Martijn Gribnau Date: Sun, 24 Jul 2022 17:20:58 +0200 Subject: [PATCH 3/4] Fix set current dir for rustup commands when specifying the manifest-path --- src/check/rustup_toolchain_check.rs | 18 +++++++++++++----- src/config.rs | 2 ++ 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/src/check/rustup_toolchain_check.rs b/src/check/rustup_toolchain_check.rs index 6eb87c18..8d80f694 100644 --- a/src/check/rustup_toolchain_check.rs +++ b/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,13 @@ 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> { + if config.crate_path().is_some() || config.manifest_path().is_some() { + config.context().crate_root_path().map(Some) + } else { + Ok(None) + } +} diff --git a/src/config.rs b/src/config.rs index 1e347d0a..f02b2766 100644 --- a/src/config.rs +++ b/src/config.rs @@ -274,10 +274,12 @@ 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() } From 2a800f93f9ba77ac98f148991d551498a46656bf Mon Sep 17 00:00:00 2001 From: Martijn Gribnau Date: Sun, 24 Jul 2022 20:04:05 +0200 Subject: [PATCH 4/4] Fix case where unrooted relative manifest-path would fail The program would fail because an unrooted relative manifest-path does not have a parent. We rely on the Path::parent() method to find the crate directory. The fix first canonicalizes the path. This assumes no cargo crate is rooted at the top of a file system. Possibly, if you mount a crate to a separate file system, it will still fail, but it is assumed that this is not the common case. If we need to support such a case, we no longer can use the Path::parent() method. We may check if manifest_path == "Cargo.toml", and then accept "" or "." as crate root. --- src/check/rustup_toolchain_check.rs | 35 +++++++++++++++++++++++++++++ src/ctx.rs | 6 +++++ tests/verify_msrv.rs | 20 +++++++++++++++++ 3 files changed, 61 insertions(+) diff --git a/src/check/rustup_toolchain_check.rs b/src/check/rustup_toolchain_check.rs index 8d80f694..7613389e 100644 --- a/src/check/rustup_toolchain_check.rs +++ b/src/check/rustup_toolchain_check.rs @@ -172,3 +172,38 @@ fn current_dir_crate_path<'c>(config: &'c Config<'c>) -> TResult TResult { 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, diff --git a/tests/verify_msrv.rs b/tests/verify_msrv.rs index 6b642ab0..323d257b 100644 --- a/tests/verify_msrv.rs +++ b/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()); +}