diff --git a/CHANGELOG.md b/CHANGELOG.md index edc8981b..33520b56 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,10 @@ Note: In this file, do not use the hard wrap in the middle of a sentence for com ## [Unreleased] +- Fix handling of weak dependency features when namespaced features is not used together. + +- Improve performance by passing `--no-deps` to `cargo metadata` except when using `--include-deps-features`. + ## [0.6.10] - 2023-10-17 - Fix compatibility with old Cargo. This fixes a regression introduced in 0.6.9. diff --git a/src/context.rs b/src/context.rs index 309d6802..a1352a5c 100644 --- a/src/context.rs +++ b/src/context.rs @@ -10,7 +10,6 @@ use std::{ }; use anyhow::{bail, Result}; -use cargo_config2::Config; use crate::{ cargo, @@ -30,6 +29,7 @@ pub(crate) struct Context { pub(crate) cargo_version: u32, pub(crate) restore: restore::Manager, pub(crate) current_dir: PathBuf, + pub(crate) current_package: Option, } impl Context { @@ -48,20 +48,10 @@ impl Context { .map(|v| v.minor) .unwrap_or(0); - let config = Config::load()?; - let targets = config.build_target_for_cli(&args.target)?; - let host = config.host_triple()?; - // if `--remove-dev-deps` flag is off, restore manifest file. let restore = restore::Manager::new(!args.remove_dev_deps); - let metadata = Metadata::new( - args.manifest_path.as_deref(), - &cargo, - cargo_version, - &targets, - host, - &restore, - )?; + let metadata = + Metadata::new(args.manifest_path.as_deref(), &cargo, cargo_version, &args, &restore)?; if metadata.cargo_version < 41 && args.include_deps_features { bail!("--include-deps-features requires Cargo 1.41 or later"); } @@ -77,6 +67,23 @@ impl Context { pkg_features.insert(id.clone(), features); } + let mut cmd = cmd!(&cargo, "locate-project", "--message-format", "plain"); + if let Some(manifest_path) = &args.manifest_path { + cmd.arg("--manifest-path"); + cmd.arg(manifest_path); + } + let locate_project = &cmd.read()?; + let mut current_package = None; + for id in &metadata.workspace_members { + let manifest_path = &metadata.packages[id].manifest_path; + // no need to use same_file as cargo-metadata and cargo-locate-project + // as they return absolute paths resolved in the same way. + if Path::new(locate_project) == manifest_path { + current_package = Some(id.clone()); + break; + } + } + let this = Self { args, metadata, @@ -86,6 +93,7 @@ impl Context { cargo_version, restore, current_dir: env::current_dir()?, + current_package, }; // TODO: Ideally, we should do this, but for now, we allow it as cargo-hack @@ -110,7 +118,7 @@ impl Context { } pub(crate) fn current_package(&self) -> Option<&PackageId> { - self.metadata.resolve.root.as_ref() + self.current_package.as_ref() } pub(crate) fn workspace_root(&self) -> &Path { diff --git a/src/features.rs b/src/features.rs index 84239d55..e234166c 100644 --- a/src/features.rs +++ b/src/features.rs @@ -22,9 +22,8 @@ impl Features { include_deps_features: bool, ) -> Self { let package = &metadata.packages[id]; - let node = &metadata.resolve.nodes[id]; - let mut features = Vec::with_capacity(package.features.len()); + let mut features: BTreeSet<_> = manifest.features.keys().map(Feature::from).collect(); let mut optional_deps = vec![]; let mut namespaced_features = vec![]; // features with `dep:` prefix @@ -36,14 +35,11 @@ impl Features { } } } - for name in package.optional_deps() { - if !namespaced_features.contains(&name) { - optional_deps.push(name); - } - } - for name in package.features.keys() { - if !optional_deps.contains(&&**name) { - features.push(name.into()); + if namespaced_features.is_empty() { + for name in package.optional_deps() { + if !namespaced_features.contains(&name) { + optional_deps.push(name); + } } } let optional_deps_start = features.len(); @@ -51,6 +47,7 @@ impl Features { let deps_features_start = features.len(); if include_deps_features { + let node = &metadata.resolve.nodes[id]; // TODO: Unpublished dependencies are not included in `node.deps`. for dep in node.deps.iter().filter(|dep| { // ignore if `dep_kinds` is empty (i.e., not Rust 1.41+), target specific or not a normal dependency. @@ -68,7 +65,7 @@ impl Features { } } - Self { features, optional_deps_start, deps_features_start } + Self { features: features.into_iter().collect(), optional_deps_start, deps_features_start } } pub(crate) fn normal(&self) -> &[Feature] { @@ -89,6 +86,7 @@ impl Features { } /// The representation of Cargo feature. +#[derive(PartialEq, Eq, PartialOrd, Ord)] pub(crate) enum Feature { /// A feature of the current crate. Normal { diff --git a/src/metadata.rs b/src/metadata.rs index 0de09d2d..9e6eff01 100644 --- a/src/metadata.rs +++ b/src/metadata.rs @@ -14,9 +14,10 @@ use std::{ }; use anyhow::{format_err, Context as _, Result}; +use cargo_config2::Config; use serde_json::{Map, Value}; -use crate::{cargo, fs, process::ProcessBuilder, restore, term}; +use crate::{cargo, cli::Args, fs, process::ProcessBuilder, restore, term}; type Object = Map; type ParseResult = Result; @@ -52,13 +53,22 @@ impl Metadata { manifest_path: Option<&str>, cargo: &OsStr, mut cargo_version: u32, - targets: &[String], - host: &str, + args: &Args, restore: &restore::Manager, ) -> Result { let stable_cargo_version = cargo::version(cmd!("rustup", "run", "stable", "cargo")).map(|v| v.minor).unwrap_or(0); + let config; + let include_deps_features = if args.include_deps_features { + config = Config::load()?; + let targets = config.build_target_for_cli(&args.target)?; + let host = config.host_triple()?; + Some((targets, host)) + } else { + None + }; + let mut cmd; let append_metadata_args = |cmd: &mut ProcessBuilder<'_>| { cmd.arg("metadata"); @@ -67,14 +77,21 @@ impl Metadata { cmd.arg("--manifest-path"); cmd.arg(manifest_path); } - if targets.is_empty() { - cmd.arg("--filter-platform"); - cmd.arg(host); - } else { - for target in targets { + if let Some((targets, host)) = &include_deps_features { + if targets.is_empty() { cmd.arg("--filter-platform"); - cmd.arg(target); + cmd.arg(host); + } else { + for target in targets { + cmd.arg("--filter-platform"); + cmd.arg(target); + } } + // features-related flags are unneeded for --no-deps. + // TODO: + // cmd.arg("--all-features"); + } else { + cmd.arg("--no-deps"); } }; let json = if stable_cargo_version > cargo_version { @@ -83,7 +100,8 @@ impl Metadata { cmd.arg("--manifest-path"); cmd.arg(manifest_path); } - let no_deps: Object = serde_json::from_str(&cmd.read()?) + let no_deps_raw = cmd.read()?; + let no_deps: Object = serde_json::from_str(&no_deps_raw) .with_context(|| format!("failed to parse output from {cmd}"))?; let lockfile = Path::new(no_deps["workspace_root"].as_str().unwrap()).join("Cargo.lock"); @@ -111,10 +129,14 @@ impl Metadata { json } Err(_e) => { - // If failed, try again with the version of cargo we will actually use. - cmd = cmd!(cargo); - append_metadata_args(&mut cmd); - cmd.read()? + if include_deps_features.is_some() { + // If failed, try again with the version of cargo we will actually use. + cmd = cmd!(cargo); + append_metadata_args(&mut cmd); + cmd.read()? + } else { + no_deps_raw + } } } } else { @@ -143,7 +165,10 @@ impl Metadata { .map(|v| Package::from_value(v, cargo_version)) .collect::>()?, workspace_members, - resolve: Resolve::from_obj(map.remove_object("resolve")?, cargo_version)?, + resolve: match map.remove_nullable("resolve", into_object)? { + Some(resolve) => Resolve::from_obj(resolve, cargo_version)?, + None => Resolve { nodes: HashMap::new() }, + }, workspace_root: map.remove_string("workspace_root")?, }) } @@ -152,10 +177,9 @@ impl Metadata { /// The resolved dependency graph for the entire workspace. pub(crate) struct Resolve { /// Nodes in a dependency graph. + /// + /// This is always empty if cargo-metadata is run with --no-deps. pub(crate) nodes: HashMap, - /// The crate for which the metadata was read. - /// This is `None` if the metadata was read in a virtual workspace. - pub(crate) root: Option, } impl Resolve { @@ -166,7 +190,6 @@ impl Resolve { .into_iter() .map(|v| Node::from_value(v, cargo_version)) .collect::>()?, - root: map.remove_nullable("root", into_string)?, }) } } diff --git a/tests/fixtures/namespaced_features/Cargo.toml b/tests/fixtures/namespaced_features/Cargo.toml index 4725c8c5..7dd6919a 100644 --- a/tests/fixtures/namespaced_features/Cargo.toml +++ b/tests/fixtures/namespaced_features/Cargo.toml @@ -13,5 +13,8 @@ easytime = ["dep:easytime"] [dependencies] # easytime 0.2.6 requires Rust 1.58 easytime = { version = "=0.2.5", optional = true, default-features = false } +# When namespace dependency is used, other optional dependencies are also not +# treated as implicit features. +portable-atomic = { version = "1", optional = true } [dev-dependencies] diff --git a/tests/fixtures/weak_dep_features/Cargo.toml b/tests/fixtures/weak_dep_features/Cargo.toml new file mode 100644 index 00000000..ef7d0d58 --- /dev/null +++ b/tests/fixtures/weak_dep_features/Cargo.toml @@ -0,0 +1,18 @@ +[package] +name = "weak_dep_features" +version = "0.0.0" +edition = "2021" +publish = false + +[workspace] +resolver = "2" + +[features] +easytime = ["easytime/std"] +default = ["easytime?/default"] + +[dependencies] +# easytime 0.2.6 requires Rust 1.58 +easytime = { version = "=0.2.5", optional = true, default-features = false } + +[dev-dependencies] diff --git a/tests/fixtures/weak_dep_features/src/lib.rs b/tests/fixtures/weak_dep_features/src/lib.rs new file mode 100644 index 00000000..8b137891 --- /dev/null +++ b/tests/fixtures/weak_dep_features/src/lib.rs @@ -0,0 +1 @@ + diff --git a/tests/fixtures/weak_dep_features_implicit/Cargo.toml b/tests/fixtures/weak_dep_features_implicit/Cargo.toml new file mode 100644 index 00000000..7b116af2 --- /dev/null +++ b/tests/fixtures/weak_dep_features_implicit/Cargo.toml @@ -0,0 +1,17 @@ +[package] +name = "weak_dep_features_implicit" +version = "0.0.0" +edition = "2021" +publish = false + +[workspace] +resolver = "2" + +[features] +default = ["easytime?/default"] + +[dependencies] +# easytime 0.2.6 requires Rust 1.58 +easytime = { version = "=0.2.5", optional = true, default-features = false } + +[dev-dependencies] diff --git a/tests/fixtures/weak_dep_features_implicit/src/lib.rs b/tests/fixtures/weak_dep_features_implicit/src/lib.rs new file mode 100644 index 00000000..8b137891 --- /dev/null +++ b/tests/fixtures/weak_dep_features_implicit/src/lib.rs @@ -0,0 +1 @@ + diff --git a/tests/fixtures/weak_dep_features_namespaced/Cargo.toml b/tests/fixtures/weak_dep_features_namespaced/Cargo.toml new file mode 100644 index 00000000..1bc497c6 --- /dev/null +++ b/tests/fixtures/weak_dep_features_namespaced/Cargo.toml @@ -0,0 +1,18 @@ +[package] +name = "weak_dep_features_namespaced" +version = "0.0.0" +edition = "2021" +publish = false + +[workspace] +resolver = "2" + +[features] +easytime = ["dep:easytime", "easytime/std"] +default = ["easytime?/default"] + +[dependencies] +# easytime 0.2.6 requires Rust 1.58 +easytime = { version = "=0.2.5", optional = true, default-features = false } + +[dev-dependencies] diff --git a/tests/fixtures/weak_dep_features_namespaced/src/lib.rs b/tests/fixtures/weak_dep_features_namespaced/src/lib.rs new file mode 100644 index 00000000..8b137891 --- /dev/null +++ b/tests/fixtures/weak_dep_features_namespaced/src/lib.rs @@ -0,0 +1 @@ + diff --git a/tests/test.rs b/tests/test.rs index 57d40f90..ea8345b2 100644 --- a/tests/test.rs +++ b/tests/test.rs @@ -1649,6 +1649,70 @@ fn namespaced_features() { ); } +#[test] +fn weak_dep_features() { + cargo_hack(["check", "--feature-powerset"]) + .assert_success2("weak_dep_features", Some(60)) + .stderr_contains( + " + running `cargo check --no-default-features` on weak_dep_features (1/4) + running `cargo check --no-default-features --features default` on weak_dep_features (2/4) + running `cargo check --no-default-features --features easytime` on weak_dep_features (3/4) + running `cargo check --no-default-features --features default,easytime` on weak_dep_features (4/4) + ", + ); + cargo_hack(["check", "--feature-powerset", "--optional-deps"]) + .assert_success2("weak_dep_features", Some(60)) + .stderr_contains( + " + running `cargo check --no-default-features` on weak_dep_features (1/4) + running `cargo check --no-default-features --features default` on weak_dep_features (2/4) + running `cargo check --no-default-features --features easytime` on weak_dep_features (3/4) + running `cargo check --no-default-features --features default,easytime` on weak_dep_features (4/4) + ", + ); + + cargo_hack(["check", "--feature-powerset"]) + .assert_success2("weak_dep_features_namespaced", Some(60)) + .stderr_contains( + " + running `cargo check --no-default-features` on weak_dep_features_namespaced (1/4) + running `cargo check --no-default-features --features default` on weak_dep_features_namespaced (2/4) + running `cargo check --no-default-features --features easytime` on weak_dep_features_namespaced (3/4) + running `cargo check --no-default-features --features default,easytime` on weak_dep_features_namespaced (4/4) + ", + ); + cargo_hack(["check", "--feature-powerset", "--optional-deps"]) + .assert_success2("weak_dep_features_namespaced", Some(60)) + .stderr_contains( + " + running `cargo check --no-default-features` on weak_dep_features_namespaced (1/4) + running `cargo check --no-default-features --features default` on weak_dep_features_namespaced (2/4) + running `cargo check --no-default-features --features easytime` on weak_dep_features_namespaced (3/4) + running `cargo check --no-default-features --features default,easytime` on weak_dep_features_namespaced (4/4) + ", + ); + + cargo_hack(["check", "--feature-powerset"]) + .assert_success2("weak_dep_features_implicit", Some(60)) + .stderr_contains( + " + running `cargo check --no-default-features` on weak_dep_features_implicit (1/2) + running `cargo check --no-default-features --features default` on weak_dep_features_implicit (2/2) + ", + ); + cargo_hack(["check", "--feature-powerset", "--optional-deps"]) + .assert_success2("weak_dep_features_implicit", Some(60)) + .stderr_contains( + " + running `cargo check --no-default-features` on weak_dep_features_implicit (1/4) + running `cargo check --no-default-features --features default` on weak_dep_features_implicit (2/4) + running `cargo check --no-default-features --features easytime` on weak_dep_features_implicit (3/4) + running `cargo check --no-default-features --features default,easytime` on weak_dep_features_implicit (4/4) + ", + ); +} + #[test] fn empty_string() { cargo_hack(["check", "--each-feature", "--skip", ""])