diff --git a/crates/resolver-tests/src/lib.rs b/crates/resolver-tests/src/lib.rs index 01d9b5e6d69..2da540d4117 100644 --- a/crates/resolver-tests/src/lib.rs +++ b/crates/resolver-tests/src/lib.rs @@ -11,7 +11,7 @@ use std::task::Poll; use std::time::Instant; use cargo::core::dependency::DepKind; -use cargo::core::resolver::{self, ResolveOpts, VersionPreferences}; +use cargo::core::resolver::{self, ResolveOpts, ResolveVersion, VersionPreferences}; use cargo::core::source::{GitReference, QueryKind, SourceId}; use cargo::core::Resolve; use cargo::core::{Dependency, PackageId, Registry, Summary}; @@ -196,6 +196,7 @@ pub fn resolve_with_config_raw( &VersionPreferences::default(), Some(config), true, + ResolveVersion::default(), ); // The largest test in our suite takes less then 30 sec. diff --git a/src/cargo/core/resolver/dep_cache.rs b/src/cargo/core/resolver/dep_cache.rs index 4fd27538555..180391dd026 100644 --- a/src/cargo/core/resolver/dep_cache.rs +++ b/src/cargo/core/resolver/dep_cache.rs @@ -13,8 +13,8 @@ use crate::core::resolver::context::Context; use crate::core::resolver::errors::describe_path_in_context; use crate::core::resolver::types::{ConflictReason, DepInfo, FeaturesSet}; use crate::core::resolver::{ - ActivateError, ActivateResult, CliFeatures, RequestedFeatures, ResolveOpts, VersionOrdering, - VersionPreferences, + ActivateError, ActivateResult, CliFeatures, RequestedFeatures, ResolveOpts, ResolveVersion, + VersionOrdering, VersionPreferences, }; use crate::core::{ Dependency, FeatureValue, PackageId, PackageIdSpec, QueryKind, Registry, Summary, @@ -230,6 +230,7 @@ impl<'a> RegistryQueryer<'a> { candidate: &Summary, opts: &ResolveOpts, first_minimal_version: bool, + version: ResolveVersion, ) -> ActivateResult, Rc>)>> { // if we have calculated a result before, then we can just return it, // as it is a "pure" query of its arguments. @@ -242,7 +243,7 @@ impl<'a> RegistryQueryer<'a> { // First, figure out our set of dependencies based on the requested set // of features. This also calculates what features we're going to enable // for our own dependencies. - let (used_features, deps) = resolve_features(parent, candidate, opts)?; + let (used_features, deps) = resolve_features(parent, candidate, opts, version)?; // Next, transform all dependencies into a list of possible candidates // which can satisfy that dependency. @@ -294,6 +295,7 @@ pub fn resolve_features<'b>( parent: Option, s: &'b Summary, opts: &'b ResolveOpts, + version: ResolveVersion, ) -> ActivateResult<(HashSet, Vec<(Dependency, FeaturesSet)>)> { // First, filter by dev-dependencies. let deps = s.dependencies(); @@ -301,15 +303,28 @@ pub fn resolve_features<'b>( let reqs = build_requirements(parent, s, opts)?; let mut ret = Vec::new(); - let default_dep = BTreeSet::new(); + let default_dep = (false, BTreeSet::new()); let mut valid_dep_names = HashSet::new(); // Next, collect all actually enabled dependencies and their features. for dep in deps { // Skip optional dependencies, but not those enabled through a // feature - if dep.is_optional() && !reqs.deps.contains_key(&dep.name_in_toml()) { - continue; + if dep.is_optional() { + let disabled = reqs + .deps + .get(&dep.name_in_toml()) + .map(|(weak, _)| { + // Preserve old behaviour on older resolve versions. + if version <= ResolveVersion::V3 { + return false; + } + *weak && !reqs.features.contains(&dep.name_in_toml()) + }) + .unwrap_or(true); + if disabled { + continue; + } } valid_dep_names.insert(dep.name_in_toml()); // So we want this dependency. Move the features we want from @@ -319,7 +334,8 @@ pub fn resolve_features<'b>( .deps .get(&dep.name_in_toml()) .unwrap_or(&default_dep) - .clone(); + .clone() + .1; base.extend(dep.features().iter()); ret.push((dep.clone(), Rc::new(base))); } @@ -329,7 +345,10 @@ pub fn resolve_features<'b>( // validation is done either in `build_requirements` or // `build_feature_map`. if parent.is_none() { - for dep_name in reqs.deps.keys() { + for (dep_name, (weak, _)) in reqs.deps.iter() { + if *weak && !reqs.features.contains(dep_name) { + continue; + } if !valid_dep_names.contains(dep_name) { let e = RequirementError::MissingDependency(*dep_name); return Err(e.into_activate_error(parent, s)); @@ -400,11 +419,11 @@ fn build_requirements<'a, 'b: 'a>( #[derive(Debug)] struct Requirements<'a> { summary: &'a Summary, - /// The deps map is a mapping of dependency name to list of features enabled. + /// The deps map is a mapping of dependency name to (weak, list of features enabled). /// - /// The resolver will activate all of these dependencies, with the given - /// features enabled. - deps: HashMap>, + /// Non-weak deps will all be activated, while for weak deps the + /// corresponding feature needs to be enabled first. + deps: HashMap)>, /// The set of features enabled on this package which is later used when /// compiling to instruct the code what features were enabled. features: HashSet, @@ -457,12 +476,21 @@ impl Requirements<'_> { { self.require_feature(package)?; } - self.deps.entry(package).or_default().insert(feat); + + let dep = self + .deps + .entry(package) + .or_insert_with(|| (weak, BTreeSet::new())); + // If the feature is non-weak, mark the entire dep as non-weak. + dep.0 &= weak; + // Add the feature as to be enabled. + dep.1.insert(feat); + Ok(()) } fn require_dependency(&mut self, pkg: InternedString) { - self.deps.entry(pkg).or_default(); + self.deps.entry(pkg).or_default().0 = false; } fn require_feature(&mut self, feat: InternedString) -> Result<(), RequirementError> { @@ -494,9 +522,6 @@ impl Requirements<'_> { FeatureValue::DepFeature { dep_name, dep_feature, - // Weak features are always activated in the dependency - // resolver. They will be narrowed inside the new feature - // resolver. weak, } => self.require_dep_feature(*dep_name, *dep_feature, *weak)?, }; diff --git a/src/cargo/core/resolver/encode.rs b/src/cargo/core/resolver/encode.rs index 88d0d82961d..89ceb018820 100644 --- a/src/cargo/core/resolver/encode.rs +++ b/src/cargo/core/resolver/encode.rs @@ -158,6 +158,7 @@ impl EncodableResolve { let mut checksums = HashMap::new(); let mut version = match self.version { + Some(4) => ResolveVersion::V4, Some(3) => ResolveVersion::V3, Some(n) => bail!( "lock file version `{}` was found, but this version of Cargo \ @@ -612,6 +613,7 @@ impl ser::Serialize for Resolve { metadata, patch, version: match self.version() { + ResolveVersion::V4 => Some(4), ResolveVersion::V3 => Some(3), ResolveVersion::V2 | ResolveVersion::V1 => None, }, diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index b9c29fb872b..ddb0d93f9ef 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/src/cargo/core/resolver/mod.rs @@ -138,6 +138,7 @@ pub fn resolve( version_prefs: &VersionPreferences, config: Option<&Config>, check_public_visible_dependencies: bool, + version: ResolveVersion, ) -> CargoResult { let _p = profile::start("resolving"); let minimal_versions = match config { @@ -158,6 +159,7 @@ pub fn resolve( summaries, direct_minimal_versions, config, + version, )?; if registry.reset_pending() { break cx; @@ -190,7 +192,7 @@ pub fn resolve( cksums, BTreeMap::new(), Vec::new(), - ResolveVersion::default(), + version, summaries, ); @@ -212,6 +214,7 @@ fn activate_deps_loop( summaries: &[(Summary, ResolveOpts)], direct_minimal_versions: bool, config: Option<&Config>, + version: ResolveVersion, ) -> CargoResult { let mut backtrack_stack = Vec::new(); let mut remaining_deps = RemainingDeps::new(); @@ -230,6 +233,7 @@ fn activate_deps_loop( summary.clone(), direct_minimal_versions, opts, + version, ); match res { Ok(Some((frame, _))) => remaining_deps.push(frame), @@ -436,6 +440,7 @@ fn activate_deps_loop( candidate, direct_minimal_version, &opts, + version, ); let successfully_activated = match res { @@ -650,6 +655,7 @@ fn activate( candidate: Summary, first_minimal_version: bool, opts: &ResolveOpts, + version: ResolveVersion, ) -> ActivateResult> { let candidate_pid = candidate.package_id(); cx.age += 1; @@ -706,6 +712,7 @@ fn activate( &candidate, opts, first_minimal_version, + version, )?; // Record what list of features is active for this package. diff --git a/src/cargo/core/resolver/resolve.rs b/src/cargo/core/resolver/resolve.rs index 6ab957e924f..ac2713c0e55 100644 --- a/src/cargo/core/resolver/resolve.rs +++ b/src/cargo/core/resolver/resolve.rs @@ -80,6 +80,8 @@ pub enum ResolveVersion { /// V3 by default staring in 1.53. #[default] V3, + /// Weak dependency features are not treated as non-weak. + V4, } impl Resolve { diff --git a/src/cargo/ops/resolve.rs b/src/cargo/ops/resolve.rs index ea5eded4aa2..7d8b3c7be62 100644 --- a/src/cargo/ops/resolve.rs +++ b/src/cargo/ops/resolve.rs @@ -495,6 +495,8 @@ pub fn resolve_with_previous<'cfg>( None => root_replace.to_vec(), }; + let version = previous.map(|p| p.version()).unwrap_or_default(); + ws.preload(registry); let mut resolved = resolver::resolve( &summaries, @@ -505,6 +507,7 @@ pub fn resolve_with_previous<'cfg>( ws.unstable_features() .require(Feature::public_dependency()) .is_ok(), + version, )?; let patches: Vec<_> = registry .patches() diff --git a/tests/testsuite/weak_dep_features.rs b/tests/testsuite/weak_dep_features.rs index 6f7c035476a..e858bbf21b9 100644 --- a/tests/testsuite/weak_dep_features.rs +++ b/tests/testsuite/weak_dep_features.rs @@ -631,3 +631,349 @@ feat2 = ["bar?/feat"] )], ); } + +#[cargo_test] +fn disabled_weak_direct_dep() { + // Issue #10801 + // A weak direct dependency should be included in Cargo.lock, + // even if disabled, and even if on lockfile version 4. + Package::new("bar", "1.0.0") + .feature("feat", &[]) + .file("src/lib.rs", &require(&["feat"], &[])) + .publish(); + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + + [dependencies] + bar = { version = "1.0", optional = true } + + [features] + f1 = ["bar?/feat"] + "#, + ) + .file("src/lib.rs", &require(&["f1"], &[])) + .build(); + + p.cargo("generate-lockfile").run(); + + let lockfile = p.read_lockfile(); + assert!( + lockfile.contains(r#"version = 3"#), + "lockfile version is not 3!\n{lockfile}", + ); + // Previous behavior: bar is inside lockfile. + assert!( + lockfile.contains(r#"name = "bar""#), + "bar not found\n{lockfile}", + ); + + // Update to new lockfile version + let new_lockfile = lockfile.replace("version = 3", "version = 4"); + p.change_file("Cargo.lock", &new_lockfile); + + p.cargo("check --features f1") + .with_stderr( + "\ +[CHECKING] foo v0.1.0 [..] +[FINISHED] [..] +", + ) + .run(); + + let lockfile = p.read_lockfile(); + assert!( + lockfile.contains(r#"version = 4"#), + "lockfile version is not 4!\n{lockfile}", + ); + // New behavior: bar is still there because it is a direct (optional) dependency. + assert!( + lockfile.contains(r#"name = "bar""#), + "bar not found\n{lockfile}", + ); + + p.cargo("check --features f1,bar") + .with_stderr( + "\ +[DOWNLOADING] crates ... +[DOWNLOADED] bar v1.0.0 [..] +[CHECKING] bar v1.0.0 +[CHECKING] foo v0.1.0 [..] +[FINISHED] [..] +", + ) + .run(); +} + +#[cargo_test] +fn disabled_weak_optional_deps() { + // Issue #10801 + // A weak dependency of a dependency should not be included in Cargo.lock, + // at least on lockfile version 4. + Package::new("bar", "1.0.0") + .feature("feat", &[]) + .file("src/lib.rs", &require(&["feat"], &[])) + .publish(); + Package::new("dep", "1.0.0") + .add_dep(Dependency::new("bar", "1.0").optional(true)) + .feature("feat", &["bar?/feat"]) + .file("src/lib.rs", "") + .publish(); + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + + [dependencies] + dep = { version = "1.0", features = ["feat"] } + "#, + ) + .file("src/lib.rs", "") + .build(); + + p.cargo("generate-lockfile").run(); + + let lockfile = p.read_lockfile(); + + assert!( + lockfile.contains(r#"version = 3"#), + "lockfile version is not 3!\n{lockfile}", + ); + // Previous behavior: bar is inside lockfile. + assert!( + lockfile.contains(r#"name = "bar""#), + "bar not found\n{lockfile}", + ); + + // Update to new lockfile version + let new_lockfile = lockfile.replace("version = 3", "version = 4"); + p.change_file("Cargo.lock", &new_lockfile); + + // Note how we are not downloading bar here + p.cargo("check") + .with_stderr( + "\ +[DOWNLOADING] crates ... +[DOWNLOADED] dep v1.0.0 [..] +[CHECKING] dep v1.0.0 +[CHECKING] foo v0.1.0 [..] +[FINISHED] [..] +", + ) + .run(); + + let lockfile = p.read_lockfile(); + assert!( + lockfile.contains(r#"version = 4"#), + "lockfile version is not 4!\n{lockfile}", + ); + // New behavior: bar is gone. + assert!( + !lockfile.contains(r#"name = "bar""#), + "bar inside lockfile!\n{lockfile}", + ); + + // Note how we are not downloading bar here + p.cargo("check --features dep/bar") + .with_stderr( + "\ +[DOWNLOADING] crates ... +[DOWNLOADED] bar v1.0.0 [..] +[CHECKING] bar v1.0.0 +[CHECKING] dep v1.0.0 +[CHECKING] foo v0.1.0 [..] +[FINISHED] [..] +", + ) + .run(); + + let lockfile = p.read_lockfile(); + assert!( + lockfile.contains(r#"version = 4"#), + "lockfile version is not 4!\n{lockfile}", + ); + // bar is still not there, even if dep/bar is enabled on the command line. + // This might be unintuitive, but it matches what happens on lock version 3 + // if there was no optional feat = bar?/feat feature in bar. + assert!( + !lockfile.contains(r#"name = "bar""#), + "bar inside lockfile!\n{lockfile}", + ); +} + +#[cargo_test] +fn deferred_v4() { + // A modified version of the deferred test from above to enable + // an entire dependency in a deferred way. + Package::new("bar", "1.0.0") + .feature("feat", &["feat_dep"]) + .add_dep(Dependency::new("feat_dep", "1.0").optional(true)) + .file("src/lib.rs", "extern crate feat_dep;") + .publish(); + Package::new("dep", "1.0.0") + .add_dep(Dependency::new("bar", "1.0").optional(true)) + .feature("feat", &["bar?/feat"]) + .publish(); + Package::new("bar_activator", "1.0.0") + .feature_dep("dep", "1.0", &["bar"]) + .publish(); + Package::new("feat_dep", "1.0.0").publish(); + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + + [dependencies] + dep = { version = "1.0", features = ["feat"] } + bar_activator = "1.0" + "#, + ) + .file("src/lib.rs", "") + .build(); + + p.cargo("check") + .with_stderr( + "\ +[UPDATING] [..] +[DOWNLOADING] crates ... +[DOWNLOADED] feat_dep v1.0.0 [..] +[DOWNLOADED] dep v1.0.0 [..] +[DOWNLOADED] bar_activator v1.0.0 [..] +[DOWNLOADED] bar v1.0.0 [..] +[CHECKING] feat_dep v1.0.0 +[CHECKING] bar v1.0.0 +[CHECKING] dep v1.0.0 +[CHECKING] bar_activator v1.0.0 +[CHECKING] foo v0.1.0 [..] +[FINISHED] [..] +", + ) + .run(); + let lockfile = p.read_lockfile(); + + assert!( + lockfile.contains(r#"version = 3"#), + "lockfile version is not 3!\n{lockfile}", + ); + // Previous behavior: feat_dep is inside lockfile. + assert!( + lockfile.contains(r#"name = "feat_dep""#), + "feat_dep not found\n{lockfile}", + ); + // Update to new lockfile version + let new_lockfile = lockfile.replace("version = 3", "version = 4"); + p.change_file("Cargo.lock", &new_lockfile); + + // We should still compile feat_dep + p.cargo("check") + .with_stderr( + "\ +[CHECKING] feat_dep v1.0.0 +[CHECKING] bar v1.0.0 +[CHECKING] dep v1.0.0 +[CHECKING] bar_activator v1.0.0 +[CHECKING] foo v0.1.0 [..] +[FINISHED] [..] +", + ) + .run(); + + let lockfile = p.read_lockfile(); + assert!( + lockfile.contains(r#"version = 4"#), + "lockfile version is not 4!\n{lockfile}", + ); + // New behavior: feat_dep is still there. + assert!( + lockfile.contains(r#"name = "feat_dep""#), + "feat_dep not found\n{lockfile}", + ); +} + +#[cargo_test] +fn weak_namespaced_v4() { + // Behavior with a dep: dependency. + Package::new("maybe_enabled", "1.0.0") + .feature("feat", &[]) + .file("src/lib.rs", &require(&["feat"], &[])) + .publish(); + Package::new("bar", "1.0.0") + .add_dep(Dependency::new("maybe_enabled", "1.0").optional(true)) + .feature("f1", &["maybe_enabled?/feat"]) + .feature("f2", &["dep:maybe_enabled"]) + .publish(); + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + + [dependencies] + bar = { version = "1.0", features = ["f2", "f1"] } + "#, + ) + .file("src/lib.rs", "") + .build(); + + p.cargo("check") + .with_stderr( + "\ +[UPDATING] [..] +[DOWNLOADING] crates ... +[DOWNLOADED] maybe_enabled v1.0.0 [..] +[DOWNLOADED] bar v1.0.0 [..] +[CHECKING] maybe_enabled v1.0.0 +[CHECKING] bar v1.0.0 +[CHECKING] foo v0.1.0 [..] +[FINISHED] [..] +", + ) + .run(); + + let lockfile = p.read_lockfile(); + + assert!( + lockfile.contains(r#"version = 3"#), + "lockfile version is not 3!\n{lockfile}", + ); + // Previous behavior: maybe_enabled is inside lockfile. + assert!( + lockfile.contains(r#"name = "maybe_enabled""#), + "maybe_enabled not found\n{lockfile}", + ); + // Update to new lockfile version + let new_lockfile = lockfile.replace("version = 3", "version = 4"); + p.change_file("Cargo.lock", &new_lockfile); + + // We should still compile maybe_enabled + p.cargo("check") + .with_stderr( + "\ +[FINISHED] [..] +", + ) + .run(); + + let lockfile = p.read_lockfile(); + assert!( + lockfile.contains(r#"version = 4"#), + "lockfile version is not 4!\n{lockfile}", + ); + // New behavior: maybe_enabled is still there. + assert!( + lockfile.contains(r#"name = "maybe_enabled""#), + "maybe_enabled not found\n{lockfile}", + ); +}