From 7c100e773b38dbd0c6c0f5bfa2a6ff12df659f35 Mon Sep 17 00:00:00 2001 From: Martijn Gribnau Date: Fri, 18 Oct 2024 22:42:14 +0200 Subject: [PATCH 1/8] Report the CargoWorkspace event The event contains the names (NB: not the package id's) of packages which were detected in the current cargo workspace. Possibly must be sent optionally in the future, because we should also support non cargo workspaces. --- src/context.rs | 21 +++++++++++++++++++-- src/lib.rs | 5 ++++- src/reporter/event.rs | 5 +++++ src/reporter/event/cargo_workspace.rs | 19 +++++++++++++++++++ 4 files changed, 47 insertions(+), 3 deletions(-) create mode 100644 src/reporter/event/cargo_workspace.rs diff --git a/src/context.rs b/src/context.rs index ed06778f..b2154ca6 100644 --- a/src/context.rs +++ b/src/context.rs @@ -74,6 +74,16 @@ impl Context { } } + pub fn environment_context(&self) -> &EnvironmentContext { + match self { + Context::Find(ctx) => &ctx.environment, + Context::List(ctx) => &ctx.environment, + Context::Set(ctx) => &ctx.environment, + Context::Show(ctx) => &ctx.environment, + Context::Verify(ctx) => &ctx.environment, + } + } + /// Returns the inner find context, if it was present. pub fn to_find_context(self) -> Option { if let Self::Find(ctx) = self { @@ -314,15 +324,22 @@ impl EnvironmentContext { #[derive(Clone, Debug)] pub struct WorkspacePackages { - _selected: Vec, + selected: Vec, } impl WorkspacePackages { pub fn from_iter(selected: impl IntoIterator) -> Self { Self { - _selected: selected.into_iter().collect(), + selected: selected.into_iter().collect(), } } + + pub fn names(&self) -> Vec { + self.selected + .iter() + .map(|pkg| pkg.name.to_string()) + .collect() + } } #[derive(Clone, Copy, Debug, Default, PartialEq, ValueEnum)] diff --git a/src/lib.rs b/src/lib.rs index 6a3c89d0..803ce8e8 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -27,7 +27,7 @@ pub use crate::sub_command::{Find, List, Set, Show, SubCommand, Verify}; use crate::check::RustupToolchainCheck; use crate::context::ReleaseSource; use crate::error::{CargoMSRVError, TResult}; -use crate::reporter::event::{Meta, SubcommandInit}; +use crate::reporter::event::{CargoWorkspace, Meta, SubcommandInit}; use crate::reporter::{Event, Reporter}; use rust_releases::semver; @@ -60,6 +60,9 @@ pub(crate) mod writer; pub fn run_app(ctx: &Context, reporter: &impl Reporter) -> TResult<()> { reporter.report_event(Meta::default())?; + reporter.report_event(CargoWorkspace::new( + ctx.environment_context().workspace_packages.names(), + ))?; reporter.report_event(SubcommandInit::new(ctx.reporting_name()))?; match ctx { diff --git a/src/reporter/event.rs b/src/reporter/event.rs index 07eebb7c..bad4dbbb 100644 --- a/src/reporter/event.rs +++ b/src/reporter/event.rs @@ -11,6 +11,7 @@ pub use scope::TestScopeGenerator; pub use auxiliary_output::{ AuxiliaryOutput, Destination, Item as AuxiliaryOutputItem, MsrvKind, ToolchainFileKind, }; +pub use cargo_workspace::CargoWorkspace; pub use check_method::{CheckMethod, Method}; pub use check_result::CheckResult; pub use check_toolchain::CheckToolchain; @@ -41,6 +42,7 @@ mod types; // specific events mod auxiliary_output; +mod cargo_workspace; mod check_method; mod check_result; mod check_toolchain; @@ -123,6 +125,9 @@ pub enum Message { // setup Meta(Meta), + // package selection + CargoWorkspace(CargoWorkspace), + // get rust-releases index FetchIndex(FetchIndex), // todo! UnableToConfirmValidReleaseVersion(UnableToConfirmValidReleaseVersion), diff --git a/src/reporter/event/cargo_workspace.rs b/src/reporter/event/cargo_workspace.rs new file mode 100644 index 00000000..7d73df37 --- /dev/null +++ b/src/reporter/event/cargo_workspace.rs @@ -0,0 +1,19 @@ +use crate::reporter::{Event, Message}; + +#[derive(Clone, Debug, Eq, PartialEq, serde::Serialize)] +#[serde(rename_all = "snake_case")] +pub struct CargoWorkspace { + package_names: Vec, +} + +impl CargoWorkspace { + pub fn new(package_names: Vec) -> Self { + Self { package_names } + } +} + +impl From for Event { + fn from(it: CargoWorkspace) -> Self { + Message::CargoWorkspace(it).into() + } +} From 9cd8bc644190ee54892bcd7f7f7c1035bc7dc717 Mon Sep 17 00:00:00 2001 From: Martijn Gribnau Date: Sat, 19 Oct 2024 02:19:42 +0200 Subject: [PATCH 2/8] Only use the selection of workspace packages if cargo metadata succeeded This works around some issues like: - cargo metadata only works for cargo projects, which is fine for most of our defaults, but we only hard require rustup right now for compatibility check commands like find and verify - an unwrap panic from within cargo metadata if it was given an invalid path Yet, the current choice is a pragmatic one and may not account for all cases or handle all errors. --- src/context.rs | 73 ++++++++++++++++++++----- src/lib.rs | 6 +- src/reporter/event.rs | 6 +- src/reporter/event/cargo_workspace.rs | 19 ------- src/reporter/event/selected_packages.rs | 28 ++++++++++ 5 files changed, 93 insertions(+), 39 deletions(-) delete mode 100644 src/reporter/event/cargo_workspace.rs create mode 100644 src/reporter/event/selected_packages.rs diff --git a/src/context.rs b/src/context.rs index b2154ca6..f69d630f 100644 --- a/src/context.rs +++ b/src/context.rs @@ -30,6 +30,7 @@ use crate::cli::rust_releases_opts::Edition; use crate::cli::{CargoMsrvOpts, SubCommand}; use crate::default_target::default_target; use crate::log_level::LogLevel; +use crate::reporter::event::SelectedPackage; pub use find::FindContext; pub use list::ListContext; pub use set::SetContext; @@ -288,13 +289,36 @@ impl<'shared_opts> TryFrom<&'shared_opts SharedOpts> for EnvironmentContext { CargoMSRVError::Path(PathError::InvalidUtf8(InvalidUtf8Error::from(err))) })?; - let metadata = cargo_metadata::MetadataCommand::new() + // Only select packages if this is a Cargo project. + // For now, to be pragmatic, we'll take a shortcut and say that it is so, + // if the cargo metadata command succeeds. If it doesn't, we'll fall + // back to just the default package. + let workspace_packages = if let Ok(metadata) = cargo_metadata::MetadataCommand::new() .manifest_path(root_crate_path.join("Cargo.toml")) - .exec()?; - let workspace = opts.workspace.partition_packages(&metadata); - let workspace_packages = WorkspacePackages::from_iter(workspace.0.into_iter().cloned()); + .exec() + { + let partition = opts.workspace.partition_packages(&metadata); + let selected = partition.0.into_iter().cloned().collect(); + let excluded = partition.1; + + info!( + action = "detect_cargo_workspace_packages", + method = "cargo_metadata", + success = true, + ?selected, + ?excluded + ); + + WorkspacePackages::from_vec(selected) + } else { + info!( + action = "detect_cargo_workspace_packages", + method = "cargo_metadata", + success = false, + ); - info!(?workspace_packages, workspace_packages_excluded = ?workspace.1); + WorkspacePackages::default() + }; Ok(Self { root_crate_path, @@ -322,23 +346,44 @@ impl EnvironmentContext { // --- -#[derive(Clone, Debug)] +#[derive(Clone, Debug, Default)] pub struct WorkspacePackages { - selected: Vec, + selected: Option>, } impl WorkspacePackages { - pub fn from_iter(selected: impl IntoIterator) -> Self { + pub fn from_vec(selected: Vec) -> Self { Self { - selected: selected.into_iter().collect(), + selected: Some(selected), } } - pub fn names(&self) -> Vec { - self.selected - .iter() - .map(|pkg| pkg.name.to_string()) - .collect() + pub fn selected(&self) -> Option> { + self.selected.as_deref().map(|pks| { + pks.iter() + .map(|pkg| SelectedPackage { + name: pkg.name.to_string(), + path: pkg.manifest_path.to_path_buf(), + }) + .collect() + }) + } + + /// The default package is used when either: + /// 1. No packages were selected (e.g. because we are not in a cargo workspace or do not use cargo) + /// 2. No workspace flags like --workspace, --package, --all or --exclude are used + /// + /// See [clap_cargo::Workspace](https://docs.rs/clap-cargo/latest/clap_cargo/struct.Workspace.html) which is + /// currently used for the selection. + pub fn use_default_package(&self) -> bool { + self.selected_packages().is_empty() + } + + /// The slice of selected packages. + /// If empty, either no workspace selection flag was used, or cargo_metadata failed, + /// for example because it wasn't a cargo workspace. + pub fn selected_packages(&self) -> &[cargo_metadata::Package] { + self.selected.as_deref().unwrap_or_default() } } diff --git a/src/lib.rs b/src/lib.rs index 803ce8e8..f647c0e7 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -27,7 +27,7 @@ pub use crate::sub_command::{Find, List, Set, Show, SubCommand, Verify}; use crate::check::RustupToolchainCheck; use crate::context::ReleaseSource; use crate::error::{CargoMSRVError, TResult}; -use crate::reporter::event::{CargoWorkspace, Meta, SubcommandInit}; +use crate::reporter::event::{Meta, SelectedPackages, SubcommandInit}; use crate::reporter::{Event, Reporter}; use rust_releases::semver; @@ -60,8 +60,8 @@ pub(crate) mod writer; pub fn run_app(ctx: &Context, reporter: &impl Reporter) -> TResult<()> { reporter.report_event(Meta::default())?; - reporter.report_event(CargoWorkspace::new( - ctx.environment_context().workspace_packages.names(), + reporter.report_event(SelectedPackages::new( + ctx.environment_context().workspace_packages.selected(), ))?; reporter.report_event(SubcommandInit::new(ctx.reporting_name()))?; diff --git a/src/reporter/event.rs b/src/reporter/event.rs index bad4dbbb..863e554e 100644 --- a/src/reporter/event.rs +++ b/src/reporter/event.rs @@ -11,7 +11,6 @@ pub use scope::TestScopeGenerator; pub use auxiliary_output::{ AuxiliaryOutput, Destination, Item as AuxiliaryOutputItem, MsrvKind, ToolchainFileKind, }; -pub use cargo_workspace::CargoWorkspace; pub use check_method::{CheckMethod, Method}; pub use check_result::CheckResult; pub use check_toolchain::CheckToolchain; @@ -19,6 +18,7 @@ pub use fetch_index::FetchIndex; pub use meta::Meta; pub use progress::Progress; pub use search_method::FindMsrv; +pub use selected_packages::{SelectedPackage, SelectedPackages}; pub use setup_toolchain::SetupToolchain; pub use subcommand_init::SubcommandInit; pub use subcommand_result::SubcommandResult; @@ -42,7 +42,6 @@ mod types; // specific events mod auxiliary_output; -mod cargo_workspace; mod check_method; mod check_result; mod check_toolchain; @@ -50,6 +49,7 @@ mod fetch_index; mod meta; mod progress; mod search_method; +mod selected_packages; mod setup_toolchain; mod subcommand_init; mod subcommand_result; @@ -126,7 +126,7 @@ pub enum Message { Meta(Meta), // package selection - CargoWorkspace(CargoWorkspace), + SelectedPackages(SelectedPackages), // get rust-releases index FetchIndex(FetchIndex), // todo! diff --git a/src/reporter/event/cargo_workspace.rs b/src/reporter/event/cargo_workspace.rs deleted file mode 100644 index 7d73df37..00000000 --- a/src/reporter/event/cargo_workspace.rs +++ /dev/null @@ -1,19 +0,0 @@ -use crate::reporter::{Event, Message}; - -#[derive(Clone, Debug, Eq, PartialEq, serde::Serialize)] -#[serde(rename_all = "snake_case")] -pub struct CargoWorkspace { - package_names: Vec, -} - -impl CargoWorkspace { - pub fn new(package_names: Vec) -> Self { - Self { package_names } - } -} - -impl From for Event { - fn from(it: CargoWorkspace) -> Self { - Message::CargoWorkspace(it).into() - } -} diff --git a/src/reporter/event/selected_packages.rs b/src/reporter/event/selected_packages.rs new file mode 100644 index 00000000..af47c48c --- /dev/null +++ b/src/reporter/event/selected_packages.rs @@ -0,0 +1,28 @@ +use crate::reporter::{Event, Message}; +use camino::Utf8PathBuf; + +/// Workspace packages selected +#[derive(Clone, Debug, Eq, PartialEq, serde::Serialize)] +#[serde(rename_all = "snake_case")] +pub struct SelectedPackages { + package_names: Option>, +} + +impl SelectedPackages { + pub fn new(package_names: Option>) -> Self { + Self { package_names } + } +} + +impl From for Event { + fn from(it: SelectedPackages) -> Self { + Message::SelectedPackages(it).into() + } +} + +#[derive(Clone, Debug, Eq, PartialEq, serde::Serialize)] +#[serde(rename_all = "snake_case")] +pub struct SelectedPackage { + pub name: String, + pub path: Utf8PathBuf, +} From 86fd24f982c3e78a8c76e2aca610ccf39ddc9a90 Mon Sep 17 00:00:00 2001 From: Martijn Gribnau Date: Sat, 19 Oct 2024 02:26:36 +0200 Subject: [PATCH 3/8] Add "rustc" only fixture --- tests/fixtures/rustc/hello.rs | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 tests/fixtures/rustc/hello.rs diff --git a/tests/fixtures/rustc/hello.rs b/tests/fixtures/rustc/hello.rs new file mode 100644 index 00000000..301e8d80 --- /dev/null +++ b/tests/fixtures/rustc/hello.rs @@ -0,0 +1,3 @@ +fn main() { + dbg!("Hello world!"); +} From 9340b0a5541b81d0571b5e8baf1ef2794c7d1ea2 Mon Sep 17 00:00:00 2001 From: Martijn Gribnau Date: Sat, 19 Oct 2024 02:36:46 +0200 Subject: [PATCH 4/8] Add clarifying note about Cargo to environment context --- src/context.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/context.rs b/src/context.rs index f69d630f..b7405361 100644 --- a/src/context.rs +++ b/src/context.rs @@ -246,6 +246,8 @@ impl From for CheckCommandContext { #[derive(Clone, Debug)] pub struct EnvironmentContext { + // TODO: Some parts assume a Cargo crate, but that's not strictly a requirement + // of cargo-msrv (only rustup is). We should fix this. /// The path to the root of a crate. /// /// Does not include a manifest file like Cargo.toml, so it's easy to append From ca6a38a40aeecf0309a72ce962c0eebc8a8877d9 Mon Sep 17 00:00:00 2001 From: Martijn Gribnau Date: Sat, 19 Oct 2024 02:42:44 +0200 Subject: [PATCH 5/8] Clarify CLI help on --path vs --manifest-path --- src/cli/shared_opts.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/cli/shared_opts.rs b/src/cli/shared_opts.rs index dab1e840..9cdb0813 100644 --- a/src/cli/shared_opts.rs +++ b/src/cli/shared_opts.rs @@ -7,7 +7,10 @@ use std::path::PathBuf; #[derive(Debug, Args)] #[command(group(ArgGroup::new("paths").args(&["path", "manifest_path"])))] pub struct SharedOpts { - /// Path to cargo project directory + /// Path to project root directory + /// + /// This should be used over `--manifest-path` if not in a Cargo project. + /// If you have a Cargo project, prefer `--manifest-path`. #[arg(long, value_name = "Crate Directory", global = true, value_hint = ValueHint::DirPath)] pub path: Option, From 83009bfd329d305d2bed6d6f976fc0bdb52a11e0 Mon Sep 17 00:00:00 2001 From: Martijn Gribnau Date: Sat, 19 Oct 2024 02:43:24 +0200 Subject: [PATCH 6/8] Use Try operator for SharedOpts into EnvironmentContext --- src/context/show.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/context/show.rs b/src/context/show.rs index d795155e..ddbd2b31 100644 --- a/src/context/show.rs +++ b/src/context/show.rs @@ -16,7 +16,7 @@ impl TryFrom for ShowContext { let CargoMsrvOpts { shared_opts, .. } = opts; Ok(Self { - environment: (&shared_opts).try_into().unwrap(), // todo! + environment: (&shared_opts).try_into()?, }) } } From 13ee5fd8454080424c1722339db20f02630b6f87 Mon Sep 17 00:00:00 2001 From: Martijn Gribnau Date: Sat, 19 Oct 2024 02:45:57 +0200 Subject: [PATCH 7/8] Add changelog entry for workspace package flags --- CHANGELOG.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 69841732..b12a67c9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,11 @@ the [discussions section](https://github.com/foresterre/cargo-msrv/discussions). ## Unreleased +### Added + +* Added `--workspace`, `--all`, `--package` and `--exclude` CLI options for package selection when in a Cargo project ( + with limited workspace support for now) + ## [0.16.2] - 2024-10-10 ### Fixed From ac2ce778d0d529e6a39745bef70cd2e175be689c Mon Sep 17 00:00:00 2001 From: Martijn Gribnau Date: Sat, 19 Oct 2024 02:58:01 +0200 Subject: [PATCH 8/8] Fix tests which still relied on WorkspacePackages::from_iter --- src/sub_command/find/tests.rs | 2 +- src/writer/write_msrv.rs | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/sub_command/find/tests.rs b/src/sub_command/find/tests.rs index 4a46b8f1..e041244f 100644 --- a/src/sub_command/find/tests.rs +++ b/src/sub_command/find/tests.rs @@ -265,7 +265,7 @@ fn create_test_context() -> FindContext { }, environment: EnvironmentContext { root_crate_path: Utf8PathBuf::new(), - workspace_packages: WorkspacePackages::from_iter([]), + workspace_packages: WorkspacePackages::default(), }, } } diff --git a/src/writer/write_msrv.rs b/src/writer/write_msrv.rs index 84b8feb7..bd3b5d1b 100644 --- a/src/writer/write_msrv.rs +++ b/src/writer/write_msrv.rs @@ -52,7 +52,7 @@ mod tests { let env = EnvironmentContext { root_crate_path: root.to_path_buf(), - workspace_packages: WorkspacePackages::from_iter([]), + workspace_packages: WorkspacePackages::default(), }; let index = ReleaseIndex::from_iter(vec![rust_releases::Release::new_stable( @@ -87,7 +87,7 @@ mod tests { let env = EnvironmentContext { root_crate_path: root.to_path_buf(), - workspace_packages: WorkspacePackages::from_iter([]), + workspace_packages: WorkspacePackages::default(), }; let index = ReleaseIndex::from_iter(vec![]); @@ -119,7 +119,7 @@ mod tests { let env = EnvironmentContext { root_crate_path: root.to_path_buf(), - workspace_packages: WorkspacePackages::from_iter([]), + workspace_packages: WorkspacePackages::default(), }; write_msrv(