diff --git a/src/bin/cargo/commands/metadata.rs b/src/bin/cargo/commands/metadata.rs index 3d14868dbb7..b475816aa67 100644 --- a/src/bin/cargo/commands/metadata.rs +++ b/src/bin/cargo/commands/metadata.rs @@ -44,9 +44,7 @@ pub fn exec(config: &mut Config, args: &ArgMatches<'_>) -> CliResult { }; let options = OutputMetadataOptions { - features: values(args, "features"), - all_features: args.is_present("all-features"), - no_default_features: args.is_present("no-default-features"), + cli_features: args.cli_features()?, no_deps: args.is_present("no-deps"), filter_platforms: args._values_of("filter-platform"), version, diff --git a/src/bin/cargo/commands/package.rs b/src/bin/cargo/commands/package.rs index 2ce80564ccf..92383dfb1f9 100644 --- a/src/bin/cargo/commands/package.rs +++ b/src/bin/cargo/commands/package.rs @@ -45,9 +45,7 @@ pub fn exec(config: &mut Config, args: &ArgMatches<'_>) -> CliResult { allow_dirty: args.is_present("allow-dirty"), targets: args.targets(), jobs: args.jobs()?, - features: args._values_of("features"), - all_features: args.is_present("all-features"), - no_default_features: args.is_present("no-default-features"), + cli_features: args.cli_features()?, }, )?; Ok(()) diff --git a/src/bin/cargo/commands/publish.rs b/src/bin/cargo/commands/publish.rs index 3e43cfe576c..83861e299c1 100644 --- a/src/bin/cargo/commands/publish.rs +++ b/src/bin/cargo/commands/publish.rs @@ -45,9 +45,7 @@ pub fn exec(config: &mut Config, args: &ArgMatches<'_>) -> CliResult { jobs: args.jobs()?, dry_run: args.is_present("dry-run"), registry, - features: args._values_of("features"), - all_features: args.is_present("all-features"), - no_default_features: args.is_present("no-default-features"), + cli_features: args.cli_features()?, }, )?; Ok(()) diff --git a/src/bin/cargo/commands/tree.rs b/src/bin/cargo/commands/tree.rs index d14c6887478..e6a1f21b2f6 100644 --- a/src/bin/cargo/commands/tree.rs +++ b/src/bin/cargo/commands/tree.rs @@ -190,9 +190,7 @@ subtree of the package given to -p.\n\ let charset = tree::Charset::from_str(args.value_of("charset").unwrap()) .map_err(|e| anyhow::anyhow!("{}", e))?; let opts = tree::TreeOptions { - features: values(args, "features"), - all_features: args.is_present("all-features"), - no_default_features: args.is_present("no-default-features"), + cli_features: args.cli_features()?, packages, target, edge_kinds, diff --git a/src/cargo/core/compiler/standard_lib.rs b/src/cargo/core/compiler/standard_lib.rs index 0b14df8055a..4b221deff45 100644 --- a/src/cargo/core/compiler/standard_lib.rs +++ b/src/cargo/core/compiler/standard_lib.rs @@ -3,8 +3,8 @@ use crate::core::compiler::UnitInterner; use crate::core::compiler::{CompileKind, CompileMode, RustcTargetData, Unit}; use crate::core::profiles::{Profiles, UnitFor}; -use crate::core::resolver::features::{FeaturesFor, RequestedFeatures, ResolvedFeatures}; -use crate::core::resolver::{HasDevUnits, ResolveOpts}; +use crate::core::resolver::features::{CliFeatures, FeaturesFor, ResolvedFeatures}; +use crate::core::resolver::HasDevUnits; use crate::core::{Dependency, PackageId, PackageSet, Resolve, SourceId, Workspace}; use crate::ops::{self, Packages}; use crate::util::errors::CargoResult; @@ -107,18 +107,14 @@ pub fn resolve_std<'cfg>( "default".to_string(), ], }; - // dev_deps setting shouldn't really matter here. - let opts = ResolveOpts::new( - /*dev_deps*/ false, - RequestedFeatures::from_command_line( - &features, /*all_features*/ false, /*uses_default_features*/ false, - ), - ); + let cli_features = CliFeatures::from_command_line( + &features, /*all_features*/ false, /*uses_default_features*/ false, + )?; let resolve = ops::resolve_ws_with_opts( &std_ws, target_data, requested_targets, - &opts, + &cli_features, &specs, HasDevUnits::No, crate::core::resolver::features::ForceAllTargets::No, diff --git a/src/cargo/core/resolver/context.rs b/src/cargo/core/resolver/context.rs index 8767170672b..f5c191cd751 100644 --- a/src/cargo/core/resolver/context.rs +++ b/src/cargo/core/resolver/context.rs @@ -1,6 +1,7 @@ use super::dep_cache::RegistryQueryer; use super::errors::ActivateResult; use super::types::{ConflictMap, ConflictReason, FeaturesSet, ResolveOpts}; +use super::RequestedFeatures; use crate::core::{Dependency, PackageId, SourceId, Summary}; use crate::util::interning::InternedString; use crate::util::Graph; @@ -160,23 +161,32 @@ impl Context { } } debug!("checking if {} is already activated", summary.package_id()); - if opts.features.all_features { - return Ok(false); - } - - let has_default_feature = summary.features().contains_key("default"); - Ok(match self.resolve_features.get(&id) { - Some(prev) => { - opts.features.features.is_subset(prev) - && (!opts.features.uses_default_features - || prev.contains("default") - || !has_default_feature) - } - None => { - opts.features.features.is_empty() - && (!opts.features.uses_default_features || !has_default_feature) + match &opts.features { + // This returns `false` for CliFeatures just for simplicity. It + // would take a bit of work to compare since they are not in the + // same format as DepFeatures (and that may be expensive + // performance-wise). Also, it should only occur once for a root + // package. The only drawback is that it may re-activate a root + // package again, which should only affect performance, but that + // should be rare. Cycles should still be detected since those + // will have `DepFeatures` edges. + RequestedFeatures::CliFeatures(_) => return Ok(false), + RequestedFeatures::DepFeatures { + features, + uses_default_features, + } => { + let has_default_feature = summary.features().contains_key("default"); + Ok(match self.resolve_features.get(&id) { + Some(prev) => { + features.is_subset(prev) + && (!uses_default_features + || prev.contains("default") + || !has_default_feature) + } + None => features.is_empty() && (!uses_default_features || !has_default_feature), + }) } - }) + } } /// If the package is active returns the `ContextAge` when it was added diff --git a/src/cargo/core/resolver/dep_cache.rs b/src/cargo/core/resolver/dep_cache.rs index 7e3f4fb5938..0446130fabe 100644 --- a/src/cargo/core/resolver/dep_cache.rs +++ b/src/cargo/core/resolver/dep_cache.rs @@ -12,7 +12,9 @@ use crate::core::resolver::context::Context; use crate::core::resolver::errors::describe_path; use crate::core::resolver::types::{ConflictReason, DepInfo, FeaturesSet}; -use crate::core::resolver::{ActivateError, ActivateResult, ResolveOpts}; +use crate::core::resolver::{ + ActivateError, ActivateResult, CliFeatures, RequestedFeatures, ResolveOpts, +}; use crate::core::{Dependency, FeatureValue, PackageId, PackageIdSpec, Registry, Summary}; use crate::util::errors::{CargoResult, CargoResultExt}; use crate::util::interning::InternedString; @@ -281,15 +283,6 @@ pub fn resolve_features<'b>( .unwrap_or(&default_dep) .clone(); base.extend(dep.features().iter()); - for feature in base.iter() { - if feature.contains('/') { - return Err(anyhow::format_err!( - "feature names may not contain slashes: `{}`", - feature - ) - .into()); - } - } ret.push((dep.clone(), Rc::new(base))); } @@ -317,30 +310,46 @@ fn build_requirements<'a, 'b: 'a>( ) -> ActivateResult> { let mut reqs = Requirements::new(s); - if opts.features.all_features { - for key in s.features().keys() { - if let Err(e) = reqs.require_feature(*key) { + let handle_default = |uses_default_features, reqs: &mut Requirements<'_>| { + if uses_default_features && s.features().contains_key("default") { + if let Err(e) = reqs.require_feature(InternedString::new("default")) { return Err(e.into_activate_error(parent, s)); } } - } else { - for &f in opts.features.features.iter() { - let fv = FeatureValue::new(f); - if fv.has_dep_prefix() { - return Err(ActivateError::Fatal(anyhow::format_err!( - "feature value `{}` is not allowed to use explicit `dep:` syntax", - fv - ))); - } - if let Err(e) = reqs.require_value(&fv) { - return Err(e.into_activate_error(parent, s)); + Ok(()) + }; + + match &opts.features { + RequestedFeatures::CliFeatures(CliFeatures { + features, + all_features, + uses_default_features, + }) => { + if *all_features { + for key in s.features().keys() { + if let Err(e) = reqs.require_feature(*key) { + return Err(e.into_activate_error(parent, s)); + } + } + } else { + for fv in features.iter() { + if let Err(e) = reqs.require_value(&fv) { + return Err(e.into_activate_error(parent, s)); + } + } + handle_default(*uses_default_features, &mut reqs)?; } } - } - - if opts.features.uses_default_features && s.features().contains_key("default") { - if let Err(e) = reqs.require_feature(InternedString::new("default")) { - return Err(e.into_activate_error(parent, s)); + RequestedFeatures::DepFeatures { + features, + uses_default_features, + } => { + for feature in features.iter() { + if let Err(e) = reqs.require_feature(*feature) { + return Err(e.into_activate_error(parent, s)); + } + } + handle_default(*uses_default_features, &mut reqs)?; } } diff --git a/src/cargo/core/resolver/features.rs b/src/cargo/core/resolver/features.rs index 383303f26be..1d3fdd2694d 100644 --- a/src/cargo/core/resolver/features.rs +++ b/src/cargo/core/resolver/features.rs @@ -42,6 +42,7 @@ use crate::core::resolver::{Resolve, ResolveBehavior}; use crate::core::{FeatureValue, PackageId, PackageIdSpec, PackageSet, Workspace}; use crate::util::interning::InternedString; use crate::util::CargoResult; +use anyhow::bail; use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet}; use std::rc::Rc; @@ -144,7 +145,7 @@ impl FeatureOpts { } "compare" => opts.compare = true, "ws" => unimplemented!(), - s => anyhow::bail!("-Zfeatures flag `{}` is not supported", s), + s => bail!("-Zfeatures flag `{}` is not supported", s), } } Ok(()) @@ -197,44 +198,93 @@ impl FeatureOpts { } /// Features flags requested for a package. +/// +/// This should be cheap and fast to clone, it is used in the resolver for +/// various caches. +/// +/// This is split into enum variants because the resolver needs to handle +/// features coming from different places (command-line and dependency +/// declarations), but those different places have different constraints on +/// which syntax is allowed. This helps ensure that every place dealing with +/// features is properly handling those syntax restrictions. +#[derive(Debug, Clone, Eq, PartialEq, Hash)] +pub enum RequestedFeatures { + /// Features requested on the command-line with flags. + CliFeatures(CliFeatures), + /// Features specified in a dependency declaration. + DepFeatures { + /// The `features` dependency field. + features: FeaturesSet, + /// The `default-features` dependency field. + uses_default_features: bool, + }, +} + +/// Features specified on the command-line. #[derive(Debug, Clone, Eq, PartialEq, Hash)] -pub struct RequestedFeatures { - pub features: FeaturesSet, +pub struct CliFeatures { + /// Features from the `--features` flag. + pub features: Rc>, + /// The `--all-features` flag. pub all_features: bool, + /// Inverse of `--no-default-features` flag. pub uses_default_features: bool, } -impl RequestedFeatures { - /// Creates a new RequestedFeatures from the given command-line flags. +impl CliFeatures { + /// Creates a new CliFeatures from the given command-line flags. pub fn from_command_line( features: &[String], all_features: bool, uses_default_features: bool, - ) -> RequestedFeatures { - RequestedFeatures { - features: Rc::new(RequestedFeatures::split_features(features)), + ) -> CargoResult { + let features = Rc::new(CliFeatures::split_features(features)); + // Some early validation to ensure correct syntax. + for feature in features.iter() { + match feature { + // Maybe call validate_feature_name here once it is an error? + FeatureValue::Feature(_) => {} + FeatureValue::Dep { .. } + | FeatureValue::DepFeature { + dep_prefix: true, .. + } => { + bail!( + "feature `{}` is not allowed to use explicit `dep:` syntax", + feature + ); + } + FeatureValue::DepFeature { dep_feature, .. } => { + if dep_feature.contains('/') { + bail!("multiple slashes in feature `{}` is not allowed", feature); + } + } + } + } + Ok(CliFeatures { + features, all_features, uses_default_features, - } + }) } - /// Creates a new RequestedFeatures with the given `all_features` setting. - pub fn new_all(all_features: bool) -> RequestedFeatures { - RequestedFeatures { + /// Creates a new CliFeatures with the given `all_features` setting. + pub fn new_all(all_features: bool) -> CliFeatures { + CliFeatures { features: Rc::new(BTreeSet::new()), all_features, uses_default_features: true, } } - fn split_features(features: &[String]) -> BTreeSet { + fn split_features(features: &[String]) -> BTreeSet { features .iter() .flat_map(|s| s.split_whitespace()) .flat_map(|s| s.split(',')) .filter(|s| !s.is_empty()) .map(InternedString::new) - .collect::>() + .map(FeatureValue::new) + .collect() } } @@ -296,7 +346,7 @@ impl ResolvedFeatures { if let Some(fs) = self.activated_features.get(&(pkg_id, is_build)) { Ok(fs.iter().cloned().collect()) } else { - anyhow::bail!("features did not find {:?} {:?}", pkg_id, is_build) + bail!("features did not find {:?} {:?}", pkg_id, is_build) } } } @@ -405,7 +455,7 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> { target_data: &RustcTargetData, resolve: &Resolve, package_set: &'a PackageSet<'cfg>, - requested_features: &RequestedFeatures, + cli_features: &CliFeatures, specs: &[PackageIdSpec], requested_targets: &[CompileKind], opts: FeatureOpts, @@ -437,7 +487,7 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> { track_for_host, deferred_weak_dependencies: HashMap::new(), }; - r.do_resolve(specs, requested_features)?; + r.do_resolve(specs, cli_features)?; log::debug!("features={:#?}", r.activated_features); if r.opts.compare { r.compare(); @@ -455,11 +505,11 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> { fn do_resolve( &mut self, specs: &[PackageIdSpec], - requested_features: &RequestedFeatures, + cli_features: &CliFeatures, ) -> CargoResult<()> { - let member_features = self.ws.members_with_features(specs, requested_features)?; - for (member, requested_features) in &member_features { - let fvs = self.fvs_from_requested(member.package_id(), requested_features); + let member_features = self.ws.members_with_features(specs, cli_features)?; + for (member, cli_features) in &member_features { + let fvs = self.fvs_from_requested(member.package_id(), cli_features); let for_host = self.track_for_host && self.is_proc_macro(member.package_id()); self.activate_pkg(member.package_id(), for_host, &fvs)?; if for_host { @@ -725,24 +775,19 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> { fn fvs_from_requested( &self, pkg_id: PackageId, - requested_features: &RequestedFeatures, + cli_features: &CliFeatures, ) -> Vec { let summary = self.resolve.summary(pkg_id); let feature_map = summary.features(); - if requested_features.all_features { + if cli_features.all_features { feature_map .keys() .map(|k| FeatureValue::Feature(*k)) .collect() } else { - let mut result: Vec = requested_features - .features - .as_ref() - .iter() - .map(|f| FeatureValue::new(*f)) - .collect(); + let mut result: Vec = cli_features.features.iter().cloned().collect(); let default = InternedString::new("default"); - if requested_features.uses_default_features && feature_map.contains_key(&default) { + if cli_features.uses_default_features && feature_map.contains_key(&default) { result.push(FeatureValue::Feature(default)); } result diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index 6cec24702f4..b4cb577db7a 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/src/cargo/core/resolver/mod.rs @@ -69,7 +69,7 @@ use self::types::{FeaturesSet, RcVecIter, RemainingDeps, ResolverProgress}; pub use self::encode::Metadata; pub use self::encode::{EncodableDependency, EncodablePackageId, EncodableResolve}; pub use self::errors::{ActivateError, ActivateResult, ResolveError}; -pub use self::features::{ForceAllTargets, HasDevUnits}; +pub use self::features::{CliFeatures, ForceAllTargets, HasDevUnits}; pub use self::resolve::{Resolve, ResolveVersion}; pub use self::types::{ResolveBehavior, ResolveOpts}; @@ -192,7 +192,7 @@ fn activate_deps_loop( // Activate all the initial summaries to kick off some work. for &(ref summary, ref opts) in summaries { debug!("initial activation: {}", summary.package_id()); - let res = activate(&mut cx, registry, None, summary.clone(), opts.clone()); + let res = activate(&mut cx, registry, None, summary.clone(), opts); match res { Ok(Some((frame, _))) => remaining_deps.push(frame), Ok(None) => (), @@ -378,9 +378,8 @@ fn activate_deps_loop( let pid = candidate.package_id(); let opts = ResolveOpts { dev_deps: false, - features: RequestedFeatures { + features: RequestedFeatures::DepFeatures { features: Rc::clone(&features), - all_features: false, uses_default_features: dep.uses_default_features(), }, }; @@ -391,7 +390,7 @@ fn activate_deps_loop( dep.package_name(), candidate.version() ); - let res = activate(&mut cx, registry, Some((&parent, &dep)), candidate, opts); + let res = activate(&mut cx, registry, Some((&parent, &dep)), candidate, &opts); let successfully_activated = match res { // Success! We've now activated our `candidate` in our context @@ -603,7 +602,7 @@ fn activate( registry: &mut RegistryQueryer<'_>, parent: Option<(&Summary, &Dependency)>, candidate: Summary, - opts: ResolveOpts, + opts: &ResolveOpts, ) -> ActivateResult> { let candidate_pid = candidate.package_id(); cx.age += 1; diff --git a/src/cargo/core/resolver/types.rs b/src/cargo/core/resolver/types.rs index bdb493e550e..6a7e3a84735 100644 --- a/src/cargo/core/resolver/types.rs +++ b/src/cargo/core/resolver/types.rs @@ -1,4 +1,4 @@ -use super::features::RequestedFeatures; +use super::features::{CliFeatures, RequestedFeatures}; use crate::core::{Dependency, PackageId, Summary}; use crate::util::errors::CargoResult; use crate::util::interning::InternedString; @@ -133,6 +133,7 @@ pub struct ResolveOpts { /// Whether or not dev-dependencies should be included. /// /// This may be set to `false` by things like `cargo install` or `-Z avoid-dev-deps`. + /// It also gets set to `false` when activating dependencies in the resolver. pub dev_deps: bool, /// Set of features requested on the command-line. pub features: RequestedFeatures, @@ -143,7 +144,7 @@ impl ResolveOpts { pub fn everything() -> ResolveOpts { ResolveOpts { dev_deps: true, - features: RequestedFeatures::new_all(true), + features: RequestedFeatures::CliFeatures(CliFeatures::new_all(true)), } } diff --git a/src/cargo/core/summary.rs b/src/cargo/core/summary.rs index 8f77c75fb4c..b0f69ae5065 100644 --- a/src/cargo/core/summary.rs +++ b/src/cargo/core/summary.rs @@ -250,6 +250,12 @@ fn build_feature_map( feature ); } + if feature.contains('/') { + bail!( + "feature named `{}` is not allowed to contain slashes", + feature + ); + } validate_feature_name(config, pkg_id, feature)?; for fv in fvs { // Find data for the referenced dependency... @@ -316,7 +322,20 @@ fn build_feature_map( ); } } - DepFeature { dep_name, weak, .. } => { + DepFeature { + dep_name, + dep_feature, + weak, + .. + } => { + // Early check for some unlikely syntax. + if dep_feature.contains('/') { + bail!( + "multiple slashes in feature `{}` (included by feature `{}`) are not allowed", + fv, + feature + ); + } // Validation of the feature name will be performed in the resolver. if !is_any_dep { bail!( @@ -362,7 +381,7 @@ fn build_feature_map( } /// FeatureValue represents the types of dependencies a feature can have. -#[derive(Clone, Debug)] +#[derive(Clone, Debug, Ord, PartialOrd, Eq, PartialEq, Hash)] pub enum FeatureValue { /// A feature enabling another feature. Feature(InternedString), diff --git a/src/cargo/core/workspace.rs b/src/cargo/core/workspace.rs index 7fcbeaf07dc..56f1536749c 100644 --- a/src/cargo/core/workspace.rs +++ b/src/cargo/core/workspace.rs @@ -5,15 +5,16 @@ use std::path::{Path, PathBuf}; use std::rc::Rc; use std::slice; +use anyhow::bail; use glob::glob; use log::debug; use url::Url; use crate::core::features::Features; use crate::core::registry::PackageRegistry; -use crate::core::resolver::features::RequestedFeatures; +use crate::core::resolver::features::CliFeatures; use crate::core::resolver::ResolveBehavior; -use crate::core::{Dependency, Edition, PackageId, PackageIdSpec}; +use crate::core::{Dependency, Edition, FeatureValue, PackageId, PackageIdSpec}; use crate::core::{EitherManifest, Package, SourceId, VirtualManifest}; use crate::ops; use crate::sources::{PathSource, CRATES_IO_INDEX, CRATES_IO_REGISTRY}; @@ -152,7 +153,7 @@ impl<'cfg> Workspace<'cfg> { ws.target_dir = config.target_dir()?; if manifest_path.is_relative() { - anyhow::bail!( + bail!( "manifest_path:{:?} is not an absolute path. Please provide an absolute path.", manifest_path ) @@ -523,7 +524,7 @@ impl<'cfg> Workspace<'cfg> { return Ok(Some(root_config.clone())); } - _ => anyhow::bail!( + _ => bail!( "root of a workspace inferred but wasn't a root: {}", root_path.display() ), @@ -663,7 +664,7 @@ impl<'cfg> Workspace<'cfg> { if exclude { continue; } - anyhow::bail!( + bail!( "package `{}` is listed in workspace’s default-members \ but is not a member.", path.display() @@ -785,7 +786,7 @@ impl<'cfg> Workspace<'cfg> { MaybePackage::Virtual(_) => continue, }; if let Some(prev) = names.insert(name, member) { - anyhow::bail!( + bail!( "two packages named `{}` in this workspace:\n\ - {}\n\ - {}", @@ -810,7 +811,7 @@ impl<'cfg> Workspace<'cfg> { .collect(); match roots.len() { 1 => Ok(()), - 0 => anyhow::bail!( + 0 => bail!( "`package.workspace` configuration points to a crate \ which is not configured with [workspace]: \n\ configuration at: {}\n\ @@ -819,7 +820,7 @@ impl<'cfg> Workspace<'cfg> { self.root_manifest.as_ref().unwrap().display() ), _ => { - anyhow::bail!( + bail!( "multiple workspace roots found in the same workspace:\n{}", roots .iter() @@ -840,7 +841,7 @@ impl<'cfg> Workspace<'cfg> { match root { Some(root) => { - anyhow::bail!( + bail!( "package `{}` is a member of the wrong workspace\n\ expected: {}\n\ actual: {}", @@ -850,7 +851,7 @@ impl<'cfg> Workspace<'cfg> { ); } None => { - anyhow::bail!( + bail!( "workspace member `{}` is not hierarchically below \ the workspace root `{}`", member.display(), @@ -907,7 +908,7 @@ impl<'cfg> Workspace<'cfg> { } } }; - anyhow::bail!( + bail!( "current package believes it's in a workspace when it's not:\n\ current: {}\n\ workspace: {}\n\n{}\n\ @@ -963,7 +964,7 @@ impl<'cfg> Workspace<'cfg> { pub fn load(&self, manifest_path: &Path) -> CargoResult { match self.packages.maybe_get(manifest_path) { Some(&MaybePackage::Package(ref p)) => return Ok(p.clone()), - Some(&MaybePackage::Virtual(_)) => anyhow::bail!("cannot load workspace root"), + Some(&MaybePackage::Virtual(_)) => bail!("cannot load workspace root"), None => {} } @@ -1046,10 +1047,10 @@ impl<'cfg> Workspace<'cfg> { pub fn members_with_features( &self, specs: &[PackageIdSpec], - requested_features: &RequestedFeatures, - ) -> CargoResult> { + cli_features: &CliFeatures, + ) -> CargoResult> { assert!( - !specs.is_empty() || requested_features.all_features, + !specs.is_empty() || cli_features.all_features, "no specs requires all_features" ); if specs.is_empty() { @@ -1057,13 +1058,13 @@ impl<'cfg> Workspace<'cfg> { // all features enabled. return Ok(self .members() - .map(|m| (m, RequestedFeatures::new_all(true))) + .map(|m| (m, CliFeatures::new_all(true))) .collect()); } if self.allows_new_cli_feature_behavior() { - self.members_with_features_new(specs, requested_features) + self.members_with_features_new(specs, cli_features) } else { - Ok(self.members_with_features_old(specs, requested_features)) + Ok(self.members_with_features_old(specs, cli_features)) } } @@ -1072,17 +1073,17 @@ impl<'cfg> Workspace<'cfg> { fn members_with_features_new( &self, specs: &[PackageIdSpec], - requested_features: &RequestedFeatures, - ) -> CargoResult> { + cli_features: &CliFeatures, + ) -> CargoResult> { // Keep track of which features matched *any* member, to produce an error // if any of them did not match anywhere. - let mut found: BTreeSet = BTreeSet::new(); + let mut found: BTreeSet = BTreeSet::new(); // Returns the requested features for the given member. // This filters out any named features that the member does not have. - let mut matching_features = |member: &Package| -> RequestedFeatures { - if requested_features.features.is_empty() || requested_features.all_features { - return requested_features.clone(); + let mut matching_features = |member: &Package| -> CliFeatures { + if cli_features.features.is_empty() || cli_features.all_features { + return cli_features.clone(); } // Only include features this member defines. let summary = member.summary(); @@ -1098,40 +1099,54 @@ impl<'cfg> Workspace<'cfg> { .any(|dep| dep.is_optional() && dep.name_in_toml() == feature) }; - for feature in requested_features.features.iter() { - let mut split = feature.splitn(2, '/'); - let split = (split.next().unwrap(), split.next()); - if let (pkg, Some(pkg_feature)) = split { - let pkg = InternedString::new(pkg); - let pkg_feature = InternedString::new(pkg_feature); - if summary - .dependencies() - .iter() - .any(|dep| dep.name_in_toml() == pkg) - { - // pkg/feat for a dependency. - // Will rely on the dependency resolver to validate `feat`. - features.insert(*feature); - found.insert(*feature); - } else if pkg == member.name() && contains(pkg_feature) { - // member/feat where "feat" is a feature in member. - features.insert(pkg_feature); - found.insert(*feature); + for feature in cli_features.features.iter() { + match feature { + FeatureValue::Feature(f) => { + if contains(*f) { + // feature exists in this member. + features.insert(feature.clone()); + found.insert(feature.clone()); + } + } + // This should be enforced by CliFeatures. + FeatureValue::Dep { .. } + | FeatureValue::DepFeature { + dep_prefix: true, .. + } => panic!("unexpected dep: syntax {}", feature), + FeatureValue::DepFeature { + dep_name, + dep_feature, + dep_prefix: _, + weak: _, + } => { + if summary + .dependencies() + .iter() + .any(|dep| dep.name_in_toml() == *dep_name) + { + // pkg/feat for a dependency. + // Will rely on the dependency resolver to validate `dep_feature`. + features.insert(feature.clone()); + found.insert(feature.clone()); + } else if *dep_name == member.name() && contains(*dep_feature) { + // member/feat where "feat" is a feature in member. + // + // `weak` can be ignored here, because the member + // either is or isn't being built. + features.insert(FeatureValue::Feature(*dep_feature)); + found.insert(feature.clone()); + } } - } else if contains(*feature) { - // feature exists in this member. - features.insert(*feature); - found.insert(*feature); } } - RequestedFeatures { + CliFeatures { features: Rc::new(features), all_features: false, - uses_default_features: requested_features.uses_default_features, + uses_default_features: cli_features.uses_default_features, } }; - let members: Vec<(&Package, RequestedFeatures)> = self + let members: Vec<(&Package, CliFeatures)> = self .members() .filter(|m| specs.iter().any(|spec| spec.matches(m.package_id()))) .map(|m| (m, matching_features(m))) @@ -1139,27 +1154,28 @@ impl<'cfg> Workspace<'cfg> { if members.is_empty() { // `cargo build -p foo`, where `foo` is not a member. // Do not allow any command-line flags (defaults only). - if !(requested_features.features.is_empty() - && !requested_features.all_features - && requested_features.uses_default_features) + if !(cli_features.features.is_empty() + && !cli_features.all_features + && cli_features.uses_default_features) { - anyhow::bail!("cannot specify features for packages outside of workspace"); + bail!("cannot specify features for packages outside of workspace"); } // Add all members from the workspace so we can ensure `-p nonmember` // is in the resolve graph. return Ok(self .members() - .map(|m| (m, RequestedFeatures::new_all(false))) + .map(|m| (m, CliFeatures::new_all(false))) .collect()); } - if *requested_features.features != found { - let missing: Vec<_> = requested_features + if *cli_features.features != found { + let mut missing: Vec<_> = cli_features .features .difference(&found) - .copied() + .map(|fv| fv.to_string()) .collect(); + missing.sort(); // TODO: typo suggestions would be good here. - anyhow::bail!( + bail!( "none of the selected packages contains these features: {}", missing.join(", ") ); @@ -1172,28 +1188,46 @@ impl<'cfg> Workspace<'cfg> { fn members_with_features_old( &self, specs: &[PackageIdSpec], - requested_features: &RequestedFeatures, - ) -> Vec<(&Package, RequestedFeatures)> { + cli_features: &CliFeatures, + ) -> Vec<(&Package, CliFeatures)> { // Split off any features with the syntax `member-name/feature-name` into a map // so that those features can be applied directly to those workspace-members. - let mut member_specific_features: HashMap<&str, BTreeSet> = HashMap::new(); + let mut member_specific_features: HashMap> = + HashMap::new(); // Features for the member in the current directory. let mut cwd_features = BTreeSet::new(); - for feature in requested_features.features.iter() { - if let Some(index) = feature.find('/') { - let name = &feature[..index]; - let is_member = self.members().any(|member| member.name() == name); - if is_member && specs.iter().any(|spec| spec.name() == name) { - member_specific_features - .entry(name) - .or_default() - .insert(InternedString::new(&feature[index + 1..])); - } else { - cwd_features.insert(*feature); + for feature in cli_features.features.iter() { + match feature { + FeatureValue::Feature(_) => { + cwd_features.insert(feature.clone()); } - } else { - cwd_features.insert(*feature); - }; + // This should be enforced by CliFeatures. + FeatureValue::Dep { .. } + | FeatureValue::DepFeature { + dep_prefix: true, .. + } => panic!("unexpected dep: syntax {}", feature), + FeatureValue::DepFeature { + dep_name, + dep_feature, + dep_prefix: _, + weak: _, + } => { + // I think weak can be ignored here. + // * With `--features member?/feat -p member`, the ? doesn't + // really mean anything (either the member is built or it isn't). + // * With `--features nonmember?/feat`, cwd_features will + // handle processing it correctly. + let is_member = self.members().any(|member| member.name() == *dep_name); + if is_member && specs.iter().any(|spec| spec.name() == *dep_name) { + member_specific_features + .entry(*dep_name) + .or_default() + .insert(FeatureValue::Feature(*dep_feature)); + } else { + cwd_features.insert(feature.clone()); + } + } + } } let ms = self.members().filter_map(|member| { @@ -1202,10 +1236,10 @@ impl<'cfg> Workspace<'cfg> { // The features passed on the command-line only apply to // the "current" package (determined by the cwd). Some(current) if member_id == current.package_id() => { - let feats = RequestedFeatures { + let feats = CliFeatures { features: Rc::new(cwd_features.clone()), - all_features: requested_features.all_features, - uses_default_features: requested_features.uses_default_features, + all_features: cli_features.all_features, + uses_default_features: cli_features.uses_default_features, }; Some((member, feats)) } @@ -1221,14 +1255,14 @@ impl<'cfg> Workspace<'cfg> { // "current" package. As an extension, this allows // member-name/feature-name to set member-specific // features, which should be backwards-compatible. - let feats = RequestedFeatures { + let feats = CliFeatures { features: Rc::new( member_specific_features .remove(member.name().as_str()) .unwrap_or_default(), ), uses_default_features: true, - all_features: requested_features.all_features, + all_features: cli_features.all_features, }; Some((member, feats)) } else { diff --git a/src/cargo/ops/cargo_compile.rs b/src/cargo/ops/cargo_compile.rs index 51faba1d974..2b6719d55fe 100644 --- a/src/cargo/ops/cargo_compile.rs +++ b/src/cargo/ops/cargo_compile.rs @@ -33,8 +33,8 @@ use crate::core::compiler::{BuildConfig, BuildContext, Compilation, Context}; use crate::core::compiler::{CompileKind, CompileMode, CompileTarget, RustcTargetData, Unit}; use crate::core::compiler::{DefaultExecutor, Executor, UnitInterner}; use crate::core::profiles::{Profiles, UnitFor}; -use crate::core::resolver::features::{self, FeaturesFor, RequestedFeatures}; -use crate::core::resolver::{HasDevUnits, Resolve, ResolveOpts}; +use crate::core::resolver::features::{self, CliFeatures, FeaturesFor}; +use crate::core::resolver::{HasDevUnits, Resolve}; use crate::core::{FeatureValue, Package, PackageSet, Shell, Summary, Target}; use crate::core::{PackageId, PackageIdSpec, SourceId, TargetKind, Workspace}; use crate::drop_println; @@ -59,12 +59,8 @@ use anyhow::Context as _; pub struct CompileOptions { /// Configuration information for a rustc build pub build_config: BuildConfig, - /// Extra features to build for the root package - pub features: Vec, - /// Flag whether all available features should be built for the root package - pub all_features: bool, - /// Flag if the default feature should be built for the root package - pub no_default_features: bool, + /// Feature flags requested by the user. + pub cli_features: CliFeatures, /// A set of packages to build. pub spec: Packages, /// Filter to apply to the root package to select which targets will be @@ -89,9 +85,7 @@ impl<'a> CompileOptions { pub fn new(config: &Config, mode: CompileMode) -> CargoResult { Ok(CompileOptions { build_config: BuildConfig::new(config, None, &[], mode)?, - features: Vec::new(), - all_features: false, - no_default_features: false, + cli_features: CliFeatures::new_all(false), spec: ops::Packages::Packages(Vec::new()), filter: CompileFilter::Default { required_features_filterable: false, @@ -334,9 +328,7 @@ pub fn create_bcx<'a, 'cfg>( let CompileOptions { ref build_config, ref spec, - ref features, - all_features, - no_default_features, + ref cli_features, ref filter, ref target_rustdoc_args, ref target_rustc_args, @@ -372,11 +364,6 @@ pub fn create_bcx<'a, 'cfg>( let target_data = RustcTargetData::new(ws, &build_config.requested_kinds)?; let specs = spec.to_package_id_specs(ws)?; - let dev_deps = ws.require_optional_deps() || filter.need_dev_deps(build_config.mode); - let opts = ResolveOpts::new( - dev_deps, - RequestedFeatures::from_command_line(features, all_features, !no_default_features), - ); let has_dev_units = if filter.need_dev_deps(build_config.mode) { HasDevUnits::Yes } else { @@ -386,7 +373,7 @@ pub fn create_bcx<'a, 'cfg>( ws, &target_data, &build_config.requested_kinds, - &opts, + cli_features, &specs, has_dev_units, crate::core::resolver::features::ForceAllTargets::No, diff --git a/src/cargo/ops/cargo_doc.rs b/src/cargo/ops/cargo_doc.rs index 8755600e338..f9b2d843381 100644 --- a/src/cargo/ops/cargo_doc.rs +++ b/src/cargo/ops/cargo_doc.rs @@ -1,5 +1,5 @@ use crate::core::compiler::RustcTargetData; -use crate::core::resolver::{features::RequestedFeatures, HasDevUnits, ResolveOpts}; +use crate::core::resolver::HasDevUnits; use crate::core::{Shell, Workspace}; use crate::ops; use crate::util::CargoResult; @@ -19,20 +19,12 @@ pub struct DocOptions { /// Main method for `cargo doc`. pub fn doc(ws: &Workspace<'_>, options: &DocOptions) -> CargoResult<()> { let specs = options.compile_opts.spec.to_package_id_specs(ws)?; - let opts = ResolveOpts::new( - /*dev_deps*/ true, - RequestedFeatures::from_command_line( - &options.compile_opts.features, - options.compile_opts.all_features, - !options.compile_opts.no_default_features, - ), - ); let target_data = RustcTargetData::new(ws, &options.compile_opts.build_config.requested_kinds)?; let ws_resolve = ops::resolve_ws_with_opts( ws, &target_data, &options.compile_opts.build_config.requested_kinds, - &opts, + &options.compile_opts.cli_features, &specs, HasDevUnits::No, crate::core::resolver::features::ForceAllTargets::No, diff --git a/src/cargo/ops/cargo_generate_lockfile.rs b/src/cargo/ops/cargo_generate_lockfile.rs index 6ef544da1d6..181e25b128b 100644 --- a/src/cargo/ops/cargo_generate_lockfile.rs +++ b/src/cargo/ops/cargo_generate_lockfile.rs @@ -4,7 +4,7 @@ use log::debug; use termcolor::Color::{self, Cyan, Green, Red}; use crate::core::registry::PackageRegistry; -use crate::core::resolver::ResolveOpts; +use crate::core::resolver::features::{CliFeatures, HasDevUnits}; use crate::core::{PackageId, PackageIdSpec}; use crate::core::{Resolve, SourceId, Workspace}; use crate::ops; @@ -25,7 +25,8 @@ pub fn generate_lockfile(ws: &Workspace<'_>) -> CargoResult<()> { let mut resolve = ops::resolve_with_previous( &mut registry, ws, - &ResolveOpts::everything(), + &CliFeatures::new_all(true), + HasDevUnits::Yes, None, None, &[], @@ -61,7 +62,8 @@ pub fn update_lockfile(ws: &Workspace<'_>, opts: &UpdateOptions<'_>) -> CargoRes ops::resolve_with_previous( &mut registry, ws, - &ResolveOpts::everything(), + &CliFeatures::new_all(true), + HasDevUnits::Yes, None, None, &[], @@ -115,7 +117,8 @@ pub fn update_lockfile(ws: &Workspace<'_>, opts: &UpdateOptions<'_>) -> CargoRes let mut resolve = ops::resolve_with_previous( &mut registry, ws, - &ResolveOpts::everything(), + &CliFeatures::new_all(true), + HasDevUnits::Yes, Some(&previous_resolve), Some(&to_avoid), &[], diff --git a/src/cargo/ops/cargo_output_metadata.rs b/src/cargo/ops/cargo_output_metadata.rs index ed99602f722..baabce47c24 100644 --- a/src/cargo/ops/cargo_output_metadata.rs +++ b/src/cargo/ops/cargo_output_metadata.rs @@ -1,7 +1,7 @@ use crate::core::compiler::{CompileKind, RustcTargetData}; use crate::core::dependency::DepKind; use crate::core::package::SerializedPackage; -use crate::core::resolver::{features::RequestedFeatures, HasDevUnits, Resolve, ResolveOpts}; +use crate::core::resolver::{features::CliFeatures, HasDevUnits, Resolve}; use crate::core::{Dependency, Package, PackageId, Workspace}; use crate::ops::{self, Packages}; use crate::util::interning::InternedString; @@ -14,9 +14,7 @@ use std::path::PathBuf; const VERSION: u32 = 1; pub struct OutputMetadataOptions { - pub features: Vec, - pub no_default_features: bool, - pub all_features: bool, + pub cli_features: CliFeatures, pub no_deps: bool, pub version: u32, pub filter_platforms: Vec, @@ -115,12 +113,6 @@ fn build_resolve_graph( let target_data = RustcTargetData::new(ws, &requested_kinds)?; // Resolve entire workspace. let specs = Packages::All.to_package_id_specs(ws)?; - let requested_features = RequestedFeatures::from_command_line( - &metadata_opts.features, - metadata_opts.all_features, - !metadata_opts.no_default_features, - ); - let resolve_opts = ResolveOpts::new(/*dev_deps*/ true, requested_features); let force_all = if metadata_opts.filter_platforms.is_empty() { crate::core::resolver::features::ForceAllTargets::Yes } else { @@ -133,7 +125,7 @@ fn build_resolve_graph( ws, &target_data, &requested_kinds, - &resolve_opts, + &metadata_opts.cli_features, &specs, HasDevUnits::Yes, force_all, diff --git a/src/cargo/ops/cargo_package.rs b/src/cargo/ops/cargo_package.rs index afa41efd21c..fd37e2c730e 100644 --- a/src/cargo/ops/cargo_package.rs +++ b/src/cargo/ops/cargo_package.rs @@ -7,6 +7,7 @@ use std::rc::Rc; use std::sync::Arc; use crate::core::compiler::{BuildConfig, CompileMode, DefaultExecutor, Executor}; +use crate::core::resolver::CliFeatures; use crate::core::{Feature, Shell, Verbosity, Workspace}; use crate::core::{Package, PackageId, PackageSet, Resolve, Source, SourceId}; use crate::sources::PathSource; @@ -28,9 +29,7 @@ pub struct PackageOpts<'cfg> { pub verify: bool, pub jobs: Option, pub targets: Vec, - pub features: Vec, - pub all_features: bool, - pub no_default_features: bool, + pub cli_features: CliFeatures, } const VCS_INFO_FILE: &str = ".cargo_vcs_info.json"; @@ -689,9 +688,7 @@ fn run_verify(ws: &Workspace<'_>, tar: &FileLock, opts: &PackageOpts<'_>) -> Car &ws, &ops::CompileOptions { build_config: BuildConfig::new(config, opts.jobs, &opts.targets, CompileMode::Build)?, - features: opts.features.clone(), - no_default_features: opts.no_default_features, - all_features: opts.all_features, + cli_features: opts.cli_features.clone(), spec: ops::Packages::Packages(Vec::new()), filter: ops::CompileFilter::Default { required_features_filterable: true, diff --git a/src/cargo/ops/common_for_install_and_uninstall.rs b/src/cargo/ops/common_for_install_and_uninstall.rs index 3ab77a50389..081df2be825 100644 --- a/src/cargo/ops/common_for_install_and_uninstall.rs +++ b/src/cargo/ops/common_for_install_and_uninstall.rs @@ -3,12 +3,13 @@ use std::env; use std::io::prelude::*; use std::io::SeekFrom; use std::path::{Path, PathBuf}; +use std::rc::Rc; use anyhow::{bail, format_err}; use serde::{Deserialize, Serialize}; use crate::core::compiler::Freshness; -use crate::core::{Dependency, Package, PackageId, Source, SourceId}; +use crate::core::{Dependency, FeatureValue, Package, PackageId, Source, SourceId}; use crate::ops::{self, CompileFilter, CompileOptions}; use crate::sources::PathSource; use crate::util::errors::{CargoResult, CargoResultExt}; @@ -422,9 +423,9 @@ impl CrateListingV2 { if let Some(info) = self.installs.get_mut(&pkg.package_id()) { info.bins.append(&mut bins.clone()); info.version_req = version_req; - info.features = feature_set(&opts.features); - info.all_features = opts.all_features; - info.no_default_features = opts.no_default_features; + info.features = feature_set(&opts.cli_features.features); + info.all_features = opts.cli_features.all_features; + info.no_default_features = !opts.cli_features.uses_default_features; info.profile = opts.build_config.requested_profile.to_string(); info.target = Some(target.to_string()); info.rustc = Some(rustc.to_string()); @@ -434,9 +435,9 @@ impl CrateListingV2 { InstallInfo { version_req, bins: bins.clone(), - features: feature_set(&opts.features), - all_features: opts.all_features, - no_default_features: opts.no_default_features, + features: feature_set(&opts.cli_features.features), + all_features: opts.cli_features.all_features, + no_default_features: !opts.cli_features.uses_default_features, profile: opts.build_config.requested_profile.to_string(), target: Some(target.to_string()), rustc: Some(rustc.to_string()), @@ -489,9 +490,9 @@ impl InstallInfo { /// /// This does not do Package/Source/Version checking. fn is_up_to_date(&self, opts: &CompileOptions, target: &str, exes: &BTreeSet) -> bool { - self.features == feature_set(&opts.features) - && self.all_features == opts.all_features - && self.no_default_features == opts.no_default_features + self.features == feature_set(&opts.cli_features.features) + && self.all_features == opts.cli_features.all_features + && self.no_default_features == !opts.cli_features.uses_default_features && self.profile.as_str() == opts.build_config.requested_profile.as_str() && (self.target.is_none() || self.target.as_deref() == Some(target)) && &self.bins == exes @@ -641,9 +642,9 @@ where } } -/// Helper to convert features Vec to a BTreeSet. -fn feature_set(features: &[String]) -> BTreeSet { - features.iter().cloned().collect() +/// Helper to convert features to a BTreeSet. +fn feature_set(features: &Rc>) -> BTreeSet { + features.iter().map(|s| s.to_string()).collect() } /// Helper to get the executable names from a filter. diff --git a/src/cargo/ops/fix.rs b/src/cargo/ops/fix.rs index 9cb7bc41b94..f13ef28b32c 100644 --- a/src/cargo/ops/fix.rs +++ b/src/cargo/ops/fix.rs @@ -52,8 +52,8 @@ use rustfix::diagnostics::Diagnostic; use rustfix::{self, CodeFix}; use crate::core::compiler::RustcTargetData; -use crate::core::resolver::features::{FeatureOpts, FeatureResolver, RequestedFeatures}; -use crate::core::resolver::{HasDevUnits, ResolveBehavior, ResolveOpts}; +use crate::core::resolver::features::{FeatureOpts, FeatureResolver}; +use crate::core::resolver::{HasDevUnits, ResolveBehavior}; use crate::core::{Edition, MaybePackage, Workspace}; use crate::ops::{self, CompileOptions}; use crate::util::diagnostic_server::{Message, RustfixDiagnosticServer}; @@ -228,14 +228,6 @@ fn check_resolver_change(ws: &Workspace<'_>, opts: &FixOptions) -> CargoResult<( // 2018 without `resolver` set must be V1 assert_eq!(ws.resolve_behavior(), ResolveBehavior::V1); let specs = opts.compile_opts.spec.to_package_id_specs(ws)?; - let resolve_opts = ResolveOpts::new( - /*dev_deps*/ true, - RequestedFeatures::from_command_line( - &opts.compile_opts.features, - opts.compile_opts.all_features, - !opts.compile_opts.no_default_features, - ), - ); let target_data = RustcTargetData::new(ws, &opts.compile_opts.build_config.requested_kinds)?; // HasDevUnits::No because that may uncover more differences. // This is not the same as what `cargo fix` is doing, since it is doing @@ -244,7 +236,7 @@ fn check_resolver_change(ws: &Workspace<'_>, opts: &FixOptions) -> CargoResult<( ws, &target_data, &opts.compile_opts.build_config.requested_kinds, - &resolve_opts, + &opts.compile_opts.cli_features, &specs, HasDevUnits::No, crate::core::resolver::features::ForceAllTargets::No, @@ -256,7 +248,7 @@ fn check_resolver_change(ws: &Workspace<'_>, opts: &FixOptions) -> CargoResult<( &target_data, &ws_resolve.targeted_resolve, &ws_resolve.pkg_set, - &resolve_opts.features, + &opts.compile_opts.cli_features, &specs, &opts.compile_opts.build_config.requested_kinds, feature_opts, diff --git a/src/cargo/ops/registry.rs b/src/cargo/ops/registry.rs index 7fef70b65ac..796623db2aa 100644 --- a/src/cargo/ops/registry.rs +++ b/src/cargo/ops/registry.rs @@ -16,6 +16,7 @@ use percent_encoding::{percent_encode, NON_ALPHANUMERIC}; use crate::core::dependency::DepKind; use crate::core::manifest::ManifestMetadata; +use crate::core::resolver::CliFeatures; use crate::core::source::Source; use crate::core::{Package, SourceId, Workspace}; use crate::ops; @@ -52,9 +53,7 @@ pub struct PublishOpts<'cfg> { pub targets: Vec, pub dry_run: bool, pub registry: Option, - pub features: Vec, - pub all_features: bool, - pub no_default_features: bool, + pub cli_features: CliFeatures, } pub fn publish(ws: &Workspace<'_>, opts: &PublishOpts<'_>) -> CargoResult<()> { @@ -112,9 +111,7 @@ pub fn publish(ws: &Workspace<'_>, opts: &PublishOpts<'_>) -> CargoResult<()> { allow_dirty: opts.allow_dirty, targets: opts.targets.clone(), jobs: opts.jobs, - features: opts.features.clone(), - all_features: opts.all_features, - no_default_features: opts.no_default_features, + cli_features: opts.cli_features.clone(), }, )? .unwrap(); diff --git a/src/cargo/ops/resolve.rs b/src/cargo/ops/resolve.rs index ee0eef7c21c..617421a72f5 100644 --- a/src/cargo/ops/resolve.rs +++ b/src/cargo/ops/resolve.rs @@ -13,7 +13,7 @@ use crate::core::compiler::{CompileKind, RustcTargetData}; use crate::core::registry::PackageRegistry; use crate::core::resolver::features::{ - FeatureOpts, FeatureResolver, ForceAllTargets, ResolvedFeatures, + CliFeatures, FeatureOpts, FeatureResolver, ForceAllTargets, RequestedFeatures, ResolvedFeatures, }; use crate::core::resolver::{self, HasDevUnits, Resolve, ResolveOpts, ResolveVersion}; use crate::core::summary::Summary; @@ -80,7 +80,7 @@ pub fn resolve_ws_with_opts<'cfg>( ws: &Workspace<'cfg>, target_data: &RustcTargetData, requested_targets: &[CompileKind], - opts: &ResolveOpts, + cli_features: &CliFeatures, specs: &[PackageIdSpec], has_dev_units: HasDevUnits, force_all_targets: ForceAllTargets, @@ -122,7 +122,8 @@ pub fn resolve_ws_with_opts<'cfg>( let resolved_with_overrides = resolve_with_previous( &mut registry, ws, - opts, + cli_features, + has_dev_units, resolve.as_ref(), None, specs, @@ -132,7 +133,7 @@ pub fn resolve_ws_with_opts<'cfg>( let pkg_set = get_resolved_packages(&resolved_with_overrides, registry)?; let member_ids = ws - .members_with_features(specs, &opts.features)? + .members_with_features(specs, cli_features)? .into_iter() .map(|(p, _fts)| p.package_id()) .collect::>(); @@ -151,7 +152,7 @@ pub fn resolve_ws_with_opts<'cfg>( target_data, &resolved_with_overrides, &pkg_set, - &opts.features, + cli_features, specs, requested_targets, feature_opts, @@ -173,7 +174,8 @@ fn resolve_with_registry<'cfg>( let mut resolve = resolve_with_previous( registry, ws, - &ResolveOpts::everything(), + &CliFeatures::new_all(true), + HasDevUnits::Yes, prev.as_ref(), None, &[], @@ -204,7 +206,8 @@ fn resolve_with_registry<'cfg>( pub fn resolve_with_previous<'cfg>( registry: &mut PackageRegistry<'cfg>, ws: &Workspace<'cfg>, - opts: &ResolveOpts, + cli_features: &CliFeatures, + has_dev_units: HasDevUnits, previous: Option<&Resolve>, to_avoid: Option<&HashSet>, specs: &[PackageIdSpec], @@ -316,16 +319,17 @@ pub fn resolve_with_previous<'cfg>( registry.add_sources(Some(member.package_id().source_id()))?; } + let dev_deps = ws.require_optional_deps() || has_dev_units == HasDevUnits::Yes; let summaries: Vec<(Summary, ResolveOpts)> = ws - .members_with_features(specs, &opts.features)? + .members_with_features(specs, cli_features)? .into_iter() .map(|(member, features)| { let summary = registry.lock(member.summary().clone()); ( summary, ResolveOpts { - dev_deps: opts.dev_deps, - features, + dev_deps, + features: RequestedFeatures::CliFeatures(features), }, ) }) diff --git a/src/cargo/ops/tree/graph.rs b/src/cargo/ops/tree/graph.rs index 3f04a554d9e..034ef0735bb 100644 --- a/src/cargo/ops/tree/graph.rs +++ b/src/cargo/ops/tree/graph.rs @@ -3,7 +3,7 @@ use super::TreeOptions; use crate::core::compiler::{CompileKind, RustcTargetData}; use crate::core::dependency::DepKind; -use crate::core::resolver::features::{FeaturesFor, RequestedFeatures, ResolvedFeatures}; +use crate::core::resolver::features::{CliFeatures, FeaturesFor, ResolvedFeatures}; use crate::core::resolver::Resolve; use crate::core::{FeatureMap, FeatureValue, Package, PackageId, PackageIdSpec, Workspace}; use crate::util::interning::InternedString; @@ -248,16 +248,16 @@ pub fn build<'a>( resolve: &Resolve, resolved_features: &ResolvedFeatures, specs: &[PackageIdSpec], - requested_features: &RequestedFeatures, + cli_features: &CliFeatures, target_data: &RustcTargetData, requested_kinds: &[CompileKind], package_map: HashMap, opts: &TreeOptions, ) -> CargoResult> { let mut graph = Graph::new(package_map); - let mut members_with_features = ws.members_with_features(specs, requested_features)?; + let mut members_with_features = ws.members_with_features(specs, cli_features)?; members_with_features.sort_unstable_by_key(|e| e.0.package_id()); - for (member, requested_features) in members_with_features { + for (member, cli_features) in members_with_features { let member_id = member.package_id(); let features_for = FeaturesFor::from_for_host(member.proc_macro()); for kind in requested_kinds { @@ -273,7 +273,7 @@ pub fn build<'a>( ); if opts.graph_features { let fmap = resolve.summary(member_id).features(); - add_cli_features(&mut graph, member_index, &requested_features, fmap); + add_cli_features(&mut graph, member_index, &cli_features, fmap); } } } @@ -392,7 +392,7 @@ fn add_pkg( EdgeKind::Dep(dep.kind()), ); } - for feature in dep.features() { + for feature in dep.features().iter() { add_feature( graph, *feature, @@ -459,48 +459,66 @@ fn add_feature( fn add_cli_features( graph: &mut Graph<'_>, package_index: usize, - requested_features: &RequestedFeatures, + cli_features: &CliFeatures, feature_map: &FeatureMap, ) { // NOTE: Recursive enabling of features will be handled by // add_internal_features. - // Create a list of feature names requested on the command-line. - let mut to_add: Vec = Vec::new(); - if requested_features.all_features { - to_add.extend(feature_map.keys().copied()); - // Add optional deps. - for (dep_name, deps) in &graph.dep_name_map[&package_index] { - if deps.iter().any(|(_idx, is_optional)| *is_optional) { - to_add.push(*dep_name); - } - } + // Create a set of feature names requested on the command-line. + let mut to_add: HashSet = HashSet::new(); + if cli_features.all_features { + to_add.extend(feature_map.keys().map(|feat| FeatureValue::Feature(*feat))); } else { - if requested_features.uses_default_features { - to_add.push(InternedString::new("default")); + if cli_features.uses_default_features { + to_add.insert(FeatureValue::Feature(InternedString::new("default"))); } - to_add.extend(requested_features.features.iter().copied()); + to_add.extend(cli_features.features.iter().cloned()); }; // Add each feature as a node, and mark as "from command-line" in graph.cli_features. - for name in to_add { - if name.contains('/') { - let mut parts = name.splitn(2, '/'); - let dep_name = InternedString::new(parts.next().unwrap()); - let feat_name = InternedString::new(parts.next().unwrap()); - for (dep_index, is_optional) in graph.dep_name_map[&package_index][&dep_name].clone() { - if is_optional { - // Activate the optional dep on self. - let index = - add_feature(graph, dep_name, None, package_index, EdgeKind::Feature); + for fv in to_add { + match fv { + FeatureValue::Feature(feature) => { + let index = add_feature(graph, feature, None, package_index, EdgeKind::Feature); + graph.cli_features.insert(index); + } + // This is enforced by CliFeatures. + FeatureValue::Dep { .. } => panic!("unexpected cli dep feature {}", fv), + FeatureValue::DepFeature { + dep_name, + dep_feature, + dep_prefix: _, + weak, + } => { + let dep_connections = match graph.dep_name_map[&package_index].get(&dep_name) { + // Clone to deal with immutable borrow of `graph`. :( + Some(dep_connections) => dep_connections.clone(), + None => { + // --features bar?/feat where `bar` is not activated should be ignored. + // If this wasn't weak, then this is a bug. + if weak { + continue; + } + panic!( + "missing dep graph connection for CLI feature `{}` for member {:?}\n\ + Please file a bug report at https://github.com/rust-lang/cargo/issues", + fv, + graph.nodes.get(package_index) + ); + } + }; + for (dep_index, is_optional) in dep_connections { + if is_optional { + // Activate the optional dep on self. + let index = + add_feature(graph, dep_name, None, package_index, EdgeKind::Feature); + graph.cli_features.insert(index); + } + let index = add_feature(graph, dep_feature, None, dep_index, EdgeKind::Feature); graph.cli_features.insert(index); } - let index = add_feature(graph, feat_name, None, dep_index, EdgeKind::Feature); - graph.cli_features.insert(index); } - } else { - let index = add_feature(graph, name, None, package_index, EdgeKind::Feature); - graph.cli_features.insert(index); } } } @@ -570,6 +588,10 @@ fn add_feature_rec( package_index, ); } + // Dependencies are already shown in the graph as dep edges. I'm + // uncertain whether or not this might be confusing in some cases + // (like feature `"somefeat" = ["dep:somedep"]`), so maybe in the + // future consider explicitly showing this? FeatureValue::Dep { .. } => {} FeatureValue::DepFeature { dep_name, diff --git a/src/cargo/ops/tree/mod.rs b/src/cargo/ops/tree/mod.rs index 3ab7da748ea..3f7d6517439 100644 --- a/src/cargo/ops/tree/mod.rs +++ b/src/cargo/ops/tree/mod.rs @@ -3,9 +3,7 @@ use self::format::Pattern; use crate::core::compiler::{CompileKind, RustcTargetData}; use crate::core::dependency::DepKind; -use crate::core::resolver::{ - features::RequestedFeatures, ForceAllTargets, HasDevUnits, ResolveOpts, -}; +use crate::core::resolver::{features::CliFeatures, ForceAllTargets, HasDevUnits}; use crate::core::{Package, PackageId, PackageIdSpec, Workspace}; use crate::ops::{self, Packages}; use crate::util::{CargoResult, Config}; @@ -21,9 +19,7 @@ mod graph; pub use {graph::EdgeKind, graph::Node}; pub struct TreeOptions { - pub features: Vec, - pub no_default_features: bool, - pub all_features: bool, + pub cli_features: CliFeatures, /// The packages to display the tree for. pub packages: Packages, /// The platform to filter for. @@ -138,12 +134,6 @@ pub fn build_and_print(ws: &Workspace<'_>, opts: &TreeOptions) -> CargoResult<() let requested_kinds = CompileKind::from_requested_targets(ws.config(), &requested_targets)?; let target_data = RustcTargetData::new(ws, &requested_kinds)?; let specs = opts.packages.to_package_id_specs(ws)?; - let requested_features = RequestedFeatures::from_command_line( - &opts.features, - opts.all_features, - !opts.no_default_features, - ); - let resolve_opts = ResolveOpts::new(/*dev_deps*/ true, requested_features); let has_dev = if opts .edge_kinds .contains(&EdgeKind::Dep(DepKind::Development)) @@ -161,7 +151,7 @@ pub fn build_and_print(ws: &Workspace<'_>, opts: &TreeOptions) -> CargoResult<() ws, &target_data, &requested_kinds, - &resolve_opts, + &opts.cli_features, &specs, has_dev, force_all, @@ -178,7 +168,7 @@ pub fn build_and_print(ws: &Workspace<'_>, opts: &TreeOptions) -> CargoResult<() &ws_resolve.targeted_resolve, &ws_resolve.resolved_features, &specs, - &resolve_opts.features, + &opts.cli_features, &target_data, &requested_kinds, package_map, diff --git a/src/cargo/util/command_prelude.rs b/src/cargo/util/command_prelude.rs index de793b68dce..3b5d9a7e23a 100644 --- a/src/cargo/util/command_prelude.rs +++ b/src/cargo/util/command_prelude.rs @@ -1,4 +1,5 @@ use crate::core::compiler::{BuildConfig, MessageFormat}; +use crate::core::resolver::CliFeatures; use crate::core::{Edition, Workspace}; use crate::ops::{CompileFilter, CompileOptions, NewOptions, Packages, VersionControl}; use crate::sources::CRATES_IO_REGISTRY; @@ -496,9 +497,7 @@ pub trait ArgMatchesExt { let opts = CompileOptions { build_config, - features: self._values_of("features"), - all_features: self._is_present("all-features"), - no_default_features: self._is_present("no-default-features"), + cli_features: self.cli_features()?, spec, filter: CompileFilter::from_raw_arguments( self._is_present("lib"), @@ -540,6 +539,14 @@ pub trait ArgMatchesExt { Ok(opts) } + fn cli_features(&self) -> CargoResult { + CliFeatures::from_command_line( + &self._values_of("features"), + self._is_present("all-features"), + !self._is_present("no-default-features"), + ) + } + fn compile_options_for_single_package( &self, config: &Config, diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index ac2f90aadb9..be8a545ef10 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -1699,6 +1699,35 @@ impl DetailedTomlDependency

{ } } + // Early detection of potentially misused feature syntax + // instead of generating a "feature not found" error. + if let Some(features) = &self.features { + for feature in features { + if feature.contains('/') { + bail!( + "feature `{}` in dependency `{}` is not allowed to contain slashes\n\ + If you want to enable features of a transitive dependency, \ + the direct dependency needs to re-export those features from \ + the `[features]` table.", + feature, + name_in_toml + ); + } + if feature.starts_with("dep:") { + bail!( + "feature `{}` in dependency `{}` is not allowed to use explicit \ + `dep:` syntax\n\ + If you want to enable an optional dependency, specify the name \ + of the optional dependency without the `dep:` prefix, or specify \ + a feature from the dependency's `[features]` table that enables \ + the optional dependency.", + feature, + name_in_toml + ); + } + } + } + let new_source_id = match ( self.git.as_ref(), self.path.as_ref(), diff --git a/tests/testsuite/features.rs b/tests/testsuite/features.rs index c4bc09ad473..49a61301bac 100644 --- a/tests/testsuite/features.rs +++ b/tests/testsuite/features.rs @@ -269,7 +269,15 @@ fn invalid8() { p.cargo("build --features foo") .with_status(101) - .with_stderr("[ERROR] feature names may not contain slashes: `foo/bar`") + .with_stderr( + "\ +error: failed to parse manifest at `[CWD]/Cargo.toml` + +Caused by: + feature `foo/bar` in dependency `bar` is not allowed to contain slashes + If you want to enable features [..] +", + ) .run(); } @@ -409,7 +417,14 @@ fn no_transitive_dep_feature_requirement() { .build(); p.cargo("build") .with_status(101) - .with_stderr("[ERROR] feature names may not contain slashes: `bar/qux`") + .with_stderr( + "\ +error: failed to parse manifest at `[CWD]/Cargo.toml` + +Caused by: + multiple slashes in feature `derived/bar/qux` (included by feature `default`) are not allowed +", + ) .run(); } @@ -1192,7 +1207,7 @@ fn dep_feature_in_cmd_line() { // Hierarchical feature specification should still be disallowed p.cargo("build --features derived/bar/some-feat") .with_status(101) - .with_stderr("[ERROR] feature names may not contain slashes: `bar/some-feat`") + .with_stderr("[ERROR] multiple slashes in feature `derived/bar/some-feat` is not allowed") .run(); } @@ -1906,7 +1921,7 @@ fn nonexistent_required_features() { } #[cargo_test] -fn invalid_feature_names() { +fn invalid_feature_names_warning() { // Warnings for more restricted feature syntax. let p = project() .file( @@ -1929,7 +1944,6 @@ fn invalid_feature_names() { "+foo" = [] "-foo" = [] ".foo" = [] - "foo/bar" = [] "foo:bar" = [] "foo?" = [] "?foo" = [] @@ -1961,9 +1975,6 @@ For more information, see issue #8813 , and please leave a comment if this will be a problem for your project. -[WARNING] invalid character `/` in feature `foo/bar` in package foo v0.1.0 ([ROOT]/foo), characters must be Unicode XID characters, `+`, or `.` (numbers, `+`, `-`, `_`, `.`, or most letters) -This was previously accepted but is being phased out; it will become a hard error in a future release. -For more information, see issue #8813 , and please leave a comment if this will be a problem for your project. [WARNING] invalid character `:` in feature `foo:bar` in package foo v0.1.0 ([ROOT]/foo), characters must be Unicode XID characters, `+`, or `.` (numbers, `+`, `-`, `_`, `.`, or most letters) This was previously accepted but is being phased out; it will become a hard error in a future release. For more information, see issue #8813 , and please leave a comment if this will be a problem for your project. @@ -1994,9 +2005,6 @@ For more information, see issue #8813 , and please leave a comment if this will be a problem for your project. -[WARNING] invalid character `/` in feature `foo/bar` in package foo v0.1.0 ([ROOT]/foo), characters must be Unicode XID characters, `+`, or `.` (numbers, `+`, `-`, `_`, `.`, or most letters) -This was previously accepted but is being phased out; it will become a hard error in a future release. -For more information, see issue #8813 , and please leave a comment if this will be a problem for your project. [WARNING] invalid character `:` in feature `foo:bar` in package foo v0.1.0 ([ROOT]/foo), characters must be Unicode XID characters, `+`, or `.` (numbers, `+`, `-`, `_`, `.`, or most letters) This was previously accepted but is being phased out; it will become a hard error in a future release. For more information, see issue #8813 , and please leave a comment if this will be a problem for your project. @@ -2017,3 +2025,34 @@ For more information, see issue #8813 Did you mean `foo`? +", + ) + .run(); + + p.cargo("check -p bar --features bar -v") + .with_stderr( + "\ +[FRESH] bar v1.0.0 +[FINISHED] [..] +", + ) + .run(); + + // TODO: This should not be allowed (future warning?) + p.cargo("check -p bar --features bar/jazz -v") + .with_stderr( + "\ +[FRESH] jazz v1.0.0 +[FRESH] bar v1.0.0 +[FINISHED] [..] +", + ) + .run(); + + ///////////////////////// V2 non-optional + eprintln!("V2 non-optional"); + p.change_file("Cargo.toml", &make_toml("2", false)); + // TODO: This should not be allowed (future warning?) + p.cargo("check --features bar/jazz -v") + .with_stderr( + "\ +[FRESH] jazz v1.0.0 +[FRESH] bar v1.0.0 +[FRESH] foo v0.1.0 [..] +[FINISHED] [..] +", + ) + .run(); + p.cargo("check -p bar -v") + .with_stderr( + "\ +[FRESH] bar v1.0.0 +[FINISHED] [..] +", + ) + .run(); + p.cargo("check -p bar --features bar/jazz") + .with_status(101) + .with_stderr("error: cannot specify features for packages outside of workspace") + .run(); + + ///////////////////////// V2 optional + eprintln!("V2 optional"); + p.change_file("Cargo.toml", &make_toml("2", true)); + p.cargo("check -p bar") + .with_status(101) + .with_stderr( + "\ +error: package ID specification `bar` did not match any packages + +Did you mean `foo`? +", + ) + .run(); + // New --features behavior does not look at cwd. + p.cargo("check -p bar --features bar") + .with_status(101) + .with_stderr("error: cannot specify features for packages outside of workspace") + .run(); + p.cargo("check -p bar --features bar/jazz") + .with_status(101) + .with_stderr("error: cannot specify features for packages outside of workspace") + .run(); + p.cargo("check -p bar --features foo/bar") + .with_status(101) + .with_stderr("error: cannot specify features for packages outside of workspace") + .run(); } diff --git a/tests/testsuite/weak_dep_features.rs b/tests/testsuite/weak_dep_features.rs index 338bf8968fa..d51c407727f 100644 --- a/tests/testsuite/weak_dep_features.rs +++ b/tests/testsuite/weak_dep_features.rs @@ -1,5 +1,7 @@ //! Tests for weak-dep-features. +use super::features2::switch_to_resolver_2; +use cargo_test_support::paths::CargoPathExt; use cargo_test_support::registry::{Dependency, Package}; use cargo_test_support::{project, publish}; use std::fmt::Write; @@ -272,6 +274,7 @@ fn optional_cli_syntax() { .file("src/lib.rs", "") .build(); + // Does not build bar. p.cargo("check --features bar?/feat -Z weak-dep-features") .masquerade_as_nightly_cargo() .with_stderr( @@ -285,6 +288,33 @@ fn optional_cli_syntax() { ) .run(); + // Builds bar. + p.cargo("check --features bar?/feat,bar -Z weak-dep-features") + .masquerade_as_nightly_cargo() + .with_stderr( + "\ +[CHECKING] bar v1.0.0 +[CHECKING] foo v0.1.0 [..] +[FINISHED] [..] +", + ) + .run(); + + eprintln!("check V2 resolver"); + switch_to_resolver_2(&p); + p.build_dir().rm_rf(); + // Does not build bar. + p.cargo("check --features bar?/feat -Z weak-dep-features") + .masquerade_as_nightly_cargo() + .with_stderr( + "\ +[CHECKING] foo v0.1.0 [..] +[FINISHED] [..] +", + ) + .run(); + + // Builds bar. p.cargo("check --features bar?/feat,bar -Z weak-dep-features") .masquerade_as_nightly_cargo() .with_stderr( @@ -561,6 +591,32 @@ bar v1.0.0 │ └── foo feature \"f1\" (command-line) └── bar feature \"feat\" └── foo feature \"f1\" (command-line) +", + ) + .run(); + + p.cargo("tree -Z weak-dep-features -e features --features bar?/feat") + .masquerade_as_nightly_cargo() + .with_stdout("foo v0.1.0 ([ROOT]/foo)") + .run(); + + // This is a little strange in that it produces no output. + // Maybe `cargo tree` should print a note about why? + p.cargo("tree -Z weak-dep-features -e features -i bar --features bar?/feat") + .masquerade_as_nightly_cargo() + .with_stdout("") + .run(); + + p.cargo("tree -Z weak-dep-features -e features -i bar --features bar?/feat,bar") + .masquerade_as_nightly_cargo() + .with_stdout( + "\ +bar v1.0.0 +├── bar feature \"default\" +│ └── foo v0.1.0 ([ROOT]/foo) +│ ├── foo feature \"bar\" (command-line) +│ └── foo feature \"default\" (command-line) +└── bar feature \"feat\" (command-line) ", ) .run();