From 949eccac3e0d7df8ef705054fa45926ce8005291 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Tue, 21 Jan 2020 17:12:41 -0800 Subject: [PATCH 01/22] Move rustc target collection out of bcx. This allows querying it earlier, and independently of the rest of stuff in BuildContext. --- src/cargo/core/compiler/build_context/mod.rs | 105 ++++-------------- .../compiler/build_context/target_info.rs | 102 +++++++++++++++-- src/cargo/core/compiler/compilation.rs | 20 +++- src/cargo/core/compiler/compile_kind.rs | 10 -- .../compiler/context/compilation_files.rs | 19 ++-- src/cargo/core/compiler/context/mod.rs | 2 +- src/cargo/core/compiler/custom_build.rs | 8 +- src/cargo/core/compiler/fingerprint.rs | 4 +- src/cargo/core/compiler/mod.rs | 2 +- src/cargo/core/compiler/timings.rs | 8 +- src/cargo/core/compiler/unit_dependencies.rs | 4 +- src/cargo/ops/cargo_clean.rs | 4 +- src/cargo/ops/cargo_compile.rs | 5 +- 13 files changed, 160 insertions(+), 133 deletions(-) diff --git a/src/cargo/core/compiler/build_context/mod.rs b/src/cargo/core/compiler/build_context/mod.rs index ee39248f51b..bf3e1c4c98c 100644 --- a/src/cargo/core/compiler/build_context/mod.rs +++ b/src/cargo/core/compiler/build_context/mod.rs @@ -1,19 +1,17 @@ use crate::core::compiler::unit::UnitInterner; -use crate::core::compiler::CompileTarget; use crate::core::compiler::{BuildConfig, BuildOutput, CompileKind, Unit}; use crate::core::profiles::Profiles; -use crate::core::{Dependency, InternedString, Workspace}; +use crate::core::{InternedString, Workspace}; use crate::core::{PackageId, PackageSet}; -use crate::util::config::{Config, TargetConfig}; +use crate::util::config::Config; use crate::util::errors::CargoResult; use crate::util::Rustc; -use cargo_platform::Cfg; use std::collections::HashMap; use std::path::PathBuf; use std::str; mod target_info; -pub use self::target_info::{FileFlavor, TargetInfo}; +pub use self::target_info::{FileFlavor, RustcTargetData, TargetInfo}; /// The build context, containing all information about a build task. /// @@ -30,26 +28,14 @@ pub struct BuildContext<'a, 'cfg> { pub build_config: &'a BuildConfig, /// Extra compiler args for either `rustc` or `rustdoc`. pub extra_compiler_args: HashMap, Vec>, + /// Package downloader. pub packages: &'a PackageSet<'cfg>, /// Source of interning new units as they're created. pub units: &'a UnitInterner<'a>, - /// Information about the compiler that we've detected on the local system. - pub rustc: Rustc, - - /// Build information for the "host", which is information about when - /// `rustc` is invoked without a `--target` flag. This is used for - /// procedural macros, build scripts, etc. - host_config: TargetConfig, - host_info: TargetInfo, - - /// Build information for targets that we're building for. This will be - /// empty if the `--target` flag is not passed, and currently also only ever - /// has at most one entry, but eventually we'd like to support multi-target - /// builds with Cargo. - target_config: HashMap, - target_info: HashMap, + /// Information about rustc and the target platform. + pub target_data: RustcTargetData, } impl<'a, 'cfg> BuildContext<'a, 'cfg> { @@ -61,74 +47,33 @@ impl<'a, 'cfg> BuildContext<'a, 'cfg> { profiles: Profiles, units: &'a UnitInterner<'a>, extra_compiler_args: HashMap, Vec>, + target_data: RustcTargetData, ) -> CargoResult> { - let rustc = config.load_global_rustc(Some(ws))?; - - let host_config = config.target_cfg_triple(&rustc.host)?; - let host_info = TargetInfo::new( - config, - build_config.requested_kind, - &rustc, - CompileKind::Host, - )?; - let mut target_config = HashMap::new(); - let mut target_info = HashMap::new(); - if let CompileKind::Target(target) = build_config.requested_kind { - let tcfg = config.target_cfg_triple(target.short_name())?; - target_config.insert(target, tcfg); - target_info.insert( - target, - TargetInfo::new( - config, - build_config.requested_kind, - &rustc, - CompileKind::Target(target), - )?, - ); - } - Ok(BuildContext { ws, packages, config, - rustc, - target_config, - target_info, - host_config, - host_info, build_config, profiles, extra_compiler_args, units, + target_data, }) } - /// Whether a dependency should be compiled for the host or target platform, - /// specified by `CompileKind`. - pub fn dep_platform_activated(&self, dep: &Dependency, kind: CompileKind) -> bool { - // If this dependency is only available for certain platforms, - // make sure we're only enabling it for that platform. - let platform = match dep.platform() { - Some(p) => p, - None => return true, - }; - let name = kind.short_name(self); - platform.matches(name, self.cfg(kind)) + pub fn rustc(&self) -> &Rustc { + &self.target_data.rustc } /// Gets the user-specified linker for a particular host or target. pub fn linker(&self, kind: CompileKind) -> Option { - self.target_config(kind) + self.target_data + .target_config(kind) .linker .as_ref() .map(|l| l.val.clone().resolve_program(self.config)) } - /// Gets the list of `cfg`s printed out from the compiler for the specified kind. - pub fn cfg(&self, kind: CompileKind) -> &[Cfg] { - self.info(kind).cfg() - } - /// Gets the host architecture triple. /// /// For example, x86_64-unknown-linux-gnu, would be @@ -136,15 +81,7 @@ impl<'a, 'cfg> BuildContext<'a, 'cfg> { /// - hardware-platform: unknown, /// - operating system: linux-gnu. pub fn host_triple(&self) -> InternedString { - self.rustc.host - } - - /// Gets the target configuration for a particular host or target. - pub fn target_config(&self, kind: CompileKind) -> &TargetConfig { - match kind { - CompileKind::Host => &self.host_config, - CompileKind::Target(s) => &self.target_config[&s], - } + self.target_data.rustc.host } /// Gets the number of jobs specified for this build. @@ -153,24 +90,17 @@ impl<'a, 'cfg> BuildContext<'a, 'cfg> { } pub fn rustflags_args(&self, unit: &Unit<'_>) -> &[String] { - &self.info(unit.kind).rustflags + &self.target_data.info(unit.kind).rustflags } pub fn rustdocflags_args(&self, unit: &Unit<'_>) -> &[String] { - &self.info(unit.kind).rustdocflags + &self.target_data.info(unit.kind).rustdocflags } pub fn show_warnings(&self, pkg: PackageId) -> bool { pkg.source_id().is_path() || self.config.extra_verbose() } - pub fn info(&self, kind: CompileKind) -> &TargetInfo { - match kind { - CompileKind::Host => &self.host_info, - CompileKind::Target(s) => &self.target_info[&s], - } - } - pub fn extra_args_for(&self, unit: &Unit<'a>) -> Option<&Vec> { self.extra_compiler_args.get(unit) } @@ -180,6 +110,9 @@ impl<'a, 'cfg> BuildContext<'a, 'cfg> { /// `lib_name` is the `links` library name and `kind` is whether it is for /// Host or Target. pub fn script_override(&self, lib_name: &str, kind: CompileKind) -> Option<&BuildOutput> { - self.target_config(kind).links_overrides.get(lib_name) + self.target_data + .target_config(kind) + .links_overrides + .get(lib_name) } } diff --git a/src/cargo/core/compiler/build_context/target_info.rs b/src/cargo/core/compiler/build_context/target_info.rs index 6cb942a8ad8..b1454802d5c 100644 --- a/src/cargo/core/compiler/build_context/target_info.rs +++ b/src/cargo/core/compiler/build_context/target_info.rs @@ -1,18 +1,18 @@ +use crate::core::compiler::CompileKind; +use crate::core::compiler::CompileTarget; +use crate::core::{Dependency, TargetKind, Workspace}; +use crate::util::config::{Config, StringList, TargetConfig}; +use crate::util::{CargoResult, CargoResultExt, ProcessBuilder, Rustc}; +use cargo_platform::{Cfg, CfgExpr}; use std::cell::RefCell; use std::collections::hash_map::{Entry, HashMap}; use std::env; use std::path::PathBuf; use std::str::{self, FromStr}; -use crate::core::compiler::CompileKind; -use crate::core::TargetKind; -use crate::util::config::StringList; -use crate::util::{CargoResult, CargoResultExt, Config, ProcessBuilder, Rustc}; -use cargo_platform::{Cfg, CfgExpr}; - /// Information about the platform target gleaned from querying rustc. /// -/// The `BuildContext` keeps two of these, one for the host and one for the +/// `RustcTargetData` keeps two of these, one for the host and one for the /// target. If no target is specified, it uses a clone from the host. #[derive(Clone)] pub struct TargetInfo { @@ -468,3 +468,91 @@ fn env_args( Ok(Vec::new()) } + +/// Collection of information about `rustc` and the host and target. +pub struct RustcTargetData { + /// Information about `rustc` itself. + pub rustc: Rustc, + /// Build information for the "host", which is information about when + /// `rustc` is invoked without a `--target` flag. This is used for + /// procedural macros, build scripts, etc. + host_config: TargetConfig, + host_info: TargetInfo, + + /// Build information for targets that we're building for. This will be + /// empty if the `--target` flag is not passed, and currently also only ever + /// has at most one entry, but eventually we'd like to support multi-target + /// builds with Cargo. + target_config: HashMap, + target_info: HashMap, +} + +impl RustcTargetData { + pub fn new(ws: &Workspace<'_>, requested_kind: CompileKind) -> CargoResult { + let config = ws.config(); + let rustc = config.load_global_rustc(Some(ws))?; + let host_config = config.target_cfg_triple(&rustc.host)?; + let host_info = TargetInfo::new(config, requested_kind, &rustc, CompileKind::Host)?; + let mut target_config = HashMap::new(); + let mut target_info = HashMap::new(); + if let CompileKind::Target(target) = requested_kind { + let tcfg = config.target_cfg_triple(target.short_name())?; + target_config.insert(target, tcfg); + target_info.insert( + target, + TargetInfo::new(config, requested_kind, &rustc, CompileKind::Target(target))?, + ); + } + + Ok(RustcTargetData { + rustc, + target_config, + target_info, + host_config, + host_info, + }) + } + + /// Returns a "short" name for the given kind, suitable for keying off + /// configuration in Cargo or presenting to users. + pub fn short_name<'a>(&'a self, kind: &'a CompileKind) -> &'a str { + match kind { + CompileKind::Host => &self.rustc.host, + CompileKind::Target(target) => target.short_name(), + } + } + + /// Whether a dependency should be compiled for the host or target platform, + /// specified by `CompileKind`. + pub fn dep_platform_activated(&self, dep: &Dependency, kind: CompileKind) -> bool { + // If this dependency is only available for certain platforms, + // make sure we're only enabling it for that platform. + let platform = match dep.platform() { + Some(p) => p, + None => return true, + }; + let name = self.short_name(&kind); + platform.matches(name, self.cfg(kind)) + } + + /// Gets the list of `cfg`s printed out from the compiler for the specified kind. + pub fn cfg(&self, kind: CompileKind) -> &[Cfg] { + self.info(kind).cfg() + } + + /// Information about the given target platform, learned by querying rustc. + pub fn info(&self, kind: CompileKind) -> &TargetInfo { + match kind { + CompileKind::Host => &self.host_info, + CompileKind::Target(s) => &self.target_info[&s], + } + } + + /// Gets the target configuration for a particular host or target. + pub fn target_config(&self, kind: CompileKind) -> &TargetConfig { + match kind { + CompileKind::Host => &self.host_config, + CompileKind::Target(s) => &self.target_config[&s], + } + } +} diff --git a/src/cargo/core/compiler/compilation.rs b/src/cargo/core/compiler/compilation.rs index f91ca922bf9..1ac7f5be172 100644 --- a/src/cargo/core/compiler/compilation.rs +++ b/src/cargo/core/compiler/compilation.rs @@ -84,7 +84,7 @@ impl<'cfg> Compilation<'cfg> { bcx: &BuildContext<'a, 'cfg>, default_kind: CompileKind, ) -> CargoResult> { - let mut rustc = bcx.rustc.process(); + let mut rustc = bcx.rustc().process(); let mut primary_unit_rustc_process = bcx.build_config.primary_unit_rustc.clone(); @@ -102,8 +102,16 @@ impl<'cfg> Compilation<'cfg> { root_output: PathBuf::from("/"), deps_output: PathBuf::from("/"), host_deps_output: PathBuf::from("/"), - host_dylib_path: bcx.info(CompileKind::Host).sysroot_host_libdir.clone(), - target_dylib_path: bcx.info(default_kind).sysroot_target_libdir.clone(), + host_dylib_path: bcx + .target_data + .info(CompileKind::Host) + .sysroot_host_libdir + .clone(), + target_dylib_path: bcx + .target_data + .info(default_kind) + .sysroot_target_libdir + .clone(), tests: Vec::new(), binaries: Vec::new(), extra_env: HashMap::new(), @@ -114,7 +122,7 @@ impl<'cfg> Compilation<'cfg> { rustc_process: rustc, primary_unit_rustc_process, host: bcx.host_triple().to_string(), - target: default_kind.short_name(bcx).to_string(), + target: bcx.target_data.short_name(&default_kind).to_string(), target_runner: target_runner(bcx, default_kind)?, }) } @@ -286,7 +294,7 @@ fn target_runner( bcx: &BuildContext<'_, '_>, kind: CompileKind, ) -> CargoResult)>> { - let target = kind.short_name(bcx); + let target = bcx.target_data.short_name(&kind); // try target.{}.runner let key = format!("target.{}.runner", target); @@ -296,7 +304,7 @@ fn target_runner( } // try target.'cfg(...)'.runner - let target_cfg = bcx.info(kind).cfg(); + let target_cfg = bcx.target_data.info(kind).cfg(); let mut cfgs = bcx .config .target_cfgs()? diff --git a/src/cargo/core/compiler/compile_kind.rs b/src/cargo/core/compiler/compile_kind.rs index 551f01c1109..ef23dbfa150 100644 --- a/src/cargo/core/compiler/compile_kind.rs +++ b/src/cargo/core/compiler/compile_kind.rs @@ -1,4 +1,3 @@ -use crate::core::compiler::BuildContext; use crate::core::{InternedString, Target}; use crate::util::errors::{CargoResult, CargoResultExt}; use serde::Serialize; @@ -40,15 +39,6 @@ impl CompileKind { CompileKind::Target(n) => CompileKind::Target(n), } } - - /// Returns a "short" name for this kind, suitable for keying off - /// configuration in Cargo or presenting to users. - pub fn short_name(&self, bcx: &BuildContext<'_, '_>) -> &str { - match self { - CompileKind::Host => bcx.host_triple().as_str(), - CompileKind::Target(target) => target.short_name(), - } - } } /// Abstraction for the representation of a compilation target that Cargo has. diff --git a/src/cargo/core/compiler/context/compilation_files.rs b/src/cargo/core/compiler/context/compilation_files.rs index 09d98740b8e..b9a08bc706f 100644 --- a/src/cargo/core/compiler/context/compilation_files.rs +++ b/src/cargo/core/compiler/context/compilation_files.rs @@ -259,13 +259,13 @@ impl<'a, 'cfg: 'a> CompilationFiles<'a, 'cfg> { ) -> CargoResult { assert!(target.is_bin()); let dest = self.layout(kind).dest(); - let info = bcx.info(kind); + let info = bcx.target_data.info(kind); let file_types = info .file_types( "bin", FileFlavor::Normal, &TargetKind::Bin, - kind.short_name(bcx), + bcx.target_data.short_name(&kind), )? .expect("target must support `bin`"); @@ -402,7 +402,7 @@ impl<'a, 'cfg: 'a> CompilationFiles<'a, 'cfg> { let out_dir = self.out_dir(unit); let link_stem = self.link_stem(unit); - let info = bcx.info(unit.kind); + let info = bcx.target_data.info(unit.kind); let file_stem = self.file_stem(unit); let mut add = |crate_type: &str, flavor: FileFlavor| -> CargoResult<()> { @@ -415,7 +415,7 @@ impl<'a, 'cfg: 'a> CompilationFiles<'a, 'cfg> { crate_type, flavor, unit.target.kind(), - unit.kind.short_name(bcx), + bcx.target_data.short_name(&unit.kind), )?; match file_types { @@ -489,14 +489,14 @@ impl<'a, 'cfg: 'a> CompilationFiles<'a, 'cfg> { does not support these crate types", unsupported.join(", "), unit.pkg, - unit.kind.short_name(bcx), + bcx.target_data.short_name(&unit.kind), ) } anyhow::bail!( "cannot compile `{}` as the target `{}` does not \ support any of the output crate types", unit.pkg, - unit.kind.short_name(bcx), + bcx.target_data.short_name(&unit.kind), ); } Ok(ret) @@ -551,11 +551,12 @@ fn compute_metadata<'a, 'cfg>( // doing this eventually. let bcx = &cx.bcx; let __cargo_default_lib_metadata = env::var("__CARGO_DEFAULT_LIB_METADATA"); + let short_name = bcx.target_data.short_name(&unit.kind); if !(unit.mode.is_any_test() || unit.mode.is_check()) && (unit.target.is_dylib() || unit.target.is_cdylib() - || (unit.target.is_executable() && unit.kind.short_name(bcx).starts_with("wasm32-")) - || (unit.target.is_executable() && unit.kind.short_name(bcx).contains("msvc"))) + || (unit.target.is_executable() && short_name.starts_with("wasm32-")) + || (unit.target.is_executable() && short_name.contains("msvc"))) && unit.pkg.package_id().source_id().is_path() && __cargo_default_lib_metadata.is_err() { @@ -609,7 +610,7 @@ fn compute_metadata<'a, 'cfg>( unit.target.name().hash(&mut hasher); unit.target.kind().hash(&mut hasher); - bcx.rustc.verbose_version.hash(&mut hasher); + bcx.rustc().verbose_version.hash(&mut hasher); if cx.is_primary_package(unit) { // This is primarily here for clippy. This ensures that the clippy diff --git a/src/cargo/core/compiler/context/mod.rs b/src/cargo/core/compiler/context/mod.rs index 197dcaca17e..fdc002d35c2 100644 --- a/src/cargo/core/compiler/context/mod.rs +++ b/src/cargo/core/compiler/context/mod.rs @@ -450,7 +450,7 @@ impl<'a, 'cfg> Context<'a, 'cfg> { suggestion, crate::version(), self.bcx.host_triple(), - unit.kind.short_name(self.bcx), + self.bcx.target_data.short_name(&unit.kind), unit, other_unit)) } diff --git a/src/cargo/core/compiler/custom_build.rs b/src/cargo/core/compiler/custom_build.rs index be03e05b5da..488d9cc3d00 100644 --- a/src/cargo/core/compiler/custom_build.rs +++ b/src/cargo/core/compiler/custom_build.rs @@ -180,7 +180,7 @@ fn build_work<'a, 'cfg>(cx: &mut Context<'a, 'cfg>, unit: &Unit<'a>) -> CargoRes cmd.env("OUT_DIR", &script_out_dir) .env("CARGO_MANIFEST_DIR", unit.pkg.root()) .env("NUM_JOBS", &bcx.jobs().to_string()) - .env("TARGET", unit.kind.short_name(bcx)) + .env("TARGET", bcx.target_data.short_name(&unit.kind)) .env("DEBUG", debug.to_string()) .env("OPT_LEVEL", &unit.profile.opt_level.to_string()) .env( @@ -191,11 +191,11 @@ fn build_work<'a, 'cfg>(cx: &mut Context<'a, 'cfg>, unit: &Unit<'a>) -> CargoRes }, ) .env("HOST", &bcx.host_triple()) - .env("RUSTC", &bcx.rustc.path) + .env("RUSTC", &bcx.rustc().path) .env("RUSTDOC", &*bcx.config.rustdoc()?) .inherit_jobserver(&cx.jobserver); - if let Some(linker) = &bcx.target_config(unit.kind).linker { + if let Some(linker) = &bcx.target_data.target_config(unit.kind).linker { cmd.env( "RUSTC_LINKER", linker.val.clone().resolve_program(bcx.config), @@ -213,7 +213,7 @@ fn build_work<'a, 'cfg>(cx: &mut Context<'a, 'cfg>, unit: &Unit<'a>) -> CargoRes } let mut cfg_map = HashMap::new(); - for cfg in bcx.cfg(unit.kind) { + for cfg in bcx.target_data.cfg(unit.kind) { match *cfg { Cfg::Name(ref n) => { cfg_map.insert(n.clone(), None); diff --git a/src/cargo/core/compiler/fingerprint.rs b/src/cargo/core/compiler/fingerprint.rs index ce0dee7e6f9..b700baad4aa 100644 --- a/src/cargo/core/compiler/fingerprint.rs +++ b/src/cargo/core/compiler/fingerprint.rs @@ -1122,7 +1122,7 @@ fn calculate_normal<'a, 'cfg>( let m = unit.pkg.manifest().metadata(); let metadata = util::hash_u64((&m.authors, &m.description, &m.homepage, &m.repository)); Ok(Fingerprint { - rustc: util::hash_u64(&cx.bcx.rustc.verbose_version), + rustc: util::hash_u64(&cx.bcx.rustc().verbose_version), target: util::hash_u64(&unit.target), profile: profile_hash, // Note that .0 is hashed here, not .1 which is the cwd. That doesn't @@ -1180,7 +1180,7 @@ fn calculate_run_custom_build<'a, 'cfg>( Ok(Fingerprint { local: Mutex::new(local), - rustc: util::hash_u64(&cx.bcx.rustc.verbose_version), + rustc: util::hash_u64(&cx.bcx.rustc().verbose_version), deps, outputs: if overridden { Vec::new() } else { vec![output] }, diff --git a/src/cargo/core/compiler/mod.rs b/src/cargo/core/compiler/mod.rs index 096b185f13e..8dce6ff31ae 100644 --- a/src/cargo/core/compiler/mod.rs +++ b/src/cargo/core/compiler/mod.rs @@ -28,7 +28,7 @@ use lazycell::LazyCell; use log::debug; pub use self::build_config::{BuildConfig, CompileMode, MessageFormat}; -pub use self::build_context::{BuildContext, FileFlavor, TargetInfo}; +pub use self::build_context::{BuildContext, FileFlavor, RustcTargetData, TargetInfo}; use self::build_plan::BuildPlan; pub use self::compilation::{Compilation, Doctest}; pub use self::compile_kind::{CompileKind, CompileTarget}; diff --git a/src/cargo/core/compiler/timings.rs b/src/cargo/core/compiler/timings.rs index e8bdac83e4a..0983ad012ea 100644 --- a/src/cargo/core/compiler/timings.rs +++ b/src/cargo/core/compiler/timings.rs @@ -605,15 +605,17 @@ fn d_as_f64(d: Duration) -> f64 { fn render_rustc_info(bcx: &BuildContext<'_, '_>) -> String { let version = bcx - .rustc + .rustc() .verbose_version .lines() .next() .expect("rustc version"); - let requested_target = bcx.build_config.requested_kind.short_name(bcx); + let requested_target = bcx.target_data.short_name(&bcx.build_config.requested_kind); format!( "{}
Host: {}
Target: {}", - version, bcx.rustc.host, requested_target + version, + bcx.rustc().host, + requested_target ) } diff --git a/src/cargo/core/compiler/unit_dependencies.rs b/src/cargo/core/compiler/unit_dependencies.rs index 48095cdb16c..a530683c091 100644 --- a/src/cargo/core/compiler/unit_dependencies.rs +++ b/src/cargo/core/compiler/unit_dependencies.rs @@ -250,7 +250,7 @@ fn compute_deps<'a, 'cfg>( // If this dependency is only available for certain platforms, // make sure we're only enabling it for that platform. - if !bcx.dep_platform_activated(dep, unit.kind) { + if !bcx.target_data.dep_platform_activated(dep, unit.kind) { return false; } @@ -396,7 +396,7 @@ fn compute_deps_doc<'a, 'cfg>( .deps(unit.pkg.package_id()) .filter(|&(_id, deps)| { deps.iter().any(|dep| match dep.kind() { - DepKind::Normal => bcx.dep_platform_activated(dep, unit.kind), + DepKind::Normal => bcx.target_data.dep_platform_activated(dep, unit.kind), _ => false, }) }); diff --git a/src/cargo/ops/cargo_clean.rs b/src/cargo/ops/cargo_clean.rs index 88be9d4070e..0acaff53c70 100644 --- a/src/cargo/ops/cargo_clean.rs +++ b/src/cargo/ops/cargo_clean.rs @@ -4,8 +4,8 @@ use std::fs; use std::path::Path; use crate::core::compiler::unit_dependencies; -use crate::core::compiler::UnitInterner; use crate::core::compiler::{BuildConfig, BuildContext, CompileKind, CompileMode, Context}; +use crate::core::compiler::{RustcTargetData, UnitInterner}; use crate::core::profiles::{Profiles, UnitFor}; use crate::core::Workspace; use crate::ops; @@ -61,6 +61,7 @@ pub fn clean(ws: &Workspace<'_>, opts: &CleanOptions<'_>) -> CargoResult<()> { let interner = UnitInterner::new(); let mut build_config = BuildConfig::new(config, Some(1), &opts.target, CompileMode::Build)?; build_config.requested_profile = opts.requested_profile; + let target_data = RustcTargetData::new(ws, build_config.requested_kind)?; let bcx = BuildContext::new( ws, &packages, @@ -69,6 +70,7 @@ pub fn clean(ws: &Workspace<'_>, opts: &CleanOptions<'_>) -> CargoResult<()> { profiles, &interner, HashMap::new(), + target_data, )?; let mut units = Vec::new(); diff --git a/src/cargo/ops/cargo_compile.rs b/src/cargo/ops/cargo_compile.rs index 99b5c050177..fd3bc702fd0 100644 --- a/src/cargo/ops/cargo_compile.rs +++ b/src/cargo/ops/cargo_compile.rs @@ -31,7 +31,7 @@ use std::sync::Arc; use crate::core::compiler::standard_lib; use crate::core::compiler::unit_dependencies::build_unit_dependencies; use crate::core::compiler::{BuildConfig, BuildContext, Compilation, Context}; -use crate::core::compiler::{CompileKind, CompileMode, Unit}; +use crate::core::compiler::{CompileKind, CompileMode, RustcTargetData, Unit}; use crate::core::compiler::{DefaultExecutor, Executor, UnitInterner}; use crate::core::profiles::{Profiles, UnitFor}; use crate::core::resolver::{Resolve, ResolveOpts}; @@ -306,6 +306,7 @@ pub fn compile_ws<'a>( build_config.requested_profile, ws.features(), )?; + let target_data = RustcTargetData::new(ws, build_config.requested_kind)?; let specs = spec.to_package_id_specs(ws)?; let dev_deps = ws.require_optional_deps() || filter.need_dev_deps(build_config.mode); @@ -397,7 +398,9 @@ pub fn compile_ws<'a>( profiles, &interner, HashMap::new(), + target_data, )?; + let units = generate_targets( ws, &to_builds, From db80baad21ba0088bce4567a039044735d9c58eb Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Tue, 21 Jan 2020 17:34:54 -0800 Subject: [PATCH 02/22] Add a RequestedFeatures struct to hold cli features. The RequestedFeatures struct holds the features requested on the command line. --- src/cargo/core/compiler/standard_lib.rs | 2 +- src/cargo/core/resolver/context.rs | 9 ++--- src/cargo/core/resolver/dep_cache.rs | 6 ++-- src/cargo/core/resolver/features.rs | 46 ++++++++++++++++++++++++ src/cargo/core/resolver/mod.rs | 10 ++++-- src/cargo/core/resolver/types.rs | 43 ++++++++-------------- src/cargo/ops/cargo_compile.rs | 2 +- src/cargo/ops/cargo_doc.rs | 2 +- src/cargo/ops/cargo_generate_lockfile.rs | 6 ++-- src/cargo/ops/cargo_install.rs | 2 +- src/cargo/ops/cargo_output_metadata.rs | 2 +- src/cargo/ops/cargo_package.rs | 2 +- src/cargo/ops/resolve.rs | 32 ++++++++--------- 13 files changed, 99 insertions(+), 65 deletions(-) create mode 100644 src/cargo/core/resolver/features.rs diff --git a/src/cargo/core/compiler/standard_lib.rs b/src/cargo/core/compiler/standard_lib.rs index 112b2f7b9fa..73e9ed4b5c2 100644 --- a/src/cargo/core/compiler/standard_lib.rs +++ b/src/cargo/core/compiler/standard_lib.rs @@ -99,7 +99,7 @@ pub fn resolve_std<'cfg>( /*dev_deps*/ false, &features, /*all_features*/ false, /*uses_default_features*/ true, ); - let resolve = ops::resolve_ws_with_opts(&std_ws, opts, &specs)?; + let resolve = ops::resolve_ws_with_opts(&std_ws, &opts, &specs)?; Ok((resolve.pkg_set, resolve.targeted_resolve)) } diff --git a/src/cargo/core/resolver/context.rs b/src/cargo/core/resolver/context.rs index 9f21ebfe79f..1a44a812d32 100644 --- a/src/cargo/core/resolver/context.rs +++ b/src/cargo/core/resolver/context.rs @@ -164,20 +164,21 @@ impl Context { } } debug!("checking if {} is already activated", summary.package_id()); - if opts.all_features { + 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.is_subset(prev) - && (!opts.uses_default_features + opts.features.features.is_subset(prev) + && (!opts.features.uses_default_features || prev.contains("default") || !has_default_feature) } None => { - opts.features.is_empty() && (!opts.uses_default_features || !has_default_feature) + opts.features.features.is_empty() + && (!opts.features.uses_default_features || !has_default_feature) } }) } diff --git a/src/cargo/core/resolver/dep_cache.rs b/src/cargo/core/resolver/dep_cache.rs index 6365b1029e1..fec1112cde6 100644 --- a/src/cargo/core/resolver/dep_cache.rs +++ b/src/cargo/core/resolver/dep_cache.rs @@ -340,7 +340,7 @@ fn build_requirements<'a, 'b: 'a>( ) -> CargoResult> { let mut reqs = Requirements::new(s); - if opts.all_features { + if opts.features.all_features { for key in s.features().keys() { reqs.require_feature(*key)?; } @@ -348,12 +348,12 @@ fn build_requirements<'a, 'b: 'a>( reqs.require_dependency(dep.name_in_toml()); } } else { - for &f in opts.features.iter() { + for &f in opts.features.features.iter() { reqs.require_value(&FeatureValue::new(f, s))?; } } - if opts.uses_default_features && s.features().contains_key("default") { + if opts.features.uses_default_features && s.features().contains_key("default") { reqs.require_feature(InternedString::new("default"))?; } diff --git a/src/cargo/core/resolver/features.rs b/src/cargo/core/resolver/features.rs new file mode 100644 index 00000000000..3d982766e3f --- /dev/null +++ b/src/cargo/core/resolver/features.rs @@ -0,0 +1,46 @@ +use crate::core::resolver::types::FeaturesSet; +use crate::core::InternedString; +use std::collections::BTreeSet; +use std::rc::Rc; + +/// Features flags requested for a package. +#[derive(Debug, Clone, Eq, PartialEq, Hash)] +pub struct RequestedFeatures { + pub features: FeaturesSet, + pub all_features: bool, + pub uses_default_features: bool, +} + +impl RequestedFeatures { + /// Creates a new RequestedFeatures 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)), + all_features, + uses_default_features, + } + } + + /// Creates a new RequestedFeatures with the given `all_features` setting. + pub fn new_all(all_features: bool) -> RequestedFeatures { + RequestedFeatures { + features: Rc::new(BTreeSet::new()), + all_features, + uses_default_features: true, + } + } + + 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::>() + } +} diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index 17728120aab..76ed0ee1e33 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/src/cargo/core/resolver/mod.rs @@ -62,6 +62,7 @@ use crate::util::profile; use self::context::Context; use self::dep_cache::RegistryQueryer; +use self::features::RequestedFeatures; use self::types::{ConflictMap, ConflictReason, DepsFrame}; use self::types::{FeaturesSet, RcVecIter, RemainingDeps, ResolverProgress}; @@ -76,6 +77,7 @@ mod context; mod dep_cache; mod encode; mod errors; +pub mod features; mod resolve; mod types; @@ -368,9 +370,11 @@ fn activate_deps_loop( let pid = candidate.package_id(); let opts = ResolveOpts { dev_deps: false, - features: Rc::clone(&features), - all_features: false, - uses_default_features: dep.uses_default_features(), + features: RequestedFeatures { + features: Rc::clone(&features), + all_features: false, + uses_default_features: dep.uses_default_features(), + }, }; trace!( "{}[{}]>{} trying {}", diff --git a/src/cargo/core/resolver/types.rs b/src/cargo/core/resolver/types.rs index d18557a7a6d..53e325bb5c5 100644 --- a/src/cargo/core/resolver/types.rs +++ b/src/cargo/core/resolver/types.rs @@ -1,15 +1,14 @@ -use std::cmp::Ordering; -use std::collections::{BTreeMap, BTreeSet}; -use std::ops::Range; -use std::rc::Rc; -use std::time::{Duration, Instant}; - +use super::features::RequestedFeatures; use crate::core::interning::InternedString; use crate::core::{Dependency, PackageId, Summary}; use crate::util::errors::CargoResult; use crate::util::Config; - use im_rc; +use std::cmp::Ordering; +use std::collections::{BTreeMap, BTreeSet}; +use std::ops::Range; +use std::rc::Rc; +use std::time::{Duration, Instant}; pub struct ResolverProgress { ticks: u16, @@ -106,12 +105,8 @@ pub struct ResolveOpts { /// /// This may be set to `false` by things like `cargo install` or `-Z avoid-dev-deps`. pub dev_deps: bool, - /// Set of features to enable (`--features=…`). - pub features: FeaturesSet, - /// Indicates *all* features should be enabled (`--all-features`). - pub all_features: bool, - /// Include the `default` feature (`--no-default-features` sets this false). - pub uses_default_features: bool, + /// Set of features requested on the command-line. + pub features: RequestedFeatures, } impl ResolveOpts { @@ -119,9 +114,7 @@ impl ResolveOpts { pub fn everything() -> ResolveOpts { ResolveOpts { dev_deps: true, - features: Rc::new(BTreeSet::new()), - all_features: true, - uses_default_features: true, + features: RequestedFeatures::new_all(true), } } @@ -133,21 +126,13 @@ impl ResolveOpts { ) -> ResolveOpts { ResolveOpts { dev_deps, - features: Rc::new(ResolveOpts::split_features(features)), - all_features, - uses_default_features, + features: RequestedFeatures::from_command_line( + features, + all_features, + uses_default_features, + ), } } - - 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::>() - } } #[derive(Clone)] diff --git a/src/cargo/ops/cargo_compile.rs b/src/cargo/ops/cargo_compile.rs index fd3bc702fd0..1e3d8b7e748 100644 --- a/src/cargo/ops/cargo_compile.rs +++ b/src/cargo/ops/cargo_compile.rs @@ -311,7 +311,7 @@ pub fn compile_ws<'a>( 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, features, all_features, !no_default_features); - let resolve = ops::resolve_ws_with_opts(ws, opts, &specs)?; + let resolve = ops::resolve_ws_with_opts(ws, &opts, &specs)?; let WorkspaceResolve { mut pkg_set, workspace_resolve, diff --git a/src/cargo/ops/cargo_doc.rs b/src/cargo/ops/cargo_doc.rs index d95c41d6ee6..a3432aa7315 100644 --- a/src/cargo/ops/cargo_doc.rs +++ b/src/cargo/ops/cargo_doc.rs @@ -24,7 +24,7 @@ pub fn doc(ws: &Workspace<'_>, options: &DocOptions<'_>) -> CargoResult<()> { options.compile_opts.all_features, !options.compile_opts.no_default_features, ); - let ws_resolve = ops::resolve_ws_with_opts(ws, opts, &specs)?; + let ws_resolve = ops::resolve_ws_with_opts(ws, &opts, &specs)?; let ids = specs .iter() diff --git a/src/cargo/ops/cargo_generate_lockfile.rs b/src/cargo/ops/cargo_generate_lockfile.rs index ff5ed4ab467..9133d9622b3 100644 --- a/src/cargo/ops/cargo_generate_lockfile.rs +++ b/src/cargo/ops/cargo_generate_lockfile.rs @@ -24,7 +24,7 @@ pub fn generate_lockfile(ws: &Workspace<'_>) -> CargoResult<()> { let resolve = ops::resolve_with_previous( &mut registry, ws, - ResolveOpts::everything(), + &ResolveOpts::everything(), None, None, &[], @@ -64,7 +64,7 @@ pub fn update_lockfile(ws: &Workspace<'_>, opts: &UpdateOptions<'_>) -> CargoRes ops::resolve_with_previous( &mut registry, ws, - ResolveOpts::everything(), + &ResolveOpts::everything(), None, None, &[], @@ -110,7 +110,7 @@ pub fn update_lockfile(ws: &Workspace<'_>, opts: &UpdateOptions<'_>) -> CargoRes let resolve = ops::resolve_with_previous( &mut registry, ws, - ResolveOpts::everything(), + &ResolveOpts::everything(), Some(&previous_resolve), Some(&to_avoid), &[], diff --git a/src/cargo/ops/cargo_install.rs b/src/cargo/ops/cargo_install.rs index 0136486e0cd..1ffee70cf9c 100644 --- a/src/cargo/ops/cargo_install.rs +++ b/src/cargo/ops/cargo_install.rs @@ -495,7 +495,7 @@ fn check_yanked_install(ws: &Workspace<'_>) -> CargoResult<()> { // It would be best if `source` could be passed in here to avoid a // duplicate "Updating", but since `source` is taken by value, then it // wouldn't be available for `compile_ws`. - let ws_resolve = ops::resolve_ws_with_opts(ws, ResolveOpts::everything(), &specs)?; + let ws_resolve = ops::resolve_ws_with_opts(ws, &ResolveOpts::everything(), &specs)?; let mut sources = ws_resolve.pkg_set.sources_mut(); // Checking the yanked status involves taking a look at the registry and diff --git a/src/cargo/ops/cargo_output_metadata.rs b/src/cargo/ops/cargo_output_metadata.rs index 498c217936a..900ed22a081 100644 --- a/src/cargo/ops/cargo_output_metadata.rs +++ b/src/cargo/ops/cargo_output_metadata.rs @@ -122,7 +122,7 @@ fn build_resolve_graph( }; // Resolve entire workspace. let specs = Packages::All.to_package_id_specs(ws)?; - let ws_resolve = ops::resolve_ws_with_opts(ws, resolve_opts, &specs)?; + let ws_resolve = ops::resolve_ws_with_opts(ws, &resolve_opts, &specs)?; // Download all Packages. This is needed to serialize the information // for every package. In theory this could honor target filtering, // but that would be somewhat complex. diff --git a/src/cargo/ops/cargo_package.rs b/src/cargo/ops/cargo_package.rs index 5bfc27238e3..3906678062f 100644 --- a/src/cargo/ops/cargo_package.rs +++ b/src/cargo/ops/cargo_package.rs @@ -154,7 +154,7 @@ fn build_lock(ws: &Workspace<'_>) -> CargoResult { // Regenerate Cargo.lock using the old one as a guide. let specs = vec![PackageIdSpec::from_package_id(new_pkg.package_id())]; let tmp_ws = Workspace::ephemeral(new_pkg, ws.config(), None, true)?; - let new_resolve = ops::resolve_ws_with_opts(&tmp_ws, ResolveOpts::everything(), &specs)?; + let new_resolve = ops::resolve_ws_with_opts(&tmp_ws, &ResolveOpts::everything(), &specs)?; if let Some(orig_resolve) = orig_resolve { compare_resolve( diff --git a/src/cargo/ops/resolve.rs b/src/cargo/ops/resolve.rs index e09e5cac155..8d160e256c7 100644 --- a/src/cargo/ops/resolve.rs +++ b/src/cargo/ops/resolve.rs @@ -10,12 +10,8 @@ //! - `resolve_with_previous`: A low-level function for running the resolver, //! providing the most power and flexibility. -use std::collections::HashSet; -use std::rc::Rc; - -use log::{debug, trace}; - use crate::core::registry::PackageRegistry; +use crate::core::resolver::features::RequestedFeatures; use crate::core::resolver::{self, Resolve, ResolveOpts}; use crate::core::Feature; use crate::core::{PackageId, PackageIdSpec, PackageSet, Source, SourceId, Workspace}; @@ -23,6 +19,8 @@ use crate::ops; use crate::sources::PathSource; use crate::util::errors::{CargoResult, CargoResultExt}; use crate::util::profile; +use log::{debug, trace}; +use std::collections::HashSet; /// Result for `resolve_ws_with_opts`. pub struct WorkspaceResolve<'a> { @@ -72,12 +70,11 @@ pub fn resolve_ws<'a>(ws: &Workspace<'a>) -> CargoResult<(PackageSet<'a>, Resolv /// members. In this case, `opts.all_features` must be `true`. pub fn resolve_ws_with_opts<'a>( ws: &Workspace<'a>, - opts: ResolveOpts, + opts: &ResolveOpts, specs: &[PackageIdSpec], ) -> CargoResult> { let mut registry = PackageRegistry::new(ws.config())?; let mut add_patches = true; - let resolve = if ws.ignore_lock() { None } else if ws.require_optional_deps() { @@ -113,7 +110,7 @@ pub fn resolve_ws_with_opts<'a>( let resolved_with_overrides = resolve_with_previous( &mut registry, ws, - opts, + &opts, resolve.as_ref(), None, specs, @@ -137,7 +134,7 @@ fn resolve_with_registry<'cfg>( let resolve = resolve_with_previous( registry, ws, - ResolveOpts::everything(), + &ResolveOpts::everything(), prev.as_ref(), None, &[], @@ -168,14 +165,14 @@ fn resolve_with_registry<'cfg>( pub fn resolve_with_previous<'cfg>( registry: &mut PackageRegistry<'cfg>, ws: &Workspace<'cfg>, - opts: ResolveOpts, + opts: &ResolveOpts, previous: Option<&Resolve>, to_avoid: Option<&HashSet>, specs: &[PackageIdSpec], register_patches: bool, ) -> CargoResult { assert!( - !specs.is_empty() || opts.all_features, + !specs.is_empty() || opts.features.all_features, "no specs requires all_features" ); @@ -264,7 +261,7 @@ pub fn resolve_with_previous<'cfg>( if specs.is_empty() { members.extend(ws.members()); } else { - if specs.len() > 1 && !opts.features.is_empty() { + if specs.len() > 1 && !opts.features.features.is_empty() { anyhow::bail!("cannot specify features for more than one package"); } members.extend( @@ -275,7 +272,10 @@ pub fn resolve_with_previous<'cfg>( // of current workspace. Add all packages from workspace to get `foo` // into the resolution graph. if members.is_empty() { - if !(opts.features.is_empty() && !opts.all_features && opts.uses_default_features) { + if !(opts.features.features.is_empty() + && !opts.features.all_features + && opts.features.uses_default_features) + { anyhow::bail!("cannot specify features for packages outside of workspace"); } members.extend(ws.members()); @@ -316,12 +316,10 @@ pub fn resolve_with_previous<'cfg>( // "current" one, don't use the local `--features`. ResolveOpts { dev_deps: opts.dev_deps, - features: Rc::default(), - all_features: opts.all_features, - uses_default_features: true, + features: RequestedFeatures::new_all(opts.features.all_features), } } else { - // `-p` for non-member, skip. + // This member was not requested on the command-line, skip. continue; } } From 7caa1612cf39896d957afb11f07f3a10bb352b78 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Tue, 10 Sep 2019 15:38:01 -0700 Subject: [PATCH 03/22] Add new feature resolver. --- src/cargo/core/compiler/standard_lib.rs | 23 +- src/cargo/core/compiler/unit.rs | 6 +- src/cargo/core/compiler/unit_dependencies.rs | 120 +++-- src/cargo/core/dependency.rs | 17 + src/cargo/core/features.rs | 9 + src/cargo/core/profiles.rs | 29 +- src/cargo/core/resolver/encode.rs | 1 + src/cargo/core/resolver/features.rs | 507 ++++++++++++++++++- src/cargo/core/resolver/mod.rs | 29 +- src/cargo/core/resolver/resolve.rs | 45 +- src/cargo/ops/cargo_clean.rs | 31 +- src/cargo/ops/cargo_compile.rs | 65 ++- src/cargo/ops/cargo_doc.rs | 5 +- src/cargo/ops/cargo_install.rs | 15 +- src/cargo/ops/cargo_output_metadata.rs | 84 +-- src/cargo/ops/cargo_package.rs | 18 +- src/cargo/ops/resolve.rs | 17 +- src/doc/src/reference/unstable.md | 62 +++ tests/testsuite/features.rs | 50 ++ tests/testsuite/features2.rs | 396 +++++++++++++++ tests/testsuite/main.rs | 1 + 21 files changed, 1372 insertions(+), 158 deletions(-) create mode 100644 tests/testsuite/features2.rs diff --git a/src/cargo/core/compiler/standard_lib.rs b/src/cargo/core/compiler/standard_lib.rs index 73e9ed4b5c2..fbc2667d93a 100644 --- a/src/cargo/core/compiler/standard_lib.rs +++ b/src/cargo/core/compiler/standard_lib.rs @@ -1,7 +1,9 @@ //! Code for building the standard library. -use crate::core::compiler::{BuildContext, CompileKind, CompileMode, Unit}; +use crate::core::compiler::{BuildContext, CompileKind, CompileMode, RustcTargetData, Unit}; +use crate::core::dependency::DepKind; use crate::core::profiles::UnitFor; +use crate::core::resolver::features::ResolvedFeatures; use crate::core::resolver::ResolveOpts; use crate::core::{Dependency, PackageId, PackageSet, Resolve, SourceId, Workspace}; use crate::ops::{self, Packages}; @@ -31,8 +33,10 @@ pub fn parse_unstable_flag(value: Option<&str>) -> Vec { /// Resolve the standard library dependencies. pub fn resolve_std<'cfg>( ws: &Workspace<'cfg>, + target_data: &RustcTargetData, + requested_target: CompileKind, crates: &[String], -) -> CargoResult<(PackageSet<'cfg>, Resolve)> { +) -> CargoResult<(PackageSet<'cfg>, Resolve, ResolvedFeatures)> { let src_path = detect_sysroot_src_path(ws)?; let to_patch = [ "rustc-std-workspace-core", @@ -99,8 +103,12 @@ pub fn resolve_std<'cfg>( /*dev_deps*/ false, &features, /*all_features*/ false, /*uses_default_features*/ true, ); - let resolve = ops::resolve_ws_with_opts(&std_ws, &opts, &specs)?; - Ok((resolve.pkg_set, resolve.targeted_resolve)) + let resolve = ops::resolve_ws_with_opts(&std_ws, target_data, requested_target, &opts, &specs)?; + Ok(( + resolve.pkg_set, + resolve.targeted_resolve, + resolve.resolved_features, + )) } /// Generate a list of root `Unit`s for the standard library. @@ -110,6 +118,7 @@ pub fn generate_std_roots<'a>( bcx: &BuildContext<'a, '_>, crates: &[String], std_resolve: &'a Resolve, + std_features: &ResolvedFeatures, kind: CompileKind, ) -> CargoResult>> { // Generate the root Units for the standard library. @@ -139,7 +148,11 @@ pub fn generate_std_roots<'a>( unit_for, mode, ); - let features = std_resolve.features_sorted(pkg.package_id()); + let features = std_features.activated_features( + pkg.package_id(), + DepKind::Normal, + bcx.build_config.requested_kind, + ); Ok(bcx.units.intern( pkg, lib, profile, kind, mode, features, /*is_std*/ true, )) diff --git a/src/cargo/core/compiler/unit.rs b/src/cargo/core/compiler/unit.rs index 205d351e0a6..dbd66edf8a9 100644 --- a/src/cargo/core/compiler/unit.rs +++ b/src/cargo/core/compiler/unit.rs @@ -1,5 +1,5 @@ use crate::core::compiler::{CompileKind, CompileMode}; -use crate::core::{profiles::Profile, Package, Target}; +use crate::core::{profiles::Profile, InternedString, Package, Target}; use crate::util::hex::short_hash; use std::cell::RefCell; use std::collections::HashSet; @@ -50,7 +50,7 @@ pub struct UnitInner<'a> { pub mode: CompileMode, /// The `cfg` features to enable for this unit. /// This must be sorted. - pub features: Vec<&'a str>, + pub features: Vec, /// Whether this is a standard library unit. pub is_std: bool, } @@ -145,7 +145,7 @@ impl<'a> UnitInterner<'a> { profile: Profile, kind: CompileKind, mode: CompileMode, - features: Vec<&'a str>, + features: Vec, is_std: bool, ) -> Unit<'a> { let inner = self.intern_inner(&UnitInner { diff --git a/src/cargo/core/compiler/unit_dependencies.rs b/src/cargo/core/compiler/unit_dependencies.rs index a530683c091..ab9528d0868 100644 --- a/src/cargo/core/compiler/unit_dependencies.rs +++ b/src/cargo/core/compiler/unit_dependencies.rs @@ -20,6 +20,7 @@ use crate::core::compiler::{BuildContext, CompileKind, CompileMode}; use crate::core::dependency::DepKind; use crate::core::package::Downloads; use crate::core::profiles::{Profile, UnitFor}; +use crate::core::resolver::features::ResolvedFeatures; use crate::core::resolver::Resolve; use crate::core::{InternedString, Package, PackageId, Target}; use crate::CargoResult; @@ -53,7 +54,9 @@ struct State<'a, 'cfg> { unit_dependencies: UnitGraph<'a>, package_cache: HashMap, usr_resolve: &'a Resolve, + usr_features: &'a ResolvedFeatures, std_resolve: Option<&'a Resolve>, + std_features: Option<&'a ResolvedFeatures>, /// This flag is `true` while generating the dependencies for the standard /// library. is_std: bool, @@ -62,10 +65,15 @@ struct State<'a, 'cfg> { pub fn build_unit_dependencies<'a, 'cfg>( bcx: &'a BuildContext<'a, 'cfg>, resolve: &'a Resolve, - std_resolve: Option<&'a Resolve>, + features: &'a ResolvedFeatures, + std_resolve: Option<&'a (Resolve, ResolvedFeatures)>, roots: &[Unit<'a>], std_roots: &[Unit<'a>], ) -> CargoResult> { + let (std_resolve, std_features) = match std_resolve { + Some((r, f)) => (Some(r), Some(f)), + None => (None, None), + }; let mut state = State { bcx, downloads: bcx.packages.enable_download()?, @@ -73,7 +81,9 @@ pub fn build_unit_dependencies<'a, 'cfg>( unit_dependencies: HashMap::new(), package_cache: HashMap::new(), usr_resolve: resolve, + usr_features: features, std_resolve, + std_features, is_std: false, }; @@ -220,7 +230,7 @@ fn compute_deps<'a, 'cfg>( unit_for: UnitFor, ) -> CargoResult>> { if unit.mode.is_run_custom_build() { - return compute_deps_custom_build(unit, state); + return compute_deps_custom_build(unit, unit_for.dep_kind(), state); } else if unit.mode.is_doc() { // Note: this does not include doc test. return compute_deps_doc(unit, state); @@ -228,40 +238,45 @@ fn compute_deps<'a, 'cfg>( let bcx = state.bcx; let id = unit.pkg.package_id(); - let deps = state.resolve().deps(id).filter(|&(_id, deps)| { - assert!(!deps.is_empty()); - deps.iter().any(|dep| { - // If this target is a build command, then we only want build - // dependencies, otherwise we want everything *other than* build - // dependencies. - if unit.target.is_custom_build() != dep.is_build() { - return false; - } + let filtered_deps = state + .resolve() + .deps(id) + .map(|(id, deps)| { + assert!(!deps.is_empty()); + let filtered_deps = deps.iter().filter(|dep| { + // If this target is a build command, then we only want build + // dependencies, otherwise we want everything *other than* build + // dependencies. + if unit.target.is_custom_build() != dep.is_build() { + return false; + } - // If this dependency is **not** a transitive dependency, then it - // only applies to test/example targets. - if !dep.is_transitive() - && !unit.target.is_test() - && !unit.target.is_example() - && !unit.mode.is_any_test() - { - return false; - } + // If this dependency is **not** a transitive dependency, then it + // only applies to test/example targets. + if !dep.is_transitive() + && !unit.target.is_test() + && !unit.target.is_example() + && !unit.mode.is_any_test() + { + return false; + } - // If this dependency is only available for certain platforms, - // make sure we're only enabling it for that platform. - if !bcx.target_data.dep_platform_activated(dep, unit.kind) { - return false; - } + // If this dependency is only available for certain platforms, + // make sure we're only enabling it for that platform. + if !bcx.target_data.dep_platform_activated(dep, unit.kind) { + return false; + } - // If we've gotten past all that, then this dependency is - // actually used! - true + // If we've gotten past all that, then this dependency is + // actually used! + true + }); + (id, filtered_deps.collect::>()) }) - }); + .filter(|(_id, deps)| !deps.is_empty()); let mut ret = Vec::new(); - for (id, _) in deps { + for (id, deps) in filtered_deps { let pkg = match state.get(id)? { Some(pkg) => pkg, None => continue, @@ -271,7 +286,16 @@ fn compute_deps<'a, 'cfg>( None => continue, }; let mode = check_or_build_mode(unit.mode, lib); - let dep_unit_for = unit_for.with_for_host(lib.for_host()); + let dep_kind = if unit.target.is_custom_build() { + DepKind::Build + } else if deps.iter().any(|dep| !dep.is_transitive()) { + DepKind::Development + } else { + DepKind::Normal + }; + let dep_unit_for = unit_for + .with_for_host(lib.for_host()) + .with_dep_kind(dep_kind); if bcx.config.cli_unstable().dual_proc_macros && lib.proc_macro() && !unit.kind.is_host() { let unit_dep = new_unit_dep(state, unit, pkg, lib, dep_unit_for, unit.kind, mode)?; @@ -299,7 +323,7 @@ fn compute_deps<'a, 'cfg>( if unit.target.is_custom_build() { return Ok(ret); } - ret.extend(dep_build_script(unit, state)?); + ret.extend(dep_build_script(unit, unit_for.dep_kind(), state)?); // If this target is a binary, test, example, etc, then it depends on // the library of the same package. The call to `resolve.deps` above @@ -326,7 +350,7 @@ fn compute_deps<'a, 'cfg>( t.is_bin() && // Skip binaries with required features that have not been selected. t.required_features().unwrap_or(&no_required_features).iter().all(|f| { - unit.features.contains(&f.as_str()) + unit.features.contains(&InternedString::new(f.as_str())) }) }) .map(|t| { @@ -353,6 +377,7 @@ fn compute_deps<'a, 'cfg>( /// the returned set of units must all be run before `unit` is run. fn compute_deps_custom_build<'a, 'cfg>( unit: &Unit<'a>, + dep_kind: DepKind, state: &mut State<'a, 'cfg>, ) -> CargoResult>> { if let Some(links) = unit.pkg.manifest().links() { @@ -361,6 +386,9 @@ fn compute_deps_custom_build<'a, 'cfg>( return Ok(Vec::new()); } } + // All dependencies of this unit should use profiles for custom + // builds. + let unit_for = UnitFor::new_build().with_dep_kind(dep_kind); // When not overridden, then the dependencies to run a build script are: // // 1. Compiling the build script itself. @@ -375,9 +403,7 @@ fn compute_deps_custom_build<'a, 'cfg>( unit, unit.pkg, unit.target, - // All dependencies of this unit should use profiles for custom - // builds. - UnitFor::new_build(), + unit_for, // Build scripts always compiled for the host. CompileKind::Host, CompileMode::Build, @@ -444,7 +470,7 @@ fn compute_deps_doc<'a, 'cfg>( } // Be sure to build/run the build script for documented libraries. - ret.extend(dep_build_script(unit, state)?); + ret.extend(dep_build_script(unit, DepKind::Normal, state)?); // If we document a binary/example, we need the library available. if unit.target.is_bin() || unit.target.is_example() { @@ -486,6 +512,7 @@ fn maybe_lib<'a>( /// build script. fn dep_build_script<'a>( unit: &Unit<'a>, + dep_kind: DepKind, state: &State<'a, '_>, ) -> CargoResult>> { unit.pkg @@ -499,12 +526,13 @@ fn dep_build_script<'a>( .bcx .profiles .get_profile_run_custom_build(&unit.profile); + let script_unit_for = UnitFor::new_build().with_dep_kind(dep_kind); new_unit_dep_with_profile( state, unit, unit.pkg, t, - UnitFor::new_build(), + script_unit_for, unit.kind, CompileMode::RunCustomBuild, profile, @@ -569,7 +597,7 @@ fn new_unit_dep_with_profile<'a>( let public = state .resolve() .is_public_dep(parent.pkg.package_id(), pkg.package_id()); - let features = state.resolve().features_sorted(pkg.package_id()); + let features = state.activated_features(pkg.package_id(), unit_for.dep_kind(), kind); let unit = state .bcx .units @@ -674,6 +702,20 @@ impl<'a, 'cfg> State<'a, 'cfg> { } } + fn activated_features( + &self, + pkg_id: PackageId, + dep_kind: DepKind, + compile_kind: CompileKind, + ) -> Vec { + let features = if self.is_std { + self.std_features.unwrap() + } else { + self.usr_features + }; + features.activated_features(pkg_id, dep_kind, compile_kind) + } + fn get(&mut self, id: PackageId) -> CargoResult> { if let Some(pkg) = self.package_cache.get(&id) { return Ok(Some(pkg)); diff --git a/src/cargo/core/dependency.rs b/src/cargo/core/dependency.rs index 82da0612925..5eff3cd505b 100644 --- a/src/cargo/core/dependency.rs +++ b/src/cargo/core/dependency.rs @@ -92,6 +92,23 @@ pub enum DepKind { Build, } +impl DepKind { + /// Convert a DepKind from a package to one of its dependencies. + /// + /// The rules here determine how feature decoupling works. This works in + /// conjunction with the new feature resolver. + pub fn sticky_kind(&self, to: DepKind) -> DepKind { + use DepKind::*; + match (self, to) { + (Normal, _) => to, + (Build, _) => Build, + (Development, Normal) => Development, + (Development, Build) => Build, + (Development, Development) => Development, + } + } +} + fn parse_req_with_deprecated( name: InternedString, req: &str, diff --git a/src/cargo/core/features.rs b/src/cargo/core/features.rs index 8f66720d894..f08aa0d33f8 100644 --- a/src/cargo/core/features.rs +++ b/src/cargo/core/features.rs @@ -341,6 +341,7 @@ pub struct CliUnstable { pub doctest_xcompile: bool, pub panic_abort_tests: bool, pub jobserver_per_rustc: bool, + pub features: Option>, } impl CliUnstable { @@ -380,6 +381,13 @@ impl CliUnstable { } } + fn parse_features(value: Option<&str>) -> Vec { + match value { + None => Vec::new(), + Some(v) => v.split(',').map(|s| s.to_string()).collect(), + } + } + // Asserts that there is no argument to the flag. fn parse_empty(key: &str, value: Option<&str>) -> CargoResult { if let Some(v) = value { @@ -409,6 +417,7 @@ impl CliUnstable { "doctest-xcompile" => self.doctest_xcompile = parse_empty(k, v)?, "panic-abort-tests" => self.panic_abort_tests = parse_empty(k, v)?, "jobserver-per-rustc" => self.jobserver_per_rustc = parse_empty(k, v)?, + "features" => self.features = Some(parse_features(v)), _ => bail!("unknown `-Z` flag specified: {}", k), } diff --git a/src/cargo/core/profiles.rs b/src/cargo/core/profiles.rs index 7ed47152af5..9bdfafeb83f 100644 --- a/src/cargo/core/profiles.rs +++ b/src/cargo/core/profiles.rs @@ -1,4 +1,5 @@ use crate::core::compiler::CompileMode; +use crate::core::dependency::DepKind; use crate::core::interning::InternedString; use crate::core::{Feature, Features, PackageId, PackageIdSpec, Resolve, Shell}; use crate::util::errors::CargoResultExt; @@ -767,6 +768,8 @@ pub struct UnitFor { /// any of its dependencies. This enables `build-override` profiles for /// these targets. build: bool, + /// The dependency kind (normal, dev, build). + dep_kind: DepKind, /// How Cargo processes the `panic` setting or profiles. This is done to /// handle test/benches inheriting from dev/release, as well as forcing /// `for_host` units to always unwind. @@ -794,6 +797,7 @@ impl UnitFor { pub fn new_normal() -> UnitFor { UnitFor { build: false, + dep_kind: DepKind::Normal, panic_setting: PanicSetting::ReadProfile, } } @@ -802,6 +806,7 @@ impl UnitFor { pub fn new_build() -> UnitFor { UnitFor { build: true, + dep_kind: DepKind::Normal, // Force build scripts to always use `panic=unwind` for now to // maximally share dependencies with procedural macros. panic_setting: PanicSetting::AlwaysUnwind, @@ -812,6 +817,7 @@ impl UnitFor { pub fn new_compiler() -> UnitFor { UnitFor { build: false, + dep_kind: DepKind::Normal, // Force plugins to use `panic=abort` so panics in the compiler do // not abort the process but instead end with a reasonable error // message that involves catching the panic in the compiler. @@ -819,7 +825,7 @@ impl UnitFor { } } - /// A unit for a test/bench target or their dependencies. + /// A unit for a test/bench target. /// /// Note that `config` is taken here for unstable CLI features to detect /// whether `panic=abort` is supported for tests. Historical versions of @@ -828,6 +834,7 @@ impl UnitFor { pub fn new_test(config: &Config) -> UnitFor { UnitFor { build: false, + dep_kind: DepKind::Development, // We're testing out an unstable feature (`-Zpanic-abort-tests`) // which inherits the panic setting from the dev/release profile // (basically avoid recompiles) but historical defaults required @@ -850,6 +857,7 @@ impl UnitFor { pub fn with_for_host(self, for_host: bool) -> UnitFor { UnitFor { build: self.build || for_host, + dep_kind: self.dep_kind, panic_setting: if for_host { PanicSetting::AlwaysUnwind } else { @@ -858,6 +866,15 @@ impl UnitFor { } } + /// Creates a variant for the given dependency kind. + /// + /// This is part of the machinery responsible for handling feature + /// decoupling in the new feature resolver. + pub fn with_dep_kind(mut self, kind: DepKind) -> UnitFor { + self.dep_kind = self.dep_kind.sticky_kind(kind); + self + } + /// Returns `true` if this unit is for a custom build script or one of its /// dependencies. pub fn is_build(self) -> bool { @@ -869,23 +886,33 @@ impl UnitFor { self.panic_setting } + /// Returns the dependency kind this unit is intended for. + pub fn dep_kind(&self) -> DepKind { + self.dep_kind + } + /// All possible values, used by `clean`. pub fn all_values() -> &'static [UnitFor] { + // dep_kind isn't needed for profiles, so its value doesn't matter. static ALL: &[UnitFor] = &[ UnitFor { build: false, + dep_kind: DepKind::Normal, panic_setting: PanicSetting::ReadProfile, }, UnitFor { build: true, + dep_kind: DepKind::Normal, panic_setting: PanicSetting::AlwaysUnwind, }, UnitFor { build: false, + dep_kind: DepKind::Normal, panic_setting: PanicSetting::AlwaysUnwind, }, UnitFor { build: false, + dep_kind: DepKind::Normal, panic_setting: PanicSetting::Inherit, }, ]; diff --git a/src/cargo/core/resolver/encode.rs b/src/cargo/core/resolver/encode.rs index 6498b57dbe3..5fb4631952f 100644 --- a/src/cargo/core/resolver/encode.rs +++ b/src/cargo/core/resolver/encode.rs @@ -365,6 +365,7 @@ impl EncodableResolve { metadata, unused_patches, version, + HashMap::new(), )) } } diff --git a/src/cargo/core/resolver/features.rs b/src/cargo/core/resolver/features.rs index 3d982766e3f..64c7b470251 100644 --- a/src/cargo/core/resolver/features.rs +++ b/src/cargo/core/resolver/features.rs @@ -1,8 +1,93 @@ +//! Feature resolver. +//! +//! This is a new feature resolver that runs independently of the main +//! dependency resolver. It is intended to make it easier to experiment with +//! new behaviors. When `-Zfeatures` is not used, it will fall back to using +//! the original `Resolve` feature computation. With `-Zfeatures` enabled, +//! this will walk the dependency graph and compute the features using a +//! different algorithm. One of its key characteristics is that it can avoid +//! unifying features for shared dependencies in some situations. +//! +//! The preferred way to engage this new resolver is via +//! `resolve_ws_with_opts`. +//! +//! There are many assumptions made about the resolver itself. It assumes +//! validation has already been done on the feature maps, and doesn't do any +//! validation itself. It assumes dev-dependencies within a dependency have +//! been removed. + +use crate::core::compiler::{CompileKind, RustcTargetData}; +use crate::core::dependency::{DepKind, Dependency}; use crate::core::resolver::types::FeaturesSet; -use crate::core::InternedString; -use std::collections::BTreeSet; +use crate::core::resolver::Resolve; +use crate::core::{FeatureValue, InternedString, PackageId, PackageIdSpec, Workspace}; +use crate::util::{CargoResult, Config}; +use std::collections::{BTreeSet, HashMap, HashSet}; use std::rc::Rc; +type ActivateMap = HashMap<(PackageId, DepKind, CompileKind), BTreeSet>; + +/// Set of all activated features for all packages in the resolve graph. +pub struct ResolvedFeatures { + activated_features: ActivateMap, + /// This is only here for legacy support when `-Zfeatures` is not enabled. + legacy: Option>>, + opts: FeatureOpts, +} + +/// Options for how the feature resolver works. +#[derive(Default)] +struct FeatureOpts { + /// -Zpackage-features, changes behavior of feature flags in a workspace. + package_features: bool, + /// -Zfeatures is enabled, use new resolver. + new_resolver: bool, + /// Build deps will not share share features with other dep kinds. + decouple_build_deps: bool, + /// Dev dep features will not be activated unless needed. + decouple_dev_deps: bool, + /// Targets that are not in use will not activate features. + ignore_inactive_targets: bool, + /// If enabled, compare against old resolver (for testing). + compare: bool, +} + +impl FeatureOpts { + fn new(config: &Config) -> CargoResult { + let mut opts = FeatureOpts::default(); + let unstable_flags = config.cli_unstable(); + opts.package_features = unstable_flags.package_features; + let mut enable = |feat_opts: &Vec| { + opts.new_resolver = true; + for opt in feat_opts { + match opt.as_ref() { + "build_dep" => opts.decouple_build_deps = true, + "dev_dep" => opts.decouple_dev_deps = true, + "itarget" => opts.ignore_inactive_targets = true, + "compare" => opts.compare = true, + "ws" => unimplemented!(), + "host" => unimplemented!(), + s => anyhow::bail!("-Zfeatures flag `{}` is not supported", s), + } + } + Ok(()) + }; + if let Some(feat_opts) = unstable_flags.features.as_ref() { + enable(feat_opts)?; + } + // This env var is intended for testing only. + if let Ok(env_opts) = std::env::var("__CARGO_FORCE_NEW_FEATURES") { + if env_opts == "1" { + opts.new_resolver = true; + } else { + let env_opts = env_opts.split(',').map(|s| s.to_string()).collect(); + enable(&env_opts)?; + } + } + Ok(opts) + } +} + /// Features flags requested for a package. #[derive(Debug, Clone, Eq, PartialEq, Hash)] pub struct RequestedFeatures { @@ -44,3 +129,421 @@ impl RequestedFeatures { .collect::>() } } + +impl ResolvedFeatures { + /// Returns the list of features that are enabled for the given package. + pub fn activated_features( + &self, + pkg_id: PackageId, + dep_kind: DepKind, + compile_kind: CompileKind, + ) -> Vec { + if let Some(legacy) = &self.legacy { + legacy.get(&pkg_id).map_or_else(Vec::new, |v| v.clone()) + } else { + // TODO: Remove panic, return empty set. + let dep_kind = if (!self.opts.decouple_build_deps && dep_kind == DepKind::Build) + || (!self.opts.decouple_dev_deps && dep_kind == DepKind::Development) + { + // Decoupling disabled, everything is unified under "Normal". + DepKind::Normal + } else { + dep_kind + }; + let fs = self + .activated_features + .get(&(pkg_id, dep_kind, compile_kind)) + .unwrap_or_else(|| panic!("features did not find {:?} {:?}", pkg_id, dep_kind)); + fs.iter().cloned().collect() + } + } +} + +pub struct FeatureResolver<'a, 'cfg> { + ws: &'a Workspace<'cfg>, + target_data: &'a RustcTargetData, + /// The platform to build for, requested by the user. + requested_target: CompileKind, + resolve: &'a Resolve, + /// Features requested by the user on the command-line. + requested_features: &'a RequestedFeatures, + /// Packages to build, requested on the command-line. + specs: &'a [PackageIdSpec], + /// Options that change how the feature resolver operates. + opts: FeatureOpts, + /// Map of features activated for each package. + activated_features: ActivateMap, + /// Keeps track of which packages have had its dependencies processed. + /// Used to avoid cycles, and to speed up processing. + processed_deps: HashSet<(PackageId, DepKind, CompileKind)>, +} + +impl<'a, 'cfg> FeatureResolver<'a, 'cfg> { + /// Runs the resolution algorithm and returns a new `ResolvedFeatures` + /// with the result. + pub fn resolve( + ws: &Workspace<'cfg>, + target_data: &RustcTargetData, + resolve: &Resolve, + requested_features: &RequestedFeatures, + specs: &[PackageIdSpec], + requested_target: CompileKind, + ) -> CargoResult { + use crate::util::profile; + let _p = profile::start("resolve features"); + + let opts = FeatureOpts::new(ws.config())?; + if !opts.new_resolver { + // Legacy mode. + return Ok(ResolvedFeatures { + activated_features: HashMap::new(), + legacy: Some(resolve.features_clone()), + opts, + }); + } + let mut r = FeatureResolver { + ws, + target_data, + requested_target, + resolve, + requested_features, + specs, + opts, + activated_features: HashMap::new(), + processed_deps: HashSet::new(), + }; + r.do_resolve()?; + log::debug!("features={:#?}", r.activated_features); + if r.opts.compare { + r.compare(); + } + Ok(ResolvedFeatures { + activated_features: r.activated_features, + legacy: None, + opts: r.opts, + }) + } + + /// Performs the process of resolving all features for the resolve graph. + fn do_resolve(&mut self) -> CargoResult<()> { + if self.opts.package_features { + let mut found = false; + for member in self.ws.members() { + let member_id = member.package_id(); + if self.specs.iter().any(|spec| spec.matches(member_id)) { + found = true; + self.activate_member(member_id, self.requested_features)?; + } + } + if !found { + // -p for a non-member. Just resolve all with defaults. + let default = RequestedFeatures::new_all(false); + for member in self.ws.members() { + self.activate_member(member.package_id(), &default)?; + } + } + } else { + for member in self.ws.members() { + let member_id = member.package_id(); + match self.ws.current_opt() { + Some(current) if member_id == current.package_id() => { + // The "current" member gets activated with the flags + // from the command line. + self.activate_member(member_id, self.requested_features)?; + } + _ => { + // Ignore members that are not enabled on the command-line. + if self.specs.iter().any(|spec| spec.matches(member_id)) { + // -p for a workspace member that is not the + // "current" one, don't use the local `--features`. + let not_current_requested = + RequestedFeatures::new_all(self.requested_features.all_features); + self.activate_member(member_id, ¬_current_requested)?; + } + } + } + } + } + Ok(()) + } + + /// Enable the given features on the given workspace member. + fn activate_member( + &mut self, + pkg_id: PackageId, + requested_features: &RequestedFeatures, + ) -> CargoResult<()> { + let fvs = self.fvs_from_requested(pkg_id, CompileKind::Host, requested_features); + self.activate_member_fvs(pkg_id, CompileKind::Host, &fvs)?; + if let CompileKind::Target(_) = self.requested_target { + let fvs = self.fvs_from_requested(pkg_id, self.requested_target, requested_features); + self.activate_member_fvs(pkg_id, self.requested_target, &fvs)?; + } + Ok(()) + } + + fn activate_member_fvs( + &mut self, + pkg_id: PackageId, + compile_kind: CompileKind, + fvs: &[FeatureValue], + ) -> CargoResult<()> { + self.activate_with_platform(pkg_id, DepKind::Normal, compile_kind, &fvs)?; + if self.opts.decouple_dev_deps { + // Activate the member as a dev dep, assuming it has at least one + // test, bench, or example. This ensures the member's normal deps get + // unified with its dev deps. + self.activate_with_platform(pkg_id, DepKind::Development, compile_kind, &fvs)?; + } + Ok(()) + } + + fn activate_with_platform( + &mut self, + pkg_id: PackageId, + dep_kind: DepKind, + compile_kind: CompileKind, + fvs: &[FeatureValue], + ) -> CargoResult<()> { + // Add an empty entry to ensure everything is covered. This is intended for + // finding bugs where the resolver missed something it should have visited. + // Remove this in the future if `activated_features` uses an empty default. + self.activated_features + .entry((pkg_id, dep_kind, compile_kind)) + .or_insert_with(BTreeSet::new); + for fv in fvs { + self.activate_fv(pkg_id, dep_kind, compile_kind, fv)?; + } + if !self.processed_deps.insert((pkg_id, dep_kind, compile_kind)) { + // Already processed dependencies. + return Ok(()); + } + // Activate any of its dependencies. + for (dep_pkg_id, deps) in self.deps(pkg_id, compile_kind) { + for dep in deps { + if dep.is_optional() { + continue; + } + // Recurse into the dependency. + let fvs = self.fvs_from_dependency(dep_pkg_id, dep); + self.activate_with_platform( + dep_pkg_id, + self.sticky_dep_kind(dep_kind, dep.kind()), + compile_kind, + &fvs, + )?; + } + } + return Ok(()); + } + + fn activate_fv( + &mut self, + pkg_id: PackageId, + dep_kind: DepKind, + compile_kind: CompileKind, + fv: &FeatureValue, + ) -> CargoResult<()> { + match fv { + FeatureValue::Feature(f) => { + self.activate_rec(pkg_id, dep_kind, compile_kind, *f)?; + } + FeatureValue::Crate(dep_name) => { + // Activate the feature name on self. + self.activate_rec(pkg_id, dep_kind, compile_kind, *dep_name)?; + // Activate the optional dep. + for (dep_pkg_id, deps) in self.deps(pkg_id, compile_kind) { + for dep in deps { + if dep.name_in_toml() == *dep_name { + let fvs = self.fvs_from_dependency(dep_pkg_id, dep); + self.activate_with_platform( + dep_pkg_id, + self.sticky_dep_kind(dep_kind, dep.kind()), + compile_kind, + &fvs, + )?; + } + } + } + } + FeatureValue::CrateFeature(dep_name, dep_feature) => { + // Activate a feature within a dependency. + for (dep_pkg_id, deps) in self.deps(pkg_id, compile_kind) { + for dep in deps { + if dep.name_in_toml() == *dep_name { + if dep.is_optional() { + // Activate the crate on self. + let fv = FeatureValue::Crate(*dep_name); + self.activate_fv(pkg_id, dep_kind, compile_kind, &fv)?; + } + // Activate the feature on the dependency. + let summary = self.resolve.summary(dep_pkg_id); + let fv = FeatureValue::new(*dep_feature, summary); + self.activate_fv( + dep_pkg_id, + self.sticky_dep_kind(dep_kind, dep.kind()), + compile_kind, + &fv, + )?; + } + } + } + } + } + Ok(()) + } + + /// Activate the given feature for the given package, and then recursively + /// activate any other features that feature enables. + fn activate_rec( + &mut self, + pkg_id: PackageId, + dep_kind: DepKind, + compile_kind: CompileKind, + feature_to_enable: InternedString, + ) -> CargoResult<()> { + let enabled = self + .activated_features + .entry((pkg_id, dep_kind, compile_kind)) + .or_insert_with(BTreeSet::new); + if !enabled.insert(feature_to_enable) { + // Already enabled. + return Ok(()); + } + let summary = self.resolve.summary(pkg_id); + let feature_map = summary.features(); + let fvs = match feature_map.get(&feature_to_enable) { + Some(fvs) => fvs, + None => { + // TODO: this should only happen for optional dependencies. + // Other cases should be validated by Summary's `build_feature_map`. + // Figure out some way to validate this assumption. + log::debug!( + "pkg {:?} does not define feature {}", + pkg_id, + feature_to_enable + ); + return Ok(()); + } + }; + for fv in fvs { + self.activate_fv(pkg_id, dep_kind, compile_kind, fv)?; + } + Ok(()) + } + + /// Returns Vec of FeatureValues from a Dependency definition. + fn fvs_from_dependency(&self, dep_id: PackageId, dep: &Dependency) -> Vec { + let summary = self.resolve.summary(dep_id); + let feature_map = summary.features(); + let mut result: Vec = dep + .features() + .iter() + .map(|f| FeatureValue::new(*f, summary)) + .collect(); + let default = InternedString::new("default"); + if dep.uses_default_features() && feature_map.contains_key(&default) { + result.push(FeatureValue::Feature(default)); + } + result + } + + /// Returns Vec of FeatureValues from a set of command-line features. + fn fvs_from_requested( + &self, + pkg_id: PackageId, + compile_kind: CompileKind, + requested_features: &RequestedFeatures, + ) -> Vec { + let summary = self.resolve.summary(pkg_id); + let feature_map = summary.features(); + if requested_features.all_features { + let mut fvs: Vec = feature_map + .keys() + .map(|k| FeatureValue::Feature(*k)) + .collect(); + // Add optional deps. + for (_dep_pkg_id, deps) in self.deps(pkg_id, compile_kind) { + for dep in deps { + if dep.is_optional() { + // This may result in duplicates, but that should be ok. + fvs.push(FeatureValue::Crate(dep.name_in_toml())); + } + } + } + fvs + } else { + let mut result: Vec = requested_features + .features + .as_ref() + .iter() + .map(|f| FeatureValue::new(*f, summary)) + .collect(); + let default = InternedString::new("default"); + if requested_features.uses_default_features && feature_map.contains_key(&default) { + result.push(FeatureValue::Feature(default)); + } + result + } + } + + /// Returns the dependencies for a package, filtering out inactive targets. + fn deps( + &self, + pkg_id: PackageId, + compile_kind: CompileKind, + ) -> Vec<(PackageId, Vec<&'a Dependency>)> { + self.resolve + .deps(pkg_id) + .map(|(dep_id, deps)| { + let deps = deps + .iter() + .filter(|dep| { + !dep.platform().is_some() + || !self.opts.ignore_inactive_targets + || self.target_data.dep_platform_activated(dep, compile_kind) + }) + .collect::>(); + (dep_id, deps) + }) + .collect() + } + + /// Convert a DepKind from a package to one of its dependencies. + /// + /// The rules here determine how decoupling works. + fn sticky_dep_kind(&self, from: DepKind, to: DepKind) -> DepKind { + if self.opts.decouple_build_deps { + if from == DepKind::Build || to == DepKind::Build { + return DepKind::Build; + } + } + if self.opts.decouple_dev_deps { + if to == DepKind::Development { + return DepKind::Development; + } + if from == DepKind::Development && to != DepKind::Build { + return DepKind::Development; + } + } + return DepKind::Normal; + } + + /// Compare the activated features to the resolver. Used for testing. + fn compare(&self) { + let mut found = false; + for ((pkg_id, dep_kind, compile_kind), features) in &self.activated_features { + let r_features = self.resolve.features(*pkg_id); + if !r_features.iter().eq(features.iter()) { + eprintln!( + "{}/{:?}/{:?} features mismatch\nresolve: {:?}\nnew: {:?}\n", + pkg_id, dep_kind, compile_kind, r_features, features + ); + found = true; + } + } + if found { + panic!("feature mismatch"); + } + } +} diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index 76ed0ee1e33..0cb197e0fdc 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/src/cargo/core/resolver/mod.rs @@ -108,9 +108,6 @@ mod types; /// * `config` - a location to print warnings and such, or `None` if no warnings /// should be printed /// -/// * `print_warnings` - whether or not to print backwards-compatibility -/// warnings and such -/// /// * `check_public_visible_dependencies` - a flag for whether to enforce the restrictions /// introduced in the "public & private dependencies" RFC (1977). The current implementation /// makes sure that there is only one version of each name visible to each package. @@ -143,17 +140,27 @@ pub fn resolve( let cksum = summary.checksum().map(|s| s.to_string()); cksums.insert(summary.package_id(), cksum); } + let graph = cx.graph(); + let replacements = cx.resolve_replacements(®istry); + let features = cx + .resolve_features + .iter() + .map(|(k, v)| (*k, v.iter().cloned().collect())) + .collect(); + let summaries = cx + .activations + .into_iter() + .map(|(_key, (summary, _age))| (summary.package_id(), summary)) + .collect(); let resolve = Resolve::new( - cx.graph(), - cx.resolve_replacements(®istry), - cx.resolve_features - .iter() - .map(|(k, v)| (*k, v.iter().map(|x| x.to_string()).collect())) - .collect(), + graph, + replacements, + features, cksums, BTreeMap::new(), Vec::new(), ResolveVersion::default_for_new_lockfiles(), + summaries, ); check_cycles(&resolve)?; @@ -163,11 +170,11 @@ pub fn resolve( Ok(resolve) } -/// Recursively activates the dependencies for `top`, in depth-first order, +/// Recursively activates the dependencies for `summaries`, in depth-first order, /// backtracking across possible candidates for each dependency as necessary. /// /// If all dependencies can be activated and resolved to a version in the -/// dependency graph, cx.resolve is returned. +/// dependency graph, `cx` is returned. fn activate_deps_loop( mut cx: Context, registry: &mut RegistryQueryer<'_>, diff --git a/src/cargo/core/resolver/resolve.rs b/src/cargo/core/resolver/resolve.rs index 67fb8341a00..cd4afbfb380 100644 --- a/src/cargo/core/resolver/resolve.rs +++ b/src/cargo/core/resolver/resolve.rs @@ -1,15 +1,13 @@ -use std::borrow::Borrow; -use std::cmp; -use std::collections::{HashMap, HashSet}; -use std::fmt; -use std::iter::FromIterator; - +use super::encode::Metadata; use crate::core::dependency::DepKind; +use crate::core::interning::InternedString; use crate::core::{Dependency, PackageId, PackageIdSpec, Summary, Target}; use crate::util::errors::CargoResult; use crate::util::Graph; - -use super::encode::Metadata; +use std::borrow::Borrow; +use std::cmp; +use std::collections::{HashMap, HashSet}; +use std::fmt; /// Represents a fully-resolved package dependency graph. Each node in the graph /// is a package and edges represent dependencies between packages. @@ -28,9 +26,9 @@ pub struct Resolve { /// An empty `HashSet` to avoid creating a new `HashSet` for every package /// that does not have any features, and to avoid using `Option` to /// simplify the API. - empty_features: HashSet, + empty_features: Vec, /// Features enabled for a given package. - features: HashMap>, + features: HashMap>, /// Checksum for each package. A SHA256 hash of the `.crate` file used to /// validate the correct crate file is used. This is `None` for sources /// that do not use `.crate` files, like path or git dependencies. @@ -50,6 +48,7 @@ pub struct Resolve { /// Version of the `Cargo.lock` format, see /// `cargo::core::resolver::encode` for more. version: ResolveVersion, + summaries: HashMap, } /// A version to indicate how a `Cargo.lock` should be serialized. Currently @@ -73,11 +72,12 @@ impl Resolve { pub fn new( graph: Graph>, replacements: HashMap, - features: HashMap>, + features: HashMap>, checksums: HashMap>, metadata: Metadata, unused_patches: Vec, version: ResolveVersion, + summaries: HashMap, ) -> Resolve { let reverse_replacements = replacements.iter().map(|(&p, &r)| (r, p)).collect(); let public_dependencies = graph @@ -103,10 +103,11 @@ impl Resolve { checksums, metadata, unused_patches, - empty_features: HashSet::new(), + empty_features: Vec::new(), reverse_replacements, public_dependencies, version, + summaries, } } @@ -285,10 +286,16 @@ unable to verify that `{0}` is the same as when the lockfile was generated &self.replacements } - pub fn features(&self, pkg: PackageId) -> &HashSet { + pub fn features(&self, pkg: PackageId) -> &[InternedString] { self.features.get(&pkg).unwrap_or(&self.empty_features) } + /// This is only here for legacy support, it will be removed when + /// switching to the new feature resolver. + pub fn features_clone(&self) -> HashMap> { + self.features.clone() + } + pub fn is_public_dep(&self, pkg: PackageId, dep: PackageId) -> bool { self.public_dependencies .get(&pkg) @@ -296,12 +303,6 @@ unable to verify that `{0}` is the same as when the lockfile was generated .unwrap_or_else(|| panic!("Unknown dependency {:?} for package {:?}", dep, pkg)) } - pub fn features_sorted(&self, pkg: PackageId) -> Vec<&str> { - let mut v = Vec::from_iter(self.features(pkg).iter().map(|s| s.as_ref())); - v.sort_unstable(); - v - } - pub fn query(&self, spec: &str) -> CargoResult { PackageIdSpec::query_str(spec, self.iter()) } @@ -374,6 +375,10 @@ unable to verify that `{0}` is the same as when the lockfile was generated pub fn version(&self) -> &ResolveVersion { &self.version } + + pub fn summary(&self, pkg_id: PackageId) -> &Summary { + &self.summaries[&pkg_id] + } } impl PartialEq for Resolve { @@ -388,7 +393,7 @@ impl PartialEq for Resolve { compare! { // fields to compare graph replacements reverse_replacements empty_features features - checksums metadata unused_patches public_dependencies + checksums metadata unused_patches public_dependencies summaries | // fields to ignore version diff --git a/src/cargo/ops/cargo_clean.rs b/src/cargo/ops/cargo_clean.rs index 0acaff53c70..1af3df713f0 100644 --- a/src/cargo/ops/cargo_clean.rs +++ b/src/cargo/ops/cargo_clean.rs @@ -6,8 +6,10 @@ use std::path::Path; use crate::core::compiler::unit_dependencies; use crate::core::compiler::{BuildConfig, BuildContext, CompileKind, CompileMode, Context}; use crate::core::compiler::{RustcTargetData, UnitInterner}; +use crate::core::dependency::DepKind; use crate::core::profiles::{Profiles, UnitFor}; -use crate::core::Workspace; +use crate::core::resolver::features::{FeatureResolver, RequestedFeatures}; +use crate::core::{PackageIdSpec, Workspace}; use crate::ops; use crate::util::errors::{CargoResult, CargoResultExt}; use crate::util::paths; @@ -72,6 +74,20 @@ pub fn clean(ws: &Workspace<'_>, opts: &CleanOptions<'_>) -> CargoResult<()> { HashMap::new(), target_data, )?; + let requested_features = RequestedFeatures::new_all(true); + let specs = opts + .spec + .iter() + .map(|spec| PackageIdSpec::parse(spec)) + .collect::>>()?; + let features = FeatureResolver::resolve( + ws, + &bcx.target_data, + &resolve, + &requested_features, + &specs, + bcx.build_config.requested_kind, + )?; let mut units = Vec::new(); for spec in opts.spec.iter() { @@ -100,10 +116,13 @@ pub fn clean(ws: &Workspace<'_>, opts: &CleanOptions<'_>) -> CargoResult<()> { *mode, ) }; - let features = resolve.features_sorted(pkg.package_id()); - units.push(bcx.units.intern( - pkg, target, profile, *kind, *mode, features, /*is_std*/ false, - )); + for dep_kind in &[DepKind::Normal, DepKind::Development, DepKind::Build] { + let features = + features.activated_features(pkg.package_id(), *dep_kind, *kind); + units.push(bcx.units.intern( + pkg, target, profile, *kind, *mode, features, /*is_std*/ false, + )); + } } } } @@ -111,7 +130,7 @@ pub fn clean(ws: &Workspace<'_>, opts: &CleanOptions<'_>) -> CargoResult<()> { } let unit_dependencies = - unit_dependencies::build_unit_dependencies(&bcx, &resolve, None, &units, &[])?; + unit_dependencies::build_unit_dependencies(&bcx, &resolve, &features, None, &units, &[])?; let mut cx = Context::new(config, &bcx, unit_dependencies, build_config.requested_kind)?; cx.prepare_units(None, &units)?; diff --git a/src/cargo/ops/cargo_compile.rs b/src/cargo/ops/cargo_compile.rs index 1e3d8b7e748..903f20138cd 100644 --- a/src/cargo/ops/cargo_compile.rs +++ b/src/cargo/ops/cargo_compile.rs @@ -33,7 +33,9 @@ use crate::core::compiler::unit_dependencies::build_unit_dependencies; use crate::core::compiler::{BuildConfig, BuildContext, Compilation, Context}; use crate::core::compiler::{CompileKind, CompileMode, RustcTargetData, Unit}; use crate::core::compiler::{DefaultExecutor, Executor, UnitInterner}; +use crate::core::dependency::DepKind; use crate::core::profiles::{Profiles, UnitFor}; +use crate::core::resolver::features; use crate::core::resolver::{Resolve, ResolveOpts}; use crate::core::{LibKind, Package, PackageSet, Target}; use crate::core::{PackageId, PackageIdSpec, TargetKind, Workspace}; @@ -311,14 +313,16 @@ pub fn compile_ws<'a>( 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, features, all_features, !no_default_features); - let resolve = ops::resolve_ws_with_opts(ws, &opts, &specs)?; + let resolve = + ops::resolve_ws_with_opts(ws, &target_data, build_config.requested_kind, &opts, &specs)?; let WorkspaceResolve { mut pkg_set, workspace_resolve, targeted_resolve: resolve, + resolved_features, } = resolve; - let std_resolve = if let Some(crates) = &config.cli_unstable().build_std { + let std_resolve_features = if let Some(crates) = &config.cli_unstable().build_std { if build_config.build_plan { config .shell() @@ -330,10 +334,11 @@ pub fn compile_ws<'a>( // requested_target to an enum, or some other approach. anyhow::bail!("-Zbuild-std requires --target"); } - let (mut std_package_set, std_resolve) = standard_lib::resolve_std(ws, crates)?; + let (mut std_package_set, std_resolve, std_features) = + standard_lib::resolve_std(ws, &target_data, build_config.requested_kind, crates)?; remove_dylib_crate_type(&mut std_package_set)?; pkg_set.add_set(std_package_set); - Some(std_resolve) + Some((std_resolve, std_features)) } else { None }; @@ -407,6 +412,7 @@ pub fn compile_ws<'a>( filter, build_config.requested_kind, &resolve, + &resolved_features, &bcx, )?; @@ -423,10 +429,12 @@ pub fn compile_ws<'a>( crates.push("test".to_string()); } } + let (std_resolve, std_features) = std_resolve_features.as_ref().unwrap(); standard_lib::generate_std_roots( &bcx, &crates, - std_resolve.as_ref().unwrap(), + std_resolve, + std_features, build_config.requested_kind, )? } else { @@ -463,8 +471,14 @@ pub fn compile_ws<'a>( } } - let unit_dependencies = - build_unit_dependencies(&bcx, &resolve, std_resolve.as_ref(), &units, &std_roots)?; + let unit_dependencies = build_unit_dependencies( + &bcx, + &resolve, + &resolved_features, + std_resolve_features.as_ref(), + &units, + &std_roots, + )?; let ret = { let _p = profile::start("compiling"); @@ -661,6 +675,7 @@ fn generate_targets<'a>( filter: &CompileFilter, default_arch_kind: CompileKind, resolve: &'a Resolve, + resolved_features: &features::ResolvedFeatures, bcx: &BuildContext<'a, '_>, ) -> CargoResult>> { // Helper for creating a `Unit` struct. @@ -723,7 +738,12 @@ fn generate_targets<'a>( let profile = bcx.profiles .get_profile(pkg.package_id(), ws.is_member(pkg), unit_for, target_mode); - let features = resolve.features_sorted(pkg.package_id()); + + let features = Vec::from(resolved_features.activated_features( + pkg.package_id(), + DepKind::Normal, + kind, + )); bcx.units.intern( pkg, target, @@ -857,6 +877,10 @@ fn generate_targets<'a>( // Only include targets that are libraries or have all required // features available. + // + // `features_map` is a map of &Package -> enabled_features + // It is computed by the set of enabled features for the package plus + // every enabled feature of every enabled dependency. let mut features_map = HashMap::new(); let mut units = HashSet::new(); for Proposal { @@ -868,9 +892,14 @@ fn generate_targets<'a>( { let unavailable_features = match target.required_features() { Some(rf) => { - let features = features_map - .entry(pkg) - .or_insert_with(|| resolve_all_features(resolve, pkg.package_id())); + let features = features_map.entry(pkg).or_insert_with(|| { + resolve_all_features( + resolve, + resolved_features, + pkg.package_id(), + default_arch_kind, + ) + }); rf.iter().filter(|f| !features.contains(*f)).collect() } None => Vec::new(), @@ -900,16 +929,24 @@ fn generate_targets<'a>( fn resolve_all_features( resolve_with_overrides: &Resolve, + resolved_features: &features::ResolvedFeatures, package_id: PackageId, + default_arch_kind: CompileKind, ) -> HashSet { - let mut features = resolve_with_overrides.features(package_id).clone(); + let mut features: HashSet = resolved_features + .activated_features(package_id, DepKind::Normal, default_arch_kind) + .iter() + .map(|s| s.to_string()) + .collect(); // Include features enabled for use by dependencies so targets can also use them with the // required-features field when deciding whether to be built or skipped. for (dep_id, deps) in resolve_with_overrides.deps(package_id) { - for feature in resolve_with_overrides.features(dep_id) { + for feature in + resolved_features.activated_features(dep_id, DepKind::Normal, default_arch_kind) + { for dep in deps { - features.insert(dep.name_in_toml().to_string() + "/" + feature); + features.insert(dep.name_in_toml().to_string() + "/" + &feature); } } } diff --git a/src/cargo/ops/cargo_doc.rs b/src/cargo/ops/cargo_doc.rs index a3432aa7315..7699fb33060 100644 --- a/src/cargo/ops/cargo_doc.rs +++ b/src/cargo/ops/cargo_doc.rs @@ -1,3 +1,4 @@ +use crate::core::compiler::RustcTargetData; use crate::core::resolver::ResolveOpts; use crate::core::{Shell, Workspace}; use crate::ops; @@ -24,7 +25,9 @@ pub fn doc(ws: &Workspace<'_>, options: &DocOptions<'_>) -> CargoResult<()> { options.compile_opts.all_features, !options.compile_opts.no_default_features, ); - let ws_resolve = ops::resolve_ws_with_opts(ws, &opts, &specs)?; + let requested_kind = options.compile_opts.build_config.requested_kind; + let target_data = RustcTargetData::new(ws, requested_kind)?; + let ws_resolve = ops::resolve_ws_with_opts(ws, &target_data, requested_kind, &opts, &specs)?; let ids = specs .iter() diff --git a/src/cargo/ops/cargo_install.rs b/src/cargo/ops/cargo_install.rs index 1ffee70cf9c..880dbbc6da9 100644 --- a/src/cargo/ops/cargo_install.rs +++ b/src/cargo/ops/cargo_install.rs @@ -7,7 +7,7 @@ use anyhow::{bail, format_err}; use tempfile::Builder as TempFileBuilder; use crate::core::compiler::Freshness; -use crate::core::compiler::{CompileKind, DefaultExecutor, Executor}; +use crate::core::compiler::{CompileKind, DefaultExecutor, Executor, RustcTargetData}; use crate::core::resolver::ResolveOpts; use crate::core::{Edition, Package, PackageId, PackageIdSpec, Source, SourceId, Workspace}; use crate::ops; @@ -492,10 +492,21 @@ fn check_yanked_install(ws: &Workspace<'_>) -> CargoResult<()> { return Ok(()); } let specs = vec![PackageIdSpec::from_package_id(ws.current()?.package_id())]; + // CompileKind here doesn't really matter, it's only needed for features. + let target_data = RustcTargetData::new(ws, CompileKind::Host)?; // It would be best if `source` could be passed in here to avoid a // duplicate "Updating", but since `source` is taken by value, then it // wouldn't be available for `compile_ws`. - let ws_resolve = ops::resolve_ws_with_opts(ws, &ResolveOpts::everything(), &specs)?; + // TODO: It would be easier to use resolve_ws, but it does not honor + // require_optional_deps to avoid writing the lock file. It might be good + // to try to fix that. + let ws_resolve = ops::resolve_ws_with_opts( + ws, + &target_data, + CompileKind::Host, + &ResolveOpts::everything(), + &specs, + )?; let mut sources = ws_resolve.pkg_set.sources_mut(); // Checking the yanked status involves taking a look at the registry and diff --git a/src/cargo/ops/cargo_output_metadata.rs b/src/cargo/ops/cargo_output_metadata.rs index 900ed22a081..b7f36290f4f 100644 --- a/src/cargo/ops/cargo_output_metadata.rs +++ b/src/cargo/ops/cargo_output_metadata.rs @@ -1,6 +1,8 @@ -use crate::core::compiler::{CompileKind, CompileTarget, TargetInfo}; +use crate::core::compiler::{CompileKind, CompileTarget, RustcTargetData}; use crate::core::resolver::{Resolve, ResolveOpts}; -use crate::core::{dependency, Dependency, Package, PackageId, Workspace}; + +use crate::core::dependency::DepKind; +use crate::core::{Dependency, InternedString, Package, PackageId, Workspace}; use crate::ops::{self, Packages}; use crate::util::CargoResult; use cargo_platform::Platform; @@ -34,13 +36,7 @@ pub fn output_metadata(ws: &Workspace<'_>, opt: &OutputMetadataOptions) -> Cargo let packages = ws.members().cloned().collect(); (packages, None) } else { - let resolve_opts = ResolveOpts::new( - /*dev_deps*/ true, - &opt.features, - opt.all_features, - !opt.no_default_features, - ); - let (packages, resolve) = build_resolve_graph(ws, resolve_opts, &opt.filter_platform)?; + let (packages, resolve) = build_resolve_graph(ws, opt)?; (packages, Some(resolve)) }; @@ -78,7 +74,7 @@ struct MetadataResolveNode { id: PackageId, dependencies: Vec, deps: Vec, - features: Vec, + features: Vec, } #[derive(Serialize)] @@ -90,7 +86,7 @@ struct Dep { #[derive(Serialize, PartialEq, Eq, PartialOrd, Ord)] struct DepKindInfo { - kind: dependency::DepKind, + kind: DepKind, target: Option, } @@ -106,23 +102,25 @@ impl From<&Dependency> for DepKindInfo { /// Builds the resolve graph as it will be displayed to the user. fn build_resolve_graph( ws: &Workspace<'_>, - resolve_opts: ResolveOpts, - target: &Option, + metadata_opts: &OutputMetadataOptions, ) -> CargoResult<(Vec, MetadataResolve)> { - let target_info = match target { - Some(target) => { - let config = ws.config(); - let ct = CompileTarget::new(target)?; - let short_name = ct.short_name().to_string(); - let kind = CompileKind::Target(ct); - let rustc = config.load_global_rustc(Some(ws))?; - Some((short_name, TargetInfo::new(config, kind, &rustc, kind)?)) - } - None => None, + // TODO: Without --filter-platform, features are being resolved for `host` only. + // How should this work? + let requested_kind = match &metadata_opts.filter_platform { + Some(t) => CompileKind::Target(CompileTarget::new(t)?), + None => CompileKind::Host, }; + let target_data = RustcTargetData::new(ws, requested_kind)?; // Resolve entire workspace. let specs = Packages::All.to_package_id_specs(ws)?; - let ws_resolve = ops::resolve_ws_with_opts(ws, &resolve_opts, &specs)?; + let resolve_opts = ResolveOpts::new( + /*dev_deps*/ true, + &metadata_opts.features, + metadata_opts.all_features, + !metadata_opts.no_default_features, + ); + let ws_resolve = + ops::resolve_ws_with_opts(ws, &target_data, requested_kind, &resolve_opts, &specs)?; // Download all Packages. This is needed to serialize the information // for every package. In theory this could honor target filtering, // but that would be somewhat complex. @@ -132,6 +130,7 @@ fn build_resolve_graph( .into_iter() .map(|pkg| (pkg.package_id(), pkg.clone())) .collect(); + // Start from the workspace roots, and recurse through filling out the // map, filtering targets as necessary. let mut node_map = HashMap::new(); @@ -141,7 +140,8 @@ fn build_resolve_graph( member_pkg.package_id(), &ws_resolve.targeted_resolve, &package_map, - target_info.as_ref(), + &target_data, + requested_kind, ); } // Get a Vec of Packages. @@ -161,27 +161,22 @@ fn build_resolve_graph_r( pkg_id: PackageId, resolve: &Resolve, package_map: &HashMap, - target: Option<&(String, TargetInfo)>, + target_data: &RustcTargetData, + requested_kind: CompileKind, ) { if node_map.contains_key(&pkg_id) { return; } - let features = resolve - .features_sorted(pkg_id) - .into_iter() - .map(|s| s.to_string()) - .collect(); + let features = resolve.features(pkg_id).into_iter().cloned().collect(); + let deps: Vec = resolve .deps(pkg_id) - .filter(|(_dep_id, deps)| match target { - Some((short_name, info)) => deps.iter().any(|dep| { - let platform = match dep.platform() { - Some(p) => p, - None => return true, - }; - platform.matches(short_name, info.cfg()) - }), - None => true, + .filter(|(_dep_id, deps)| match requested_kind { + CompileKind::Target(_) => deps + .iter() + .any(|dep| target_data.dep_platform_activated(dep, requested_kind)), + // No --filter-platform is interpreted as "all platforms". + CompileKind::Host => true, }) .filter_map(|(dep_id, deps)| { let mut dep_kinds: Vec<_> = deps.iter().map(DepKindInfo::from).collect(); @@ -210,6 +205,13 @@ fn build_resolve_graph_r( }; node_map.insert(pkg_id, node); for dep_id in to_visit { - build_resolve_graph_r(node_map, dep_id, resolve, package_map, target); + build_resolve_graph_r( + node_map, + dep_id, + resolve, + package_map, + target_data, + requested_kind, + ); } } diff --git a/src/cargo/ops/cargo_package.rs b/src/cargo/ops/cargo_package.rs index 3906678062f..21522c57307 100644 --- a/src/cargo/ops/cargo_package.rs +++ b/src/cargo/ops/cargo_package.rs @@ -16,10 +16,10 @@ use serde_json::{self, json}; use tar::{Archive, Builder, EntryType, Header}; use crate::core::compiler::{BuildConfig, CompileMode, DefaultExecutor, Executor}; -use crate::core::resolver::ResolveOpts; + use crate::core::Feature; use crate::core::{ - Package, PackageId, PackageIdSpec, PackageSet, Resolve, Source, SourceId, Verbosity, Workspace, + Package, PackageId, PackageSet, Resolve, Source, SourceId, Verbosity, Workspace, }; use crate::ops; use crate::sources::PathSource; @@ -152,21 +152,15 @@ fn build_lock(ws: &Workspace<'_>) -> CargoResult { let new_pkg = Package::new(manifest, orig_pkg.manifest_path()); // Regenerate Cargo.lock using the old one as a guide. - let specs = vec![PackageIdSpec::from_package_id(new_pkg.package_id())]; let tmp_ws = Workspace::ephemeral(new_pkg, ws.config(), None, true)?; - let new_resolve = ops::resolve_ws_with_opts(&tmp_ws, &ResolveOpts::everything(), &specs)?; + let (pkg_set, new_resolve) = ops::resolve_ws(&tmp_ws)?; if let Some(orig_resolve) = orig_resolve { - compare_resolve( - config, - tmp_ws.current()?, - &orig_resolve, - &new_resolve.targeted_resolve, - )?; + compare_resolve(config, tmp_ws.current()?, &orig_resolve, &new_resolve)?; } - check_yanked(config, &new_resolve.pkg_set, &new_resolve.targeted_resolve)?; + check_yanked(config, &pkg_set, &new_resolve)?; - ops::resolve_to_string(&tmp_ws, &new_resolve.targeted_resolve) + ops::resolve_to_string(&tmp_ws, &new_resolve) } // Checks that the package has some piece of metadata that a human can diff --git a/src/cargo/ops/resolve.rs b/src/cargo/ops/resolve.rs index 8d160e256c7..1422f8ff44f 100644 --- a/src/cargo/ops/resolve.rs +++ b/src/cargo/ops/resolve.rs @@ -10,8 +10,9 @@ //! - `resolve_with_previous`: A low-level function for running the resolver, //! providing the most power and flexibility. +use crate::core::compiler::{CompileKind, RustcTargetData}; use crate::core::registry::PackageRegistry; -use crate::core::resolver::features::RequestedFeatures; +use crate::core::resolver::features::{FeatureResolver, RequestedFeatures, ResolvedFeatures}; use crate::core::resolver::{self, Resolve, ResolveOpts}; use crate::core::Feature; use crate::core::{PackageId, PackageIdSpec, PackageSet, Source, SourceId, Workspace}; @@ -34,6 +35,8 @@ pub struct WorkspaceResolve<'a> { /// The narrowed resolve, with the specific features enabled, and only the /// given package specs requested. pub targeted_resolve: Resolve, + /// The features activated per package. + pub resolved_features: ResolvedFeatures, } const UNUSED_PATCH_WARNING: &str = "\ @@ -70,6 +73,8 @@ pub fn resolve_ws<'a>(ws: &Workspace<'a>) -> CargoResult<(PackageSet<'a>, Resolv /// members. In this case, `opts.all_features` must be `true`. pub fn resolve_ws_with_opts<'a>( ws: &Workspace<'a>, + target_data: &RustcTargetData, + requested_target: CompileKind, opts: &ResolveOpts, specs: &[PackageIdSpec], ) -> CargoResult> { @@ -119,10 +124,20 @@ pub fn resolve_ws_with_opts<'a>( let pkg_set = get_resolved_packages(&resolved_with_overrides, registry)?; + let resolved_features = FeatureResolver::resolve( + ws, + target_data, + &resolved_with_overrides, + &opts.features, + specs, + requested_target, + )?; + Ok(WorkspaceResolve { pkg_set, workspace_resolve: resolve, targeted_resolve: resolved_with_overrides, + resolved_features, }) } diff --git a/src/doc/src/reference/unstable.md b/src/doc/src/reference/unstable.md index bb0f8d8b889..6848052713c 100644 --- a/src/doc/src/reference/unstable.md +++ b/src/doc/src/reference/unstable.md @@ -473,3 +473,65 @@ cargo +nightly -Zunstable-options -Zconfig-include --config somefile.toml build ``` CLI paths are relative to the current working directory. + +## Features + +The `-Zfeatures` option causes Cargo to use a new feature resolver that can +resolve features differently from before. It takes a comma separated list of +options to indicate which new behaviors to enable. With no options, it should +behave the same as without the flag. + +```console +cargo +nightly -Zfeatures=itarget,build_dep +``` + +The available options are: + +* `itarget` — Ignores features for target-specific dependencies for targets + that don't match the current compile target. For example: + + ```toml + [dependency.common] + version = "1.0" + features = ["f1"] + + [target.'cfg(windows)'.dependencies.common] + version = "1.0" + features = ["f2"] + ``` + + When building this example for a non-Windows platform, the `f2` feature will + *not* be enabled. + +* `build_dep` — Prevents features enabled on build dependencies from being + enabled for normal dependencies. For example: + + ```toml + [dependencies] + log = "0.4" + + [build-dependencies] + log = {version = "0.4", features=['std']} + ``` + + When building the build script, the `log` crate will be built with the `std` + feature. When building the library of your package, it will not enable the + feature. + +* `dev_dep` — Prevents features enabled on dev dependencies from being enabled + for normal dependencies. For example: + + ```toml + [dependencies] + serde = {version = "1.0", default-features = false} + + [dev-dependencies] + serde = {version = "1.0", features = ["std"]} + ``` + + In this example, the library will normally link against `serde` without the + `std` feature. However, when built as a test or example, it will include the + `std` feature. + +* `compare` — This option compares the resolved features to the old resolver, + and will print any differences. diff --git a/tests/testsuite/features.rs b/tests/testsuite/features.rs index 98cbbed89a5..e5487610647 100644 --- a/tests/testsuite/features.rs +++ b/tests/testsuite/features.rs @@ -2142,3 +2142,53 @@ fn all_features_virtual_ws() { .with_stdout("f1\nf2\nf3\n") .run(); } + +#[cargo_test] +fn slash_optional_enables() { + // --features dep/feat will enable `dep` and set its feature. + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + + [dependencies] + dep = {path="dep", optional=true} + "#, + ) + .file( + "src/lib.rs", + r#" + #[cfg(not(feature="dep"))] + compile_error!("dep not set"); + "#, + ) + .file( + "dep/Cargo.toml", + r#" + [package] + name = "dep" + version = "0.1.0" + + [features] + feat = [] + "#, + ) + .file( + "dep/src/lib.rs", + r#" + #[cfg(not(feature="feat"))] + compile_error!("feat not set"); + "#, + ) + .build(); + + p.cargo("check") + .with_status(101) + .with_stderr_contains("[..]dep not set[..]") + .run(); + + p.cargo("check --features dep/feat").run(); +} diff --git a/tests/testsuite/features2.rs b/tests/testsuite/features2.rs new file mode 100644 index 00000000000..bb91a95204f --- /dev/null +++ b/tests/testsuite/features2.rs @@ -0,0 +1,396 @@ +//! Tests for the new feature resolver. + +use cargo_test_support::project; +use cargo_test_support::registry::{Dependency, Package}; + +#[cargo_test] +fn inactivate_targets() { + // Basic test of `itarget`. A shared dependency where an inactive [target] + // changes the features. + Package::new("common", "1.0.0") + .feature("f1", &[]) + .file( + "src/lib.rs", + r#" + #[cfg(feature = "f1")] + compile_error!("f1 should not activate"); + "#, + ) + .publish(); + + Package::new("bar", "1.0.0") + .add_dep( + Dependency::new("common", "1.0") + .target("cfg(whatever)") + .enable_features(&["f1"]), + ) + .publish(); + + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + + [dependencies] + common = "1.0" + bar = "1.0" + "#, + ) + .file("src/lib.rs", "") + .build(); + + p.cargo("check") + .with_status(101) + .with_stderr_contains("[..]f1 should not activate[..]") + .run(); + + p.cargo("check -Zfeatures=itarget") + .masquerade_as_nightly_cargo() + .run(); +} + +#[cargo_test] +fn inactive_target_optional() { + // Activating optional [target] dependencies for inactivate target. + Package::new("common", "1.0.0") + .feature("f1", &[]) + .feature("f2", &[]) + .feature("f3", &[]) + .feature("f4", &[]) + .file( + "src/lib.rs", + r#" + pub fn f() { + if cfg!(feature="f1") { println!("f1"); } + if cfg!(feature="f2") { println!("f2"); } + if cfg!(feature="f3") { println!("f3"); } + if cfg!(feature="f4") { println!("f4"); } + } + "#, + ) + .publish(); + + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + edition = "2018" + + [dependencies] + common = "1.0" + + [target.'cfg(whatever)'.dependencies] + dep1 = {path='dep1', optional=true} + dep2 = {path='dep2', optional=true, features=["f3"]} + common = {version="1.0", optional=true, features=["f4"]} + + [features] + foo1 = ["dep1/f2"] + "#, + ) + .file( + "src/main.rs", + r#" + fn main() { + if cfg!(feature="foo1") { println!("foo1"); } + if cfg!(feature="dep1") { println!("dep1"); } + if cfg!(feature="dep2") { println!("dep2"); } + if cfg!(feature="common") { println!("common"); } + common::f(); + } + "#, + ) + .file( + "dep1/Cargo.toml", + r#" + [package] + name = "dep1" + version = "0.1.0" + + [dependencies] + common = {version="1.0", features=["f1"]} + + [features] + f2 = ["common/f2"] + "#, + ) + .file( + "dep1/src/lib.rs", + r#"compile_error!("dep1 should not build");"#, + ) + .file( + "dep2/Cargo.toml", + r#" + [package] + name = "dep2" + version = "0.1.0" + + [dependencies] + common = "1.0" + + [features] + f3 = ["common/f3"] + "#, + ) + .file( + "dep2/src/lib.rs", + r#"compile_error!("dep2 should not build");"#, + ) + .build(); + + p.cargo("run --all-features") + .with_stdout("foo1\ndep1\ndep2\ncommon\nf1\nf2\nf3\nf4\n") + .run(); + p.cargo("run --features dep1") + .with_stdout("dep1\nf1\n") + .run(); + p.cargo("run --features foo1") + .with_stdout("foo1\ndep1\nf1\nf2\n") + .run(); + p.cargo("run --features dep2") + .with_stdout("dep2\nf3\n") + .run(); + p.cargo("run --features common") + .with_stdout("common\nf4\n") + .run(); + + p.cargo("run -Zfeatures=itarget --all-features") + .masquerade_as_nightly_cargo() + .with_stdout("foo1\n") + .run(); + p.cargo("run -Zfeatures=itarget --features dep1") + .masquerade_as_nightly_cargo() + .with_stdout("dep1\n") + .run(); + p.cargo("run -Zfeatures=itarget --features foo1") + .masquerade_as_nightly_cargo() + .with_stdout("foo1\n") + .run(); + p.cargo("run -Zfeatures=itarget --features dep2") + .masquerade_as_nightly_cargo() + .with_stdout("dep2\n") + .run(); + p.cargo("run -Zfeatures=itarget --features common") + .masquerade_as_nightly_cargo() + .with_stdout("common") + .run(); +} + +#[cargo_test] +fn decouple_build_deps() { + // Basic test for `build_dep` decouple. + Package::new("common", "1.0.0") + .feature("f1", &[]) + .file( + "src/lib.rs", + r#" + #[cfg(feature = "f1")] + pub fn foo() {} + #[cfg(not(feature = "f1"))] + pub fn bar() {} + "#, + ) + .publish(); + + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + edition = "2018" + + [build-dependencies] + common = {version="1.0", features=["f1"]} + + [dependencies] + common = "1.0" + "#, + ) + .file( + "build.rs", + r#" + use common::foo; + fn main() {} + "#, + ) + .file("src/lib.rs", "use common::bar;") + .build(); + + p.cargo("check") + .with_status(101) + .with_stderr_contains("[..]unresolved import `common::bar`[..]") + .run(); + + p.cargo("check -Zfeatures=build_dep") + .masquerade_as_nightly_cargo() + .run(); +} + +#[cargo_test] +fn decouple_build_deps_nested() { + // `build_dep` decouple of transitive dependencies. + Package::new("common", "1.0.0") + .feature("f1", &[]) + .file( + "src/lib.rs", + r#" + #[cfg(feature = "f1")] + pub fn foo() {} + #[cfg(not(feature = "f1"))] + pub fn bar() {} + "#, + ) + .publish(); + + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + edition = "2018" + + [build-dependencies] + bdep = {path="bdep"} + + [dependencies] + common = "1.0" + "#, + ) + .file( + "build.rs", + r#" + use bdep::foo; + fn main() {} + "#, + ) + .file("src/lib.rs", "use common::bar;") + .file( + "bdep/Cargo.toml", + r#" + [package] + name = "bdep" + version = "0.1.0" + edition = "2018" + + [dependencies] + common = {version="1.0", features=["f1"]} + "#, + ) + .file("bdep/src/lib.rs", "pub use common::foo;") + .build(); + + p.cargo("check") + .with_status(101) + .with_stderr_contains("[..]unresolved import `common::bar`[..]") + .run(); + + p.cargo("check -Zfeatures=build_dep") + .masquerade_as_nightly_cargo() + .run(); +} + +#[cargo_test] +fn decouple_dev_deps() { + // Basic test for `dev_dep` decouple. + Package::new("common", "1.0.0") + .feature("f1", &[]) + .feature("f2", &[]) + .file( + "src/lib.rs", + r#" + pub fn foo() -> u32 { + let mut res = 0; + if cfg!(feature = "f1") { + res |= 1; + } + if cfg!(feature = "f2") { + res |= 2; + } + res + } + "#, + ) + .publish(); + + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + edition = "2018" + + [dependencies] + common = {version="1.0", features=["f1"]} + + [dev-dependencies] + common = {version="1.0", features=["f2"]} + "#, + ) + .file( + "src/main.rs", + r#" + fn main() { + assert_eq!(foo::foo(), 1); + assert_eq!(common::foo(), 1); + } + + #[test] + fn test_bin() { + assert_eq!(foo::foo(), 3); + assert_eq!(common::foo(), 3); + } + "#, + ) + .file( + "src/lib.rs", + r#" + pub fn foo() -> u32 { + common::foo() + } + + #[test] + fn test_lib() { + assert_eq!(foo(), 3); + assert_eq!(common::foo(), 3); + } + "#, + ) + .file( + "tests/t1.rs", + r#" + #[test] + fn test_t1() { + assert_eq!(foo::foo(), 3); + assert_eq!(common::foo(), 3); + } + "#, + ) + .build(); + + p.cargo("run") + .with_status(101) + .with_stderr_contains("[..]assertion failed[..]") + .run(); + + p.cargo("run -Zfeatures=dev_dep") + .masquerade_as_nightly_cargo() + .run(); + + p.cargo("test").run(); + + p.cargo("test -Zfeatures=dev_dep") + .masquerade_as_nightly_cargo() + .run(); +} diff --git a/tests/testsuite/main.rs b/tests/testsuite/main.rs index fa22a1e7658..00bafadc6cb 100644 --- a/tests/testsuite/main.rs +++ b/tests/testsuite/main.rs @@ -44,6 +44,7 @@ mod doc; mod edition; mod error; mod features; +mod features2; mod fetch; mod fix; mod freshness; From d728f386e73e02a98d49892a2913b9821b23589f Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Thu, 30 Jan 2020 11:12:23 -0800 Subject: [PATCH 04/22] Ensure dev-dep features are unified if any are used. There is a complex issue where `cargo test -Zfeatures=dev_dep` was building things incorrectly if there was a binary executable. The issue is that lib.rs needed to be built twice (once linked against normal dependencies, once against dev dependencies), but the current Unit structure can't distinguish between the two, and thus it was picking the wrong one. Instead of allowing `cargo test` to build binaries without dev-dependency features, just link main.rs against the dev-dependency-unified versions. We may need to revisit this in the future, but for now it is not clear what people want or how this should work. Fixing this would require substantial changes to how unit dependencies are computed (to properly handle deduplication), so for now we'll use a simpler solution. It would also mean `cargo test` would take longer on some projects (because it would need to build the library 3 times instead of twice). --- src/cargo/core/compiler/standard_lib.rs | 3 +- src/cargo/core/resolver/features.rs | 16 ++++++++-- src/cargo/ops/cargo_clean.rs | 1 + src/cargo/ops/cargo_compile.rs | 11 +++++-- src/cargo/ops/cargo_doc.rs | 3 +- src/cargo/ops/cargo_install.rs | 1 + src/cargo/ops/cargo_output_metadata.rs | 10 ++++-- src/cargo/ops/resolve.rs | 2 ++ src/doc/src/reference/unstable.md | 4 +++ tests/testsuite/features2.rs | 41 ++++++++++++++++++++----- 10 files changed, 76 insertions(+), 16 deletions(-) diff --git a/src/cargo/core/compiler/standard_lib.rs b/src/cargo/core/compiler/standard_lib.rs index fbc2667d93a..f4bd0345c89 100644 --- a/src/cargo/core/compiler/standard_lib.rs +++ b/src/cargo/core/compiler/standard_lib.rs @@ -103,7 +103,8 @@ pub fn resolve_std<'cfg>( /*dev_deps*/ false, &features, /*all_features*/ false, /*uses_default_features*/ true, ); - let resolve = ops::resolve_ws_with_opts(&std_ws, target_data, requested_target, &opts, &specs)?; + let resolve = + ops::resolve_ws_with_opts(&std_ws, target_data, requested_target, &opts, &specs, false)?; Ok(( resolve.pkg_set, resolve.targeted_resolve, diff --git a/src/cargo/core/resolver/features.rs b/src/cargo/core/resolver/features.rs index 64c7b470251..9f78aa9c587 100644 --- a/src/cargo/core/resolver/features.rs +++ b/src/cargo/core/resolver/features.rs @@ -53,7 +53,7 @@ struct FeatureOpts { } impl FeatureOpts { - fn new(config: &Config) -> CargoResult { + fn new(config: &Config, has_dev_units: bool) -> CargoResult { let mut opts = FeatureOpts::default(); let unstable_flags = config.cli_unstable(); opts.package_features = unstable_flags.package_features; @@ -84,6 +84,15 @@ impl FeatureOpts { enable(&env_opts)?; } } + if has_dev_units { + // Decoupling of dev deps is not allowed if any test/bench/example + // is being built. It may be possible to relax this in the future, + // but it will require significant changes to how unit + // dependencies are computed, and can result in longer build times + // with `cargo test` because the lib may need to be built 3 times + // instead of twice. + opts.decouple_dev_deps = false; + } Ok(opts) } } @@ -141,7 +150,6 @@ impl ResolvedFeatures { if let Some(legacy) = &self.legacy { legacy.get(&pkg_id).map_or_else(Vec::new, |v| v.clone()) } else { - // TODO: Remove panic, return empty set. let dep_kind = if (!self.opts.decouple_build_deps && dep_kind == DepKind::Build) || (!self.opts.decouple_dev_deps && dep_kind == DepKind::Development) { @@ -150,6 +158,7 @@ impl ResolvedFeatures { } else { dep_kind }; + // TODO: Remove panic, return empty set. let fs = self .activated_features .get(&(pkg_id, dep_kind, compile_kind)) @@ -188,11 +197,12 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> { requested_features: &RequestedFeatures, specs: &[PackageIdSpec], requested_target: CompileKind, + has_dev_units: bool, ) -> CargoResult { use crate::util::profile; let _p = profile::start("resolve features"); - let opts = FeatureOpts::new(ws.config())?; + let opts = FeatureOpts::new(ws.config(), has_dev_units)?; if !opts.new_resolver { // Legacy mode. return Ok(ResolvedFeatures { diff --git a/src/cargo/ops/cargo_clean.rs b/src/cargo/ops/cargo_clean.rs index 1af3df713f0..223a6c57302 100644 --- a/src/cargo/ops/cargo_clean.rs +++ b/src/cargo/ops/cargo_clean.rs @@ -87,6 +87,7 @@ pub fn clean(ws: &Workspace<'_>, opts: &CleanOptions<'_>) -> CargoResult<()> { &requested_features, &specs, bcx.build_config.requested_kind, + true, )?; let mut units = Vec::new(); diff --git a/src/cargo/ops/cargo_compile.rs b/src/cargo/ops/cargo_compile.rs index 903f20138cd..173848e4d4c 100644 --- a/src/cargo/ops/cargo_compile.rs +++ b/src/cargo/ops/cargo_compile.rs @@ -313,8 +313,15 @@ pub fn compile_ws<'a>( 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, features, all_features, !no_default_features); - let resolve = - ops::resolve_ws_with_opts(ws, &target_data, build_config.requested_kind, &opts, &specs)?; + let has_dev_units = filter.need_dev_deps(build_config.mode); + let resolve = ops::resolve_ws_with_opts( + ws, + &target_data, + build_config.requested_kind, + &opts, + &specs, + has_dev_units, + )?; let WorkspaceResolve { mut pkg_set, workspace_resolve, diff --git a/src/cargo/ops/cargo_doc.rs b/src/cargo/ops/cargo_doc.rs index 7699fb33060..f932a11cac8 100644 --- a/src/cargo/ops/cargo_doc.rs +++ b/src/cargo/ops/cargo_doc.rs @@ -27,7 +27,8 @@ pub fn doc(ws: &Workspace<'_>, options: &DocOptions<'_>) -> CargoResult<()> { ); let requested_kind = options.compile_opts.build_config.requested_kind; let target_data = RustcTargetData::new(ws, requested_kind)?; - let ws_resolve = ops::resolve_ws_with_opts(ws, &target_data, requested_kind, &opts, &specs)?; + let ws_resolve = + ops::resolve_ws_with_opts(ws, &target_data, requested_kind, &opts, &specs, false)?; let ids = specs .iter() diff --git a/src/cargo/ops/cargo_install.rs b/src/cargo/ops/cargo_install.rs index 880dbbc6da9..394c5651286 100644 --- a/src/cargo/ops/cargo_install.rs +++ b/src/cargo/ops/cargo_install.rs @@ -506,6 +506,7 @@ fn check_yanked_install(ws: &Workspace<'_>) -> CargoResult<()> { CompileKind::Host, &ResolveOpts::everything(), &specs, + false, )?; let mut sources = ws_resolve.pkg_set.sources_mut(); diff --git a/src/cargo/ops/cargo_output_metadata.rs b/src/cargo/ops/cargo_output_metadata.rs index b7f36290f4f..d1f05f65dd3 100644 --- a/src/cargo/ops/cargo_output_metadata.rs +++ b/src/cargo/ops/cargo_output_metadata.rs @@ -119,8 +119,14 @@ fn build_resolve_graph( metadata_opts.all_features, !metadata_opts.no_default_features, ); - let ws_resolve = - ops::resolve_ws_with_opts(ws, &target_data, requested_kind, &resolve_opts, &specs)?; + let ws_resolve = ops::resolve_ws_with_opts( + ws, + &target_data, + requested_kind, + &resolve_opts, + &specs, + true, + )?; // Download all Packages. This is needed to serialize the information // for every package. In theory this could honor target filtering, // but that would be somewhat complex. diff --git a/src/cargo/ops/resolve.rs b/src/cargo/ops/resolve.rs index 1422f8ff44f..55f2e6df34d 100644 --- a/src/cargo/ops/resolve.rs +++ b/src/cargo/ops/resolve.rs @@ -77,6 +77,7 @@ pub fn resolve_ws_with_opts<'a>( requested_target: CompileKind, opts: &ResolveOpts, specs: &[PackageIdSpec], + has_dev_units: bool, ) -> CargoResult> { let mut registry = PackageRegistry::new(ws.config())?; let mut add_patches = true; @@ -131,6 +132,7 @@ pub fn resolve_ws_with_opts<'a>( &opts.features, specs, requested_target, + has_dev_units, )?; Ok(WorkspaceResolve { diff --git a/src/doc/src/reference/unstable.md b/src/doc/src/reference/unstable.md index 6848052713c..6c3cee83d8c 100644 --- a/src/doc/src/reference/unstable.md +++ b/src/doc/src/reference/unstable.md @@ -533,5 +533,9 @@ The available options are: `std` feature. However, when built as a test or example, it will include the `std` feature. + This mode is ignored if you are building any test, bench, or example. That + is, dev dependency features will still be unified if you run commands like + `cargo test` or `cargo build --all-targets`. + * `compare` — This option compares the resolved features to the old resolver, and will print any differences. diff --git a/tests/testsuite/features2.rs b/tests/testsuite/features2.rs index bb91a95204f..c3bbfa118db 100644 --- a/tests/testsuite/features2.rs +++ b/tests/testsuite/features2.rs @@ -308,6 +308,14 @@ fn decouple_dev_deps() { .file( "src/lib.rs", r#" + // const ensures it uses the correct dependency at *build time* + // compared to *link time*. + #[cfg(all(feature="f1", not(feature="f2")))] + pub const X: u32 = 1; + + #[cfg(all(feature="f1", feature="f2"))] + pub const X: u32 = 3; + pub fn foo() -> u32 { let mut res = 0; if cfg!(feature = "f1") { @@ -342,14 +350,19 @@ fn decouple_dev_deps() { "src/main.rs", r#" fn main() { - assert_eq!(foo::foo(), 1); - assert_eq!(common::foo(), 1); + let expected: u32 = std::env::args().skip(1).next().unwrap().parse().unwrap(); + assert_eq!(foo::foo(), expected); + assert_eq!(foo::build_time(), expected); + assert_eq!(common::foo(), expected); + assert_eq!(common::X, expected); } #[test] fn test_bin() { assert_eq!(foo::foo(), 3); assert_eq!(common::foo(), 3); + assert_eq!(common::X, 3); + assert_eq!(foo::build_time(), 3); } "#, ) @@ -360,10 +373,15 @@ fn decouple_dev_deps() { common::foo() } + pub fn build_time() -> u32 { + common::X + } + #[test] fn test_lib() { assert_eq!(foo(), 3); assert_eq!(common::foo(), 3); + assert_eq!(common::X, 3); } "#, ) @@ -374,17 +392,26 @@ fn decouple_dev_deps() { fn test_t1() { assert_eq!(foo::foo(), 3); assert_eq!(common::foo(), 3); + assert_eq!(common::X, 3); + assert_eq!(foo::build_time(), 3); + } + + #[test] + fn test_main() { + // Features are unified for main when run with `cargo test`, + // even with -Zfeatures=dev_dep. + let s = std::process::Command::new("target/debug/foo") + .arg("3") + .status().unwrap(); + assert!(s.success()); } "#, ) .build(); - p.cargo("run") - .with_status(101) - .with_stderr_contains("[..]assertion failed[..]") - .run(); + p.cargo("run 3").run(); - p.cargo("run -Zfeatures=dev_dep") + p.cargo("run -Zfeatures=dev_dep 1") .masquerade_as_nightly_cargo() .run(); From a2135fe1cf7789d6a99edfbb366aa574dffbd860 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Fri, 31 Jan 2020 14:17:42 -0800 Subject: [PATCH 05/22] Do not store `requested_features` in FeatureResolver. This isn't really needed "globally", just at the beginning. --- src/cargo/core/resolver/features.rs | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/src/cargo/core/resolver/features.rs b/src/cargo/core/resolver/features.rs index 9f78aa9c587..887b328e38f 100644 --- a/src/cargo/core/resolver/features.rs +++ b/src/cargo/core/resolver/features.rs @@ -174,8 +174,6 @@ pub struct FeatureResolver<'a, 'cfg> { /// The platform to build for, requested by the user. requested_target: CompileKind, resolve: &'a Resolve, - /// Features requested by the user on the command-line. - requested_features: &'a RequestedFeatures, /// Packages to build, requested on the command-line. specs: &'a [PackageIdSpec], /// Options that change how the feature resolver operates. @@ -216,13 +214,12 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> { target_data, requested_target, resolve, - requested_features, specs, opts, activated_features: HashMap::new(), processed_deps: HashSet::new(), }; - r.do_resolve()?; + r.do_resolve(requested_features)?; log::debug!("features={:#?}", r.activated_features); if r.opts.compare { r.compare(); @@ -235,14 +232,14 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> { } /// Performs the process of resolving all features for the resolve graph. - fn do_resolve(&mut self) -> CargoResult<()> { + fn do_resolve(&mut self, requested_features: &RequestedFeatures) -> CargoResult<()> { if self.opts.package_features { let mut found = false; for member in self.ws.members() { let member_id = member.package_id(); if self.specs.iter().any(|spec| spec.matches(member_id)) { found = true; - self.activate_member(member_id, self.requested_features)?; + self.activate_member(member_id, requested_features)?; } } if !found { @@ -259,7 +256,7 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> { Some(current) if member_id == current.package_id() => { // The "current" member gets activated with the flags // from the command line. - self.activate_member(member_id, self.requested_features)?; + self.activate_member(member_id, requested_features)?; } _ => { // Ignore members that are not enabled on the command-line. @@ -267,7 +264,7 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> { // -p for a workspace member that is not the // "current" one, don't use the local `--features`. let not_current_requested = - RequestedFeatures::new_all(self.requested_features.all_features); + RequestedFeatures::new_all(requested_features.all_features); self.activate_member(member_id, ¬_current_requested)?; } } From 3f06823095682a1058376aaecdc9e55be901cd8f Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Fri, 31 Jan 2020 14:21:51 -0800 Subject: [PATCH 06/22] Add some comments to help clarify some features stuff from review. --- src/cargo/core/compiler/unit_dependencies.rs | 7 +++++++ src/cargo/core/resolver/features.rs | 8 ++++++++ src/cargo/ops/cargo_compile.rs | 8 ++++++++ 3 files changed, 23 insertions(+) diff --git a/src/cargo/core/compiler/unit_dependencies.rs b/src/cargo/core/compiler/unit_dependencies.rs index ab9528d0868..dde9a284078 100644 --- a/src/cargo/core/compiler/unit_dependencies.rs +++ b/src/cargo/core/compiler/unit_dependencies.rs @@ -287,8 +287,15 @@ fn compute_deps<'a, 'cfg>( }; let mode = check_or_build_mode(unit.mode, lib); let dep_kind = if unit.target.is_custom_build() { + // Custom build scripts can *only* have build-dependencies. DepKind::Build } else if deps.iter().any(|dep| !dep.is_transitive()) { + // A dependency can be listed in both [dependencies] and + // [dev-dependencies], so this checks if any of the deps are + // listed in dev-dependencies. Note that `filtered_deps` has + // already removed dev-dependencies if it is not a + // test/bench/example, so it is not necessary to check `unit` + // here. DepKind::Development } else { DepKind::Normal diff --git a/src/cargo/core/resolver/features.rs b/src/cargo/core/resolver/features.rs index 887b328e38f..9108f91bc9c 100644 --- a/src/cargo/core/resolver/features.rs +++ b/src/cargo/core/resolver/features.rs @@ -25,6 +25,14 @@ use crate::util::{CargoResult, Config}; use std::collections::{BTreeSet, HashMap, HashSet}; use std::rc::Rc; +/// Map of activated features for a PackageId/DepKind/CompileKind. +/// +/// `DepKind` is needed, as the same package can be built multiple times with +/// different features. For example, with `decouple_build_deps`, a dependency +/// can be built once as a build dependency (for example with a 'std' +/// feature), and once as a normal dependency (without that 'std' feature). +/// +/// `CompileKind` is used currently not needed. type ActivateMap = HashMap<(PackageId, DepKind, CompileKind), BTreeSet>; /// Set of all activated features for all packages in the resolve graph. diff --git a/src/cargo/ops/cargo_compile.rs b/src/cargo/ops/cargo_compile.rs index 173848e4d4c..874b60963d4 100644 --- a/src/cargo/ops/cargo_compile.rs +++ b/src/cargo/ops/cargo_compile.rs @@ -746,6 +746,9 @@ fn generate_targets<'a>( bcx.profiles .get_profile(pkg.package_id(), ws.is_member(pkg), unit_for, target_mode); + // Root units are not a dependency of anything, so they are always + // DepKind::Normal. Their features are driven by the command-line + // arguments. let features = Vec::from(resolved_features.activated_features( pkg.package_id(), DepKind::Normal, @@ -934,6 +937,11 @@ fn generate_targets<'a>( Ok(units.into_iter().collect()) } +/// Gets all of the features enabled for a package, plus its dependencies' +/// features. +/// +/// Dependencies are added as `dep_name/feat_name` because `required-features` +/// wants to support that syntax. fn resolve_all_features( resolve_with_overrides: &Resolve, resolved_features: &features::ResolvedFeatures, From 2c8b9fb5ee7083b2c330e881e9cd1ca2ef6d68bd Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Sat, 1 Feb 2020 11:38:05 -0800 Subject: [PATCH 07/22] Add test for build script being run multiple times. --- tests/testsuite/features2.rs | 188 +++++++++++++++++++++++++++++++++++ 1 file changed, 188 insertions(+) diff --git a/tests/testsuite/features2.rs b/tests/testsuite/features2.rs index c3bbfa118db..a86a410fccd 100644 --- a/tests/testsuite/features2.rs +++ b/tests/testsuite/features2.rs @@ -421,3 +421,191 @@ fn decouple_dev_deps() { .masquerade_as_nightly_cargo() .run(); } + +#[cargo_test] +fn build_script_runtime_features() { + // Check that the CARGO_FEATURE_* environment variable is set correctly. + // + // This has a common dependency between build/normal/dev-deps, and it + // queries which features it was built with in different circumstances. + Package::new("common", "1.0.0") + .feature("normal", &[]) + .feature("dev", &[]) + .feature("build", &[]) + .file( + "build.rs", + r#" + fn is_set(name: &str) -> bool { + std::env::var(name) == Ok("1".to_string()) + } + + fn main() { + let mut res = 0; + if is_set("CARGO_FEATURE_NORMAL") { + res |= 1; + } + if is_set("CARGO_FEATURE_DEV") { + res |= 2; + } + if is_set("CARGO_FEATURE_BUILD") { + res |= 4; + } + println!("cargo:rustc-cfg=RunCustomBuild=\"{}\"", res); + + let mut res = 0; + if cfg!(feature = "normal") { + res |= 1; + } + if cfg!(feature = "dev") { + res |= 2; + } + if cfg!(feature = "build") { + res |= 4; + } + println!("cargo:rustc-cfg=CustomBuild=\"{}\"", res); + } + "#, + ) + .file( + "src/lib.rs", + r#" + pub fn foo() -> u32 { + let mut res = 0; + if cfg!(feature = "normal") { + res |= 1; + } + if cfg!(feature = "dev") { + res |= 2; + } + if cfg!(feature = "build") { + res |= 4; + } + res + } + + pub fn build_time() -> u32 { + #[cfg(RunCustomBuild="1")] return 1; + #[cfg(RunCustomBuild="3")] return 3; + #[cfg(RunCustomBuild="4")] return 4; + #[cfg(RunCustomBuild="5")] return 5; + #[cfg(RunCustomBuild="7")] return 7; + } + "#, + ) + .publish(); + + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + edition = "2018" + + [build-dependencies] + common = {version="1.0", features=["build"]} + + [dependencies] + common = {version="1.0", features=["normal"]} + + [dev-dependencies] + common = {version="1.0", features=["dev"]} + "#, + ) + .file( + "build.rs", + r#" + fn main() { + assert_eq!(common::foo(), common::build_time()); + println!("cargo:rustc-cfg=from_build=\"{}\"", common::foo()); + } + "#, + ) + .file( + "src/lib.rs", + r#" + pub fn foo() -> u32 { + common::foo() + } + + pub fn build_time() -> u32 { + common::build_time() + } + + #[test] + fn test_lib() { + assert_eq!(common::foo(), common::build_time()); + assert_eq!(common::foo(), + std::env::var("CARGO_FEATURE_EXPECT").unwrap().parse().unwrap()); + } + "#, + ) + .file( + "src/main.rs", + r#" + fn main() { + assert_eq!(common::foo(), common::build_time()); + assert_eq!(common::foo(), + std::env::var("CARGO_FEATURE_EXPECT").unwrap().parse().unwrap()); + } + + #[test] + fn test_bin() { + assert_eq!(common::foo(), common::build_time()); + assert_eq!(common::foo(), + std::env::var("CARGO_FEATURE_EXPECT").unwrap().parse().unwrap()); + } + "#, + ) + .file( + "tests/t1.rs", + r#" + #[test] + fn test_t1() { + assert_eq!(common::foo(), common::build_time()); + assert_eq!(common::foo(), + std::env::var("CARGO_FEATURE_EXPECT").unwrap().parse().unwrap()); + } + + #[test] + fn test_main() { + // Features are unified for main when run with `cargo test`, + // even with -Zfeatures=dev_dep. + let s = std::process::Command::new("target/debug/foo") + .status().unwrap(); + assert!(s.success()); + } + "#, + ) + .build(); + + // Old way, unifies all 3. + p.cargo("run").env("CARGO_FEATURE_EXPECT", "7").run(); + + // normal + build unify + p.cargo("run -Zfeatures=dev_dep") + .env("CARGO_FEATURE_EXPECT", "5") + .masquerade_as_nightly_cargo() + .run(); + + // Normal only. + p.cargo("run -Zfeatures=dev_dep,build_dep") + .env("CARGO_FEATURE_EXPECT", "1") + .masquerade_as_nightly_cargo() + .run(); + + p.cargo("test").env("CARGO_FEATURE_EXPECT", "7").run(); + + // dev_deps are still unified with `cargo test` + p.cargo("test -Zfeatures=dev_dep") + .env("CARGO_FEATURE_EXPECT", "7") + .masquerade_as_nightly_cargo() + .run(); + + // normal + dev unify + p.cargo("test -Zfeatures=build_dep") + .env("CARGO_FEATURE_EXPECT", "3") + .masquerade_as_nightly_cargo() + .run(); +} From 76f0d68de9855aeb65e37f565dc8ec323574ca12 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Sat, 1 Feb 2020 13:32:25 -0800 Subject: [PATCH 08/22] Consolidate -Zpackage-features member iteration logic. --- src/cargo/core/resolver/features.rs | 51 +++------------- src/cargo/core/workspace.rs | 81 ++++++++++++++++++++++++ src/cargo/ops/resolve.rs | 95 +++++------------------------ 3 files changed, 105 insertions(+), 122 deletions(-) diff --git a/src/cargo/core/resolver/features.rs b/src/cargo/core/resolver/features.rs index 9108f91bc9c..b6f8da85586 100644 --- a/src/cargo/core/resolver/features.rs +++ b/src/cargo/core/resolver/features.rs @@ -182,8 +182,6 @@ pub struct FeatureResolver<'a, 'cfg> { /// The platform to build for, requested by the user. requested_target: CompileKind, resolve: &'a Resolve, - /// Packages to build, requested on the command-line. - specs: &'a [PackageIdSpec], /// Options that change how the feature resolver operates. opts: FeatureOpts, /// Map of features activated for each package. @@ -222,12 +220,11 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> { target_data, requested_target, resolve, - specs, opts, activated_features: HashMap::new(), processed_deps: HashSet::new(), }; - r.do_resolve(requested_features)?; + r.do_resolve(specs, requested_features)?; log::debug!("features={:#?}", r.activated_features); if r.opts.compare { r.compare(); @@ -240,44 +237,14 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> { } /// Performs the process of resolving all features for the resolve graph. - fn do_resolve(&mut self, requested_features: &RequestedFeatures) -> CargoResult<()> { - if self.opts.package_features { - let mut found = false; - for member in self.ws.members() { - let member_id = member.package_id(); - if self.specs.iter().any(|spec| spec.matches(member_id)) { - found = true; - self.activate_member(member_id, requested_features)?; - } - } - if !found { - // -p for a non-member. Just resolve all with defaults. - let default = RequestedFeatures::new_all(false); - for member in self.ws.members() { - self.activate_member(member.package_id(), &default)?; - } - } - } else { - for member in self.ws.members() { - let member_id = member.package_id(); - match self.ws.current_opt() { - Some(current) if member_id == current.package_id() => { - // The "current" member gets activated with the flags - // from the command line. - self.activate_member(member_id, requested_features)?; - } - _ => { - // Ignore members that are not enabled on the command-line. - if self.specs.iter().any(|spec| spec.matches(member_id)) { - // -p for a workspace member that is not the - // "current" one, don't use the local `--features`. - let not_current_requested = - RequestedFeatures::new_all(requested_features.all_features); - self.activate_member(member_id, ¬_current_requested)?; - } - } - } - } + fn do_resolve( + &mut self, + specs: &[PackageIdSpec], + requested_features: &RequestedFeatures, + ) -> CargoResult<()> { + let member_features = self.ws.members_with_features(specs, requested_features)?; + for (member, requested_features) in member_features { + self.activate_member(member.package_id(), &requested_features)?; } Ok(()) } diff --git a/src/cargo/core/workspace.rs b/src/cargo/core/workspace.rs index c2cb23a8fa8..cd818302588 100644 --- a/src/cargo/core/workspace.rs +++ b/src/cargo/core/workspace.rs @@ -10,6 +10,7 @@ use url::Url; use crate::core::features::Features; use crate::core::registry::PackageRegistry; +use crate::core::resolver::features::RequestedFeatures; use crate::core::{Dependency, PackageId, PackageIdSpec}; use crate::core::{EitherManifest, Package, SourceId, VirtualManifest}; use crate::ops; @@ -833,6 +834,86 @@ impl<'cfg> Workspace<'cfg> { pub fn set_target_dir(&mut self, target_dir: Filesystem) { self.target_dir = Some(target_dir); } + + /// Returns a Vec of `(&Package, RequestedFeatures)` tuples that + /// represent the workspace members that were requested on the command-line. + /// + /// `specs` may be empty, which indicates it should return all workspace + /// members. In this case, `requested_features.all_features` must be + /// `true`. This is used for generating `Cargo.lock`, which must include + /// all members with all features enabled. + pub fn members_with_features( + &self, + specs: &[PackageIdSpec], + requested_features: &RequestedFeatures, + ) -> CargoResult> { + assert!( + !specs.is_empty() || requested_features.all_features, + "no specs requires all_features" + ); + if specs.is_empty() { + // When resolving the entire workspace, resolve each member with + // all features enabled. + return Ok(self + .members() + .map(|m| (m, RequestedFeatures::new_all(true))) + .collect()); + } + if self.config().cli_unstable().package_features { + if specs.len() > 1 && !requested_features.features.is_empty() { + anyhow::bail!("cannot specify features for more than one package"); + } + let members: Vec<(&Package, RequestedFeatures)> = self + .members() + .filter(|m| specs.iter().any(|spec| spec.matches(m.package_id()))) + .map(|m| (m, requested_features.clone())) + .collect(); + 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) + { + anyhow::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))) + .collect()); + } + return Ok(members); + } else { + let ms = self.members().filter_map(|member| { + let member_id = member.package_id(); + match self.current_opt() { + // 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() => { + Some((member, requested_features.clone())) + } + _ => { + // Ignore members that are not enabled on the command-line. + if specs.iter().any(|spec| spec.matches(member_id)) { + // -p for a workspace member that is not the + // "current" one, don't use the local + // `--features`, only allow `--all-features`. + Some(( + member, + RequestedFeatures::new_all(requested_features.all_features), + )) + } else { + // This member was not requested on the command-line, skip. + None + } + } + } + }); + return Ok(ms.collect()); + } + } } impl<'cfg> Packages<'cfg> { diff --git a/src/cargo/ops/resolve.rs b/src/cargo/ops/resolve.rs index 55f2e6df34d..905efee8436 100644 --- a/src/cargo/ops/resolve.rs +++ b/src/cargo/ops/resolve.rs @@ -12,8 +12,9 @@ use crate::core::compiler::{CompileKind, RustcTargetData}; use crate::core::registry::PackageRegistry; -use crate::core::resolver::features::{FeatureResolver, RequestedFeatures, ResolvedFeatures}; +use crate::core::resolver::features::{FeatureResolver, ResolvedFeatures}; use crate::core::resolver::{self, Resolve, ResolveOpts}; +use crate::core::summary::Summary; use crate::core::Feature; use crate::core::{PackageId, PackageIdSpec, PackageSet, Source, SourceId, Workspace}; use crate::ops; @@ -188,11 +189,6 @@ pub fn resolve_with_previous<'cfg>( specs: &[PackageIdSpec], register_patches: bool, ) -> CargoResult { - assert!( - !specs.is_empty() || opts.features.all_features, - "no specs requires all_features" - ); - // We only want one Cargo at a time resolving a crate graph since this can // involve a lot of frobbing of the global caches. let _lock = ws.config().acquire_package_cache_lock()?; @@ -272,81 +268,20 @@ pub fn resolve_with_previous<'cfg>( registry.add_sources(Some(member.package_id().source_id()))?; } - let mut summaries = Vec::new(); - if ws.config().cli_unstable().package_features { - let mut members = Vec::new(); - if specs.is_empty() { - members.extend(ws.members()); - } else { - if specs.len() > 1 && !opts.features.features.is_empty() { - anyhow::bail!("cannot specify features for more than one package"); - } - members.extend( - ws.members() - .filter(|m| specs.iter().any(|spec| spec.matches(m.package_id()))), - ); - // Edge case: running `cargo build -p foo`, where `foo` is not a member - // of current workspace. Add all packages from workspace to get `foo` - // into the resolution graph. - if members.is_empty() { - if !(opts.features.features.is_empty() - && !opts.features.all_features - && opts.features.uses_default_features) - { - anyhow::bail!("cannot specify features for packages outside of workspace"); - } - members.extend(ws.members()); - } - } - for member in members { + let summaries: Vec<(Summary, ResolveOpts)> = ws + .members_with_features(specs, &opts.features)? + .into_iter() + .map(|(member, features)| { let summary = registry.lock(member.summary().clone()); - summaries.push((summary, opts.clone())) - } - } else { - for member in ws.members() { - let summary_resolve_opts = if specs.is_empty() { - // When resolving the entire workspace, resolve each member - // with all features enabled. - opts.clone() - } else { - // If we're not resolving everything though then we're constructing the - // exact crate graph we're going to build. Here we don't necessarily - // want to keep around all workspace crates as they may not all be - // built/tested. - // - // Additionally, the `opts` specified represents command line - // flags, which really only matters for the current package - // (determined by the cwd). If other packages are specified (via - // `-p`) then the command line flags like features don't apply to - // them. - // - // As a result, if this `member` is the current member of the - // workspace, then we use `opts` specified. Otherwise we use a - // base `opts` with no features specified but using default features - // for any other packages specified with `-p`. - let member_id = member.package_id(); - match ws.current_opt() { - Some(current) if member_id == current.package_id() => opts.clone(), - _ => { - if specs.iter().any(|spec| spec.matches(member_id)) { - // -p for a workspace member that is not the - // "current" one, don't use the local `--features`. - ResolveOpts { - dev_deps: opts.dev_deps, - features: RequestedFeatures::new_all(opts.features.all_features), - } - } else { - // This member was not requested on the command-line, skip. - continue; - } - } - } - }; - - let summary = registry.lock(member.summary().clone()); - summaries.push((summary, summary_resolve_opts)); - } - }; + ( + summary, + ResolveOpts { + dev_deps: opts.dev_deps, + features, + }, + ) + }) + .collect(); let root_replace = ws.root_replace(); From 899067fbec52cf8507250c23920c84fce3590e08 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Sat, 1 Feb 2020 14:33:51 -0800 Subject: [PATCH 09/22] Add a cyclical dev-dep test. --- tests/testsuite/features2.rs | 74 ++++++++++++++++++++++++++++++++++++ 1 file changed, 74 insertions(+) diff --git a/tests/testsuite/features2.rs b/tests/testsuite/features2.rs index a86a410fccd..fe0ebd38e33 100644 --- a/tests/testsuite/features2.rs +++ b/tests/testsuite/features2.rs @@ -609,3 +609,77 @@ fn build_script_runtime_features() { .masquerade_as_nightly_cargo() .run(); } + +#[cargo_test] +fn cyclical_dev_dep() { + // Check how a cyclical dev-dependency will work. + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + edition = "2018" + + [features] + dev = [] + + [dev-dependencies] + foo = { path = '.', features = ["dev"] } + "#, + ) + .file( + "src/lib.rs", + r#" + pub fn assert_dev(enabled: bool) { + assert_eq!(enabled, cfg!(feature="dev")); + } + + #[test] + fn test_in_lib() { + assert_dev(true); + } + "#, + ) + .file( + "src/main.rs", + r#" + fn main() { + let expected: bool = std::env::args().skip(1).next().unwrap().parse().unwrap(); + foo::assert_dev(expected); + } + "#, + ) + .file( + "tests/t1.rs", + r#" + #[test] + fn integration_links() { + foo::assert_dev(true); + // The lib linked with main.rs will also be unified. + let s = std::process::Command::new("target/debug/foo") + .arg("true") + .status().unwrap(); + assert!(s.success()); + } + "#, + ) + .build(); + + // Old way unifies features. + p.cargo("run true").run(); + + // Should decouple main. + p.cargo("run -Zfeatures=dev_dep false") + .masquerade_as_nightly_cargo() + .run(); + + // dev feature should always be enabled in tests. + p.cargo("test").run(); + + // And this should be no different. + p.cargo("test -Zfeatures=dev_dep") + .masquerade_as_nightly_cargo() + .run(); +} From da678a75d5806081799cbb7350ef5ffbb8b26920 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Sat, 1 Feb 2020 14:57:28 -0800 Subject: [PATCH 10/22] Add `-Zfeatures=all` option. --- src/cargo/core/resolver/features.rs | 5 ++ src/doc/src/reference/unstable.md | 2 + tests/testsuite/features2.rs | 82 +++++++++++++++++++++++++++++ 3 files changed, 89 insertions(+) diff --git a/src/cargo/core/resolver/features.rs b/src/cargo/core/resolver/features.rs index b6f8da85586..deeb77590b0 100644 --- a/src/cargo/core/resolver/features.rs +++ b/src/cargo/core/resolver/features.rs @@ -72,6 +72,11 @@ impl FeatureOpts { "build_dep" => opts.decouple_build_deps = true, "dev_dep" => opts.decouple_dev_deps = true, "itarget" => opts.ignore_inactive_targets = true, + "all" => { + opts.decouple_build_deps = true; + opts.decouple_dev_deps = true; + opts.ignore_inactive_targets = true; + } "compare" => opts.compare = true, "ws" => unimplemented!(), "host" => unimplemented!(), diff --git a/src/doc/src/reference/unstable.md b/src/doc/src/reference/unstable.md index 6c3cee83d8c..48b32afedc1 100644 --- a/src/doc/src/reference/unstable.md +++ b/src/doc/src/reference/unstable.md @@ -537,5 +537,7 @@ The available options are: is, dev dependency features will still be unified if you run commands like `cargo test` or `cargo build --all-targets`. +* `all` — Enable all feature options (`itarget,build_dep,dev_dep`). + * `compare` — This option compares the resolved features to the old resolver, and will print any differences. diff --git a/tests/testsuite/features2.rs b/tests/testsuite/features2.rs index fe0ebd38e33..55b2baec42f 100644 --- a/tests/testsuite/features2.rs +++ b/tests/testsuite/features2.rs @@ -683,3 +683,85 @@ fn cyclical_dev_dep() { .masquerade_as_nightly_cargo() .run(); } + +#[cargo_test] +fn all_feature_opts() { + // All feature options at once. + Package::new("common", "1.0.0") + .feature("normal", &[]) + .feature("build", &[]) + .feature("dev", &[]) + .feature("itarget", &[]) + .file( + "src/lib.rs", + r#" + pub fn feats() -> u32 { + let mut res = 0; + if cfg!(feature="normal") { res |= 1; } + if cfg!(feature="build") { res |= 2; } + if cfg!(feature="dev") { res |= 4; } + if cfg!(feature="itarget") { res |= 8; } + res + } + "#, + ) + .publish(); + + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + edition = "2018" + + [dependencies] + common = {version = "1.0", features=["normal"]} + + [dev-dependencies] + common = {version = "1.0", features=["dev"]} + + [build-dependencies] + common = {version = "1.0", features=["build"]} + + [target.'cfg(whatever)'.dependencies] + common = {version = "1.0", features=["itarget"]} + "#, + ) + .file( + "src/main.rs", + r#" + fn main() { + expect(); + } + + fn expect() { + let expected: u32 = std::env::var("EXPECTED_FEATS").unwrap().parse().unwrap(); + assert_eq!(expected, common::feats()); + } + + #[test] + fn from_test() { + expect(); + } + "#, + ) + .build(); + + p.cargo("run").env("EXPECTED_FEATS", "15").run(); + + // Only normal feature. + p.cargo("run -Zfeatures=all") + .masquerade_as_nightly_cargo() + .env("EXPECTED_FEATS", "1") + .run(); + + p.cargo("test").env("EXPECTED_FEATS", "15").run(); + + // only normal+dev + p.cargo("test -Zfeatures=all") + .masquerade_as_nightly_cargo() + .env("EXPECTED_FEATS", "5") + .run(); +} From 134aeff1bfd3e07c7da869717ede6e3eb4711c5c Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Sat, 1 Feb 2020 15:20:24 -0800 Subject: [PATCH 11/22] Add comment explaining dep_kind in compute_deps_custom_build. --- src/cargo/core/compiler/unit_dependencies.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/cargo/core/compiler/unit_dependencies.rs b/src/cargo/core/compiler/unit_dependencies.rs index dde9a284078..9ea46bc1f08 100644 --- a/src/cargo/core/compiler/unit_dependencies.rs +++ b/src/cargo/core/compiler/unit_dependencies.rs @@ -382,6 +382,11 @@ fn compute_deps<'a, 'cfg>( /// /// The `unit` provided must represent an execution of a build script, and /// the returned set of units must all be run before `unit` is run. +/// +/// `dep_kind` is the dependency kind of the package this build script is +/// being built for. This ensures that the build script is built with the same +/// features the package is built with (if the build script has cfg!() +/// checks). fn compute_deps_custom_build<'a, 'cfg>( unit: &Unit<'a>, dep_kind: DepKind, From 615464518639de308153a2e60b4a6d4801f87034 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Sun, 2 Feb 2020 11:54:29 -0800 Subject: [PATCH 12/22] Rewrite new feature resolver to simplify DepKind handling. Now that dev-dependencies are a global setting, only build-dependencies matter. This is maybe a little simpler to understand? --- .../compiler/context/compilation_files.rs | 4 +- src/cargo/core/compiler/standard_lib.rs | 7 +- src/cargo/core/compiler/unit_dependencies.rs | 112 ++++------- src/cargo/core/dependency.rs | 17 -- src/cargo/core/profiles.rs | 104 ++++++---- src/cargo/core/resolver/features.rs | 186 +++++++----------- src/cargo/ops/cargo_clean.rs | 13 +- src/cargo/ops/cargo_compile.rs | 21 +- src/cargo/ops/cargo_output_metadata.rs | 2 +- src/cargo/ops/resolve.rs | 2 +- tests/testsuite/profile_config.rs | 2 +- 11 files changed, 187 insertions(+), 283 deletions(-) diff --git a/src/cargo/core/compiler/context/compilation_files.rs b/src/cargo/core/compiler/context/compilation_files.rs index b9a08bc706f..313935d8e3d 100644 --- a/src/cargo/core/compiler/context/compilation_files.rs +++ b/src/cargo/core/compiler/context/compilation_files.rs @@ -147,7 +147,7 @@ impl<'a, 'cfg: 'a> CompilationFiles<'a, 'cfg> { /// Returns `None` if the unit should not use a metadata data hash (like /// rustdoc, or some dylibs). pub fn metadata(&self, unit: &Unit<'a>) -> Option { - self.metas[unit].clone() + self.metas[unit] } /// Gets the short hash based only on the `PackageId`. @@ -515,7 +515,7 @@ fn metadata_of<'a, 'cfg>( metadata_of(&dep.unit, cx, metas); } } - metas[unit].clone() + metas[unit] } fn compute_metadata<'a, 'cfg>( diff --git a/src/cargo/core/compiler/standard_lib.rs b/src/cargo/core/compiler/standard_lib.rs index f4bd0345c89..80048f9780d 100644 --- a/src/cargo/core/compiler/standard_lib.rs +++ b/src/cargo/core/compiler/standard_lib.rs @@ -1,7 +1,6 @@ //! Code for building the standard library. use crate::core::compiler::{BuildContext, CompileKind, CompileMode, RustcTargetData, Unit}; -use crate::core::dependency::DepKind; use crate::core::profiles::UnitFor; use crate::core::resolver::features::ResolvedFeatures; use crate::core::resolver::ResolveOpts; @@ -149,11 +148,7 @@ pub fn generate_std_roots<'a>( unit_for, mode, ); - let features = std_features.activated_features( - pkg.package_id(), - DepKind::Normal, - bcx.build_config.requested_kind, - ); + let features = std_features.activated_features(pkg.package_id(), false); Ok(bcx.units.intern( pkg, lib, profile, kind, mode, features, /*is_std*/ true, )) diff --git a/src/cargo/core/compiler/unit_dependencies.rs b/src/cargo/core/compiler/unit_dependencies.rs index 9ea46bc1f08..b84ff7e7098 100644 --- a/src/cargo/core/compiler/unit_dependencies.rs +++ b/src/cargo/core/compiler/unit_dependencies.rs @@ -178,7 +178,7 @@ fn deps_of_roots<'a, 'cfg>(roots: &[Unit<'a>], mut state: &mut State<'a, 'cfg>) } else if unit.target.is_custom_build() { // This normally doesn't happen, except `clean` aggressively // generates all units. - UnitFor::new_build() + UnitFor::new_build(false) } else if unit.target.for_host() { // Proc macro / plugin should never have panic set. UnitFor::new_compiler() @@ -230,7 +230,7 @@ fn compute_deps<'a, 'cfg>( unit_for: UnitFor, ) -> CargoResult>> { if unit.mode.is_run_custom_build() { - return compute_deps_custom_build(unit, unit_for.dep_kind(), state); + return compute_deps_custom_build(unit, unit_for, state); } else if unit.mode.is_doc() { // Note: this does not include doc test. return compute_deps_doc(unit, state); @@ -238,45 +238,40 @@ fn compute_deps<'a, 'cfg>( let bcx = state.bcx; let id = unit.pkg.package_id(); - let filtered_deps = state - .resolve() - .deps(id) - .map(|(id, deps)| { - assert!(!deps.is_empty()); - let filtered_deps = deps.iter().filter(|dep| { - // If this target is a build command, then we only want build - // dependencies, otherwise we want everything *other than* build - // dependencies. - if unit.target.is_custom_build() != dep.is_build() { - return false; - } + let filtered_deps = state.resolve().deps(id).filter(|&(_id, deps)| { + assert!(!deps.is_empty()); + deps.iter().any(|dep| { + // If this target is a build command, then we only want build + // dependencies, otherwise we want everything *other than* build + // dependencies. + if unit.target.is_custom_build() != dep.is_build() { + return false; + } - // If this dependency is **not** a transitive dependency, then it - // only applies to test/example targets. - if !dep.is_transitive() - && !unit.target.is_test() - && !unit.target.is_example() - && !unit.mode.is_any_test() - { - return false; - } + // If this dependency is **not** a transitive dependency, then it + // only applies to test/example targets. + if !dep.is_transitive() + && !unit.target.is_test() + && !unit.target.is_example() + && !unit.mode.is_any_test() + { + return false; + } - // If this dependency is only available for certain platforms, - // make sure we're only enabling it for that platform. - if !bcx.target_data.dep_platform_activated(dep, unit.kind) { - return false; - } + // If this dependency is only available for certain platforms, + // make sure we're only enabling it for that platform. + if !bcx.target_data.dep_platform_activated(dep, unit.kind) { + return false; + } - // If we've gotten past all that, then this dependency is - // actually used! - true - }); - (id, filtered_deps.collect::>()) + // If we've gotten past all that, then this dependency is + // actually used! + true }) - .filter(|(_id, deps)| !deps.is_empty()); + }); let mut ret = Vec::new(); - for (id, deps) in filtered_deps { + for (id, _) in filtered_deps { let pkg = match state.get(id)? { Some(pkg) => pkg, None => continue, @@ -286,23 +281,10 @@ fn compute_deps<'a, 'cfg>( None => continue, }; let mode = check_or_build_mode(unit.mode, lib); - let dep_kind = if unit.target.is_custom_build() { - // Custom build scripts can *only* have build-dependencies. - DepKind::Build - } else if deps.iter().any(|dep| !dep.is_transitive()) { - // A dependency can be listed in both [dependencies] and - // [dev-dependencies], so this checks if any of the deps are - // listed in dev-dependencies. Note that `filtered_deps` has - // already removed dev-dependencies if it is not a - // test/bench/example, so it is not necessary to check `unit` - // here. - DepKind::Development - } else { - DepKind::Normal - }; let dep_unit_for = unit_for .with_for_host(lib.for_host()) - .with_dep_kind(dep_kind); + // If it is a custom build script, then it *only* has build dependencies. + .with_build_dep(unit.target.is_custom_build()); if bcx.config.cli_unstable().dual_proc_macros && lib.proc_macro() && !unit.kind.is_host() { let unit_dep = new_unit_dep(state, unit, pkg, lib, dep_unit_for, unit.kind, mode)?; @@ -330,7 +312,7 @@ fn compute_deps<'a, 'cfg>( if unit.target.is_custom_build() { return Ok(ret); } - ret.extend(dep_build_script(unit, unit_for.dep_kind(), state)?); + ret.extend(dep_build_script(unit, unit_for, state)?); // If this target is a binary, test, example, etc, then it depends on // the library of the same package. The call to `resolve.deps` above @@ -382,14 +364,9 @@ fn compute_deps<'a, 'cfg>( /// /// The `unit` provided must represent an execution of a build script, and /// the returned set of units must all be run before `unit` is run. -/// -/// `dep_kind` is the dependency kind of the package this build script is -/// being built for. This ensures that the build script is built with the same -/// features the package is built with (if the build script has cfg!() -/// checks). fn compute_deps_custom_build<'a, 'cfg>( unit: &Unit<'a>, - dep_kind: DepKind, + unit_for: UnitFor, state: &mut State<'a, 'cfg>, ) -> CargoResult>> { if let Some(links) = unit.pkg.manifest().links() { @@ -400,7 +377,7 @@ fn compute_deps_custom_build<'a, 'cfg>( } // All dependencies of this unit should use profiles for custom // builds. - let unit_for = UnitFor::new_build().with_dep_kind(dep_kind); + let script_unit_for = UnitFor::new_build(unit_for.is_for_build_dep()); // When not overridden, then the dependencies to run a build script are: // // 1. Compiling the build script itself. @@ -415,7 +392,7 @@ fn compute_deps_custom_build<'a, 'cfg>( unit, unit.pkg, unit.target, - unit_for, + script_unit_for, // Build scripts always compiled for the host. CompileKind::Host, CompileMode::Build, @@ -482,7 +459,7 @@ fn compute_deps_doc<'a, 'cfg>( } // Be sure to build/run the build script for documented libraries. - ret.extend(dep_build_script(unit, DepKind::Normal, state)?); + ret.extend(dep_build_script(unit, UnitFor::new_normal(), state)?); // If we document a binary/example, we need the library available. if unit.target.is_bin() || unit.target.is_example() { @@ -524,7 +501,7 @@ fn maybe_lib<'a>( /// build script. fn dep_build_script<'a>( unit: &Unit<'a>, - dep_kind: DepKind, + unit_for: UnitFor, state: &State<'a, '_>, ) -> CargoResult>> { unit.pkg @@ -538,7 +515,7 @@ fn dep_build_script<'a>( .bcx .profiles .get_profile_run_custom_build(&unit.profile); - let script_unit_for = UnitFor::new_build().with_dep_kind(dep_kind); + let script_unit_for = UnitFor::new_build(unit_for.is_for_build_dep()); new_unit_dep_with_profile( state, unit, @@ -609,7 +586,7 @@ fn new_unit_dep_with_profile<'a>( let public = state .resolve() .is_public_dep(parent.pkg.package_id(), pkg.package_id()); - let features = state.activated_features(pkg.package_id(), unit_for.dep_kind(), kind); + let features = state.activated_features(pkg.package_id(), unit_for.is_for_build_dep()); let unit = state .bcx .units @@ -714,18 +691,13 @@ impl<'a, 'cfg> State<'a, 'cfg> { } } - fn activated_features( - &self, - pkg_id: PackageId, - dep_kind: DepKind, - compile_kind: CompileKind, - ) -> Vec { + fn activated_features(&self, pkg_id: PackageId, for_build_dep: bool) -> Vec { let features = if self.is_std { self.std_features.unwrap() } else { self.usr_features }; - features.activated_features(pkg_id, dep_kind, compile_kind) + features.activated_features(pkg_id, for_build_dep) } fn get(&mut self, id: PackageId) -> CargoResult> { diff --git a/src/cargo/core/dependency.rs b/src/cargo/core/dependency.rs index 5eff3cd505b..82da0612925 100644 --- a/src/cargo/core/dependency.rs +++ b/src/cargo/core/dependency.rs @@ -92,23 +92,6 @@ pub enum DepKind { Build, } -impl DepKind { - /// Convert a DepKind from a package to one of its dependencies. - /// - /// The rules here determine how feature decoupling works. This works in - /// conjunction with the new feature resolver. - pub fn sticky_kind(&self, to: DepKind) -> DepKind { - use DepKind::*; - match (self, to) { - (Normal, _) => to, - (Build, _) => Build, - (Development, Normal) => Development, - (Development, Build) => Build, - (Development, Development) => Development, - } - } -} - fn parse_req_with_deprecated( name: InternedString, req: &str, diff --git a/src/cargo/core/profiles.rs b/src/cargo/core/profiles.rs index 9bdfafeb83f..9c079a7cdd3 100644 --- a/src/cargo/core/profiles.rs +++ b/src/cargo/core/profiles.rs @@ -1,5 +1,4 @@ use crate::core::compiler::CompileMode; -use crate::core::dependency::DepKind; use crate::core::interning::InternedString; use crate::core::{Feature, Features, PackageId, PackageIdSpec, Resolve, Shell}; use crate::util::errors::CargoResultExt; @@ -484,7 +483,7 @@ fn merge_toml_overrides( profile: &mut Profile, toml: &TomlProfile, ) { - if unit_for.is_build() { + if unit_for.is_for_host() { if let Some(ref build_override) = toml.build_override { merge_profile(profile, build_override); } @@ -767,9 +766,17 @@ pub struct UnitFor { /// A target for `build.rs` or any of its dependencies, or a proc-macro or /// any of its dependencies. This enables `build-override` profiles for /// these targets. - build: bool, - /// The dependency kind (normal, dev, build). - dep_kind: DepKind, + host: bool, + /// A target for a build dependency (or any of its dependencies). This is + /// used for computing features of build dependencies independently of + /// other dependency kinds. + /// + /// The subtle difference between this and `host` is that the build script + /// for a non-host package sets this to `false` because it wants the + /// features of the non-host package (whereas `host` is true because the + /// build script is being built for the host). `build_dep` becomes `true` + /// for build-dependencies, or any of their dependencies. + build_dep: bool, /// How Cargo processes the `panic` setting or profiles. This is done to /// handle test/benches inheriting from dev/release, as well as forcing /// `for_host` units to always unwind. @@ -796,17 +803,22 @@ impl UnitFor { /// proc macro/plugin, or test/bench). pub fn new_normal() -> UnitFor { UnitFor { - build: false, - dep_kind: DepKind::Normal, + host: false, + build_dep: false, panic_setting: PanicSetting::ReadProfile, } } /// A unit for a custom build script or its dependencies. - pub fn new_build() -> UnitFor { + /// + /// The `build_dep` parameter is whether or not this is for a build + /// dependency. Build scripts for non-host units should use `false` + /// because they want to use the features of the package they are running + /// for. + pub fn new_build(build_dep: bool) -> UnitFor { UnitFor { - build: true, - dep_kind: DepKind::Normal, + host: true, + build_dep, // Force build scripts to always use `panic=unwind` for now to // maximally share dependencies with procedural macros. panic_setting: PanicSetting::AlwaysUnwind, @@ -816,8 +828,8 @@ impl UnitFor { /// A unit for a proc macro or compiler plugin or their dependencies. pub fn new_compiler() -> UnitFor { UnitFor { - build: false, - dep_kind: DepKind::Normal, + host: false, + build_dep: false, // Force plugins to use `panic=abort` so panics in the compiler do // not abort the process but instead end with a reasonable error // message that involves catching the panic in the compiler. @@ -825,7 +837,7 @@ impl UnitFor { } } - /// A unit for a test/bench target. + /// A unit for a test/bench target or their dependencies. /// /// Note that `config` is taken here for unstable CLI features to detect /// whether `panic=abort` is supported for tests. Historical versions of @@ -833,8 +845,8 @@ impl UnitFor { /// compiler flag. pub fn new_test(config: &Config) -> UnitFor { UnitFor { - build: false, - dep_kind: DepKind::Development, + host: false, + build_dep: false, // We're testing out an unstable feature (`-Zpanic-abort-tests`) // which inherits the panic setting from the dev/release profile // (basically avoid recompiles) but historical defaults required @@ -847,7 +859,7 @@ impl UnitFor { } } - /// Creates a variant based on `for_host` setting. + /// Returns a new copy based on `for_host` setting. /// /// When `for_host` is true, this clears `panic_abort_ok` in a sticky /// fashion so that all its dependencies also have `panic_abort_ok=false`. @@ -856,8 +868,8 @@ impl UnitFor { /// graph where everything is `panic=unwind`. pub fn with_for_host(self, for_host: bool) -> UnitFor { UnitFor { - build: self.build || for_host, - dep_kind: self.dep_kind, + host: self.host || for_host, + build_dep: self.build_dep, panic_setting: if for_host { PanicSetting::AlwaysUnwind } else { @@ -866,55 +878,65 @@ impl UnitFor { } } - /// Creates a variant for the given dependency kind. + /// Returns a new copy updating it for a build dependency. /// /// This is part of the machinery responsible for handling feature - /// decoupling in the new feature resolver. - pub fn with_dep_kind(mut self, kind: DepKind) -> UnitFor { - self.dep_kind = self.dep_kind.sticky_kind(kind); + /// decoupling for build dependencies in the new feature resolver. + pub fn with_build_dep(mut self, build_dep: bool) -> UnitFor { + self.build_dep = self.build_dep || build_dep; self } - /// Returns `true` if this unit is for a custom build script or one of its - /// dependencies. - pub fn is_build(self) -> bool { - self.build + /// Returns `true` if this unit is for a build script or any of its + /// dependencies, or a proc macro or any of its dependencies. + pub fn is_for_host(&self) -> bool { + self.host } - /// Returns how `panic` settings should be handled for this profile - fn panic_setting(self) -> PanicSetting { - self.panic_setting + pub fn is_for_build_dep(&self) -> bool { + self.build_dep } - /// Returns the dependency kind this unit is intended for. - pub fn dep_kind(&self) -> DepKind { - self.dep_kind + /// Returns how `panic` settings should be handled for this profile + fn panic_setting(&self) -> PanicSetting { + self.panic_setting } /// All possible values, used by `clean`. pub fn all_values() -> &'static [UnitFor] { - // dep_kind isn't needed for profiles, so its value doesn't matter. static ALL: &[UnitFor] = &[ UnitFor { - build: false, - dep_kind: DepKind::Normal, + host: false, + build_dep: false, panic_setting: PanicSetting::ReadProfile, }, UnitFor { - build: true, - dep_kind: DepKind::Normal, + host: true, + build_dep: false, panic_setting: PanicSetting::AlwaysUnwind, }, UnitFor { - build: false, - dep_kind: DepKind::Normal, + host: false, + build_dep: false, panic_setting: PanicSetting::AlwaysUnwind, }, UnitFor { - build: false, - dep_kind: DepKind::Normal, + host: false, + build_dep: false, panic_setting: PanicSetting::Inherit, }, + // build_dep=true must always have host=true + // `Inherit` is not used in build dependencies. + UnitFor { + host: true, + build_dep: true, + panic_setting: PanicSetting::ReadProfile, + }, + UnitFor { + host: true, + build_dep: true, + panic_setting: PanicSetting::AlwaysUnwind, + }, ]; ALL } diff --git a/src/cargo/core/resolver/features.rs b/src/cargo/core/resolver/features.rs index deeb77590b0..490ff58a670 100644 --- a/src/cargo/core/resolver/features.rs +++ b/src/cargo/core/resolver/features.rs @@ -25,15 +25,11 @@ use crate::util::{CargoResult, Config}; use std::collections::{BTreeSet, HashMap, HashSet}; use std::rc::Rc; -/// Map of activated features for a PackageId/DepKind/CompileKind. +/// Map of activated features. /// -/// `DepKind` is needed, as the same package can be built multiple times with -/// different features. For example, with `decouple_build_deps`, a dependency -/// can be built once as a build dependency (for example with a 'std' -/// feature), and once as a normal dependency (without that 'std' feature). -/// -/// `CompileKind` is used currently not needed. -type ActivateMap = HashMap<(PackageId, DepKind, CompileKind), BTreeSet>; +/// The key is `(PackageId, bool)` where the bool is `true` if these +/// are features for a build dependency. +type ActivateMap = HashMap<(PackageId, bool), BTreeSet>; /// Set of all activated features for all packages in the resolve graph. pub struct ResolvedFeatures { @@ -154,28 +150,16 @@ impl RequestedFeatures { impl ResolvedFeatures { /// Returns the list of features that are enabled for the given package. - pub fn activated_features( - &self, - pkg_id: PackageId, - dep_kind: DepKind, - compile_kind: CompileKind, - ) -> Vec { + pub fn activated_features(&self, pkg_id: PackageId, is_build: bool) -> Vec { if let Some(legacy) = &self.legacy { legacy.get(&pkg_id).map_or_else(Vec::new, |v| v.clone()) } else { - let dep_kind = if (!self.opts.decouple_build_deps && dep_kind == DepKind::Build) - || (!self.opts.decouple_dev_deps && dep_kind == DepKind::Development) - { - // Decoupling disabled, everything is unified under "Normal". - DepKind::Normal - } else { - dep_kind - }; + let is_build = self.opts.decouple_build_deps && is_build; // TODO: Remove panic, return empty set. let fs = self .activated_features - .get(&(pkg_id, dep_kind, compile_kind)) - .unwrap_or_else(|| panic!("features did not find {:?} {:?}", pkg_id, dep_kind)); + .get(&(pkg_id, is_build)) + .unwrap_or_else(|| panic!("features did not find {:?} {:?}", pkg_id, is_build)); fs.iter().cloned().collect() } } @@ -193,7 +177,7 @@ pub struct FeatureResolver<'a, 'cfg> { activated_features: ActivateMap, /// Keeps track of which packages have had its dependencies processed. /// Used to avoid cycles, and to speed up processing. - processed_deps: HashSet<(PackageId, DepKind, CompileKind)>, + processed_deps: HashSet<(PackageId, bool)>, } impl<'a, 'cfg> FeatureResolver<'a, 'cfg> { @@ -248,106 +232,72 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> { requested_features: &RequestedFeatures, ) -> CargoResult<()> { let member_features = self.ws.members_with_features(specs, requested_features)?; - for (member, requested_features) in member_features { - self.activate_member(member.package_id(), &requested_features)?; - } - Ok(()) - } - - /// Enable the given features on the given workspace member. - fn activate_member( - &mut self, - pkg_id: PackageId, - requested_features: &RequestedFeatures, - ) -> CargoResult<()> { - let fvs = self.fvs_from_requested(pkg_id, CompileKind::Host, requested_features); - self.activate_member_fvs(pkg_id, CompileKind::Host, &fvs)?; - if let CompileKind::Target(_) = self.requested_target { - let fvs = self.fvs_from_requested(pkg_id, self.requested_target, requested_features); - self.activate_member_fvs(pkg_id, self.requested_target, &fvs)?; - } - Ok(()) - } - - fn activate_member_fvs( - &mut self, - pkg_id: PackageId, - compile_kind: CompileKind, - fvs: &[FeatureValue], - ) -> CargoResult<()> { - self.activate_with_platform(pkg_id, DepKind::Normal, compile_kind, &fvs)?; - if self.opts.decouple_dev_deps { - // Activate the member as a dev dep, assuming it has at least one - // test, bench, or example. This ensures the member's normal deps get - // unified with its dev deps. - self.activate_with_platform(pkg_id, DepKind::Development, compile_kind, &fvs)?; + for (member, requested_features) in &member_features { + let fvs = self.fvs_from_requested(member.package_id(), requested_features); + self.activate_pkg(member.package_id(), &fvs, false)?; } Ok(()) } - fn activate_with_platform( + fn activate_pkg( &mut self, pkg_id: PackageId, - dep_kind: DepKind, - compile_kind: CompileKind, fvs: &[FeatureValue], + for_build: bool, ) -> CargoResult<()> { // Add an empty entry to ensure everything is covered. This is intended for // finding bugs where the resolver missed something it should have visited. // Remove this in the future if `activated_features` uses an empty default. self.activated_features - .entry((pkg_id, dep_kind, compile_kind)) + .entry((pkg_id, for_build)) .or_insert_with(BTreeSet::new); for fv in fvs { - self.activate_fv(pkg_id, dep_kind, compile_kind, fv)?; + self.activate_fv(pkg_id, fv, for_build)?; } - if !self.processed_deps.insert((pkg_id, dep_kind, compile_kind)) { + if !self.processed_deps.insert((pkg_id, for_build)) { // Already processed dependencies. return Ok(()); } - // Activate any of its dependencies. - for (dep_pkg_id, deps) in self.deps(pkg_id, compile_kind) { + for (dep_pkg_id, deps) in self.deps(pkg_id, for_build) { for dep in deps { if dep.is_optional() { continue; } // Recurse into the dependency. let fvs = self.fvs_from_dependency(dep_pkg_id, dep); - self.activate_with_platform( + self.activate_pkg( dep_pkg_id, - self.sticky_dep_kind(dep_kind, dep.kind()), - compile_kind, &fvs, + for_build || (self.opts.decouple_build_deps && dep.is_build()), )?; } } - return Ok(()); + Ok(()) } + /// Activate a single FeatureValue for a package. fn activate_fv( &mut self, pkg_id: PackageId, - dep_kind: DepKind, - compile_kind: CompileKind, fv: &FeatureValue, + for_build: bool, ) -> CargoResult<()> { match fv { FeatureValue::Feature(f) => { - self.activate_rec(pkg_id, dep_kind, compile_kind, *f)?; + self.activate_rec(pkg_id, *f, for_build)?; } FeatureValue::Crate(dep_name) => { // Activate the feature name on self. - self.activate_rec(pkg_id, dep_kind, compile_kind, *dep_name)?; + self.activate_rec(pkg_id, *dep_name, for_build)?; // Activate the optional dep. - for (dep_pkg_id, deps) in self.deps(pkg_id, compile_kind) { + for (dep_pkg_id, deps) in self.deps(pkg_id, for_build) { for dep in deps { if dep.name_in_toml() == *dep_name { let fvs = self.fvs_from_dependency(dep_pkg_id, dep); - self.activate_with_platform( + self.activate_pkg( dep_pkg_id, - self.sticky_dep_kind(dep_kind, dep.kind()), - compile_kind, &fvs, + for_build || (self.opts.decouple_build_deps && dep.is_build()), )?; } } @@ -355,22 +305,21 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> { } FeatureValue::CrateFeature(dep_name, dep_feature) => { // Activate a feature within a dependency. - for (dep_pkg_id, deps) in self.deps(pkg_id, compile_kind) { + for (dep_pkg_id, deps) in self.deps(pkg_id, for_build) { for dep in deps { if dep.name_in_toml() == *dep_name { if dep.is_optional() { // Activate the crate on self. let fv = FeatureValue::Crate(*dep_name); - self.activate_fv(pkg_id, dep_kind, compile_kind, &fv)?; + self.activate_fv(pkg_id, &fv, for_build)?; } // Activate the feature on the dependency. let summary = self.resolve.summary(dep_pkg_id); let fv = FeatureValue::new(*dep_feature, summary); self.activate_fv( dep_pkg_id, - self.sticky_dep_kind(dep_kind, dep.kind()), - compile_kind, &fv, + for_build || (self.opts.decouple_build_deps && dep.is_build()), )?; } } @@ -385,13 +334,12 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> { fn activate_rec( &mut self, pkg_id: PackageId, - dep_kind: DepKind, - compile_kind: CompileKind, feature_to_enable: InternedString, + for_build: bool, ) -> CargoResult<()> { let enabled = self .activated_features - .entry((pkg_id, dep_kind, compile_kind)) + .entry((pkg_id, for_build)) .or_insert_with(BTreeSet::new); if !enabled.insert(feature_to_enable) { // Already enabled. @@ -414,7 +362,7 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> { } }; for fv in fvs { - self.activate_fv(pkg_id, dep_kind, compile_kind, fv)?; + self.activate_fv(pkg_id, fv, for_build)?; } Ok(()) } @@ -439,7 +387,6 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> { fn fvs_from_requested( &self, pkg_id: PackageId, - compile_kind: CompileKind, requested_features: &RequestedFeatures, ) -> Vec { let summary = self.resolve.summary(pkg_id); @@ -450,7 +397,9 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> { .map(|k| FeatureValue::Feature(*k)) .collect(); // Add optional deps. - for (_dep_pkg_id, deps) in self.deps(pkg_id, compile_kind) { + // Top-level requested features can never apply to + // build-dependencies, so for_build is `false` here. + for (_dep_pkg_id, deps) in self.deps(pkg_id, false) { for dep in deps { if dep.is_optional() { // This may result in duplicates, but that should be ok. @@ -475,56 +424,55 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> { } /// Returns the dependencies for a package, filtering out inactive targets. - fn deps( - &self, - pkg_id: PackageId, - compile_kind: CompileKind, - ) -> Vec<(PackageId, Vec<&'a Dependency>)> { + fn deps(&self, pkg_id: PackageId, for_build: bool) -> Vec<(PackageId, Vec<&'a Dependency>)> { + // Helper for determining if a platform is activated. + let platform_activated = |dep: &Dependency| -> bool { + // We always care about build-dependencies, and they are always + // Host. If we are computing dependencies "for a build script", + // even normal dependencies are host-only. + if for_build || dep.is_build() { + return self + .target_data + .dep_platform_activated(dep, CompileKind::Host); + } + // Not a build dependency, and not for a build script, so must be Target. + return self + .target_data + .dep_platform_activated(dep, self.requested_target); + }; self.resolve .deps(pkg_id) .map(|(dep_id, deps)| { let deps = deps .iter() .filter(|dep| { - !dep.platform().is_some() - || !self.opts.ignore_inactive_targets - || self.target_data.dep_platform_activated(dep, compile_kind) + if dep.platform().is_some() + && self.opts.ignore_inactive_targets + && !platform_activated(dep) + { + return false; + } + if self.opts.decouple_dev_deps && dep.kind() == DepKind::Development { + return false; + } + true }) .collect::>(); (dep_id, deps) }) + .filter(|(_id, deps)| !deps.is_empty()) .collect() } - /// Convert a DepKind from a package to one of its dependencies. - /// - /// The rules here determine how decoupling works. - fn sticky_dep_kind(&self, from: DepKind, to: DepKind) -> DepKind { - if self.opts.decouple_build_deps { - if from == DepKind::Build || to == DepKind::Build { - return DepKind::Build; - } - } - if self.opts.decouple_dev_deps { - if to == DepKind::Development { - return DepKind::Development; - } - if from == DepKind::Development && to != DepKind::Build { - return DepKind::Development; - } - } - return DepKind::Normal; - } - /// Compare the activated features to the resolver. Used for testing. fn compare(&self) { let mut found = false; - for ((pkg_id, dep_kind, compile_kind), features) in &self.activated_features { + for ((pkg_id, dep_kind), features) in &self.activated_features { let r_features = self.resolve.features(*pkg_id); if !r_features.iter().eq(features.iter()) { eprintln!( - "{}/{:?}/{:?} features mismatch\nresolve: {:?}\nnew: {:?}\n", - pkg_id, dep_kind, compile_kind, r_features, features + "{}/{:?} features mismatch\nresolve: {:?}\nnew: {:?}\n", + pkg_id, dep_kind, r_features, features ); found = true; } diff --git a/src/cargo/ops/cargo_clean.rs b/src/cargo/ops/cargo_clean.rs index 223a6c57302..ea60bd31f00 100644 --- a/src/cargo/ops/cargo_clean.rs +++ b/src/cargo/ops/cargo_clean.rs @@ -6,7 +6,6 @@ use std::path::Path; use crate::core::compiler::unit_dependencies; use crate::core::compiler::{BuildConfig, BuildContext, CompileKind, CompileMode, Context}; use crate::core::compiler::{RustcTargetData, UnitInterner}; -use crate::core::dependency::DepKind; use crate::core::profiles::{Profiles, UnitFor}; use crate::core::resolver::features::{FeatureResolver, RequestedFeatures}; use crate::core::{PackageIdSpec, Workspace}; @@ -117,13 +116,11 @@ pub fn clean(ws: &Workspace<'_>, opts: &CleanOptions<'_>) -> CargoResult<()> { *mode, ) }; - for dep_kind in &[DepKind::Normal, DepKind::Development, DepKind::Build] { - let features = - features.activated_features(pkg.package_id(), *dep_kind, *kind); - units.push(bcx.units.intern( - pkg, target, profile, *kind, *mode, features, /*is_std*/ false, - )); - } + let features = features + .activated_features(pkg.package_id(), unit_for.is_for_build_dep()); + units.push(bcx.units.intern( + pkg, target, profile, *kind, *mode, features, /*is_std*/ false, + )); } } } diff --git a/src/cargo/ops/cargo_compile.rs b/src/cargo/ops/cargo_compile.rs index 874b60963d4..b6300055e98 100644 --- a/src/cargo/ops/cargo_compile.rs +++ b/src/cargo/ops/cargo_compile.rs @@ -33,7 +33,6 @@ use crate::core::compiler::unit_dependencies::build_unit_dependencies; use crate::core::compiler::{BuildConfig, BuildContext, Compilation, Context}; use crate::core::compiler::{CompileKind, CompileMode, RustcTargetData, Unit}; use crate::core::compiler::{DefaultExecutor, Executor, UnitInterner}; -use crate::core::dependency::DepKind; use crate::core::profiles::{Profiles, UnitFor}; use crate::core::resolver::features; use crate::core::resolver::{Resolve, ResolveOpts}; @@ -746,13 +745,9 @@ fn generate_targets<'a>( bcx.profiles .get_profile(pkg.package_id(), ws.is_member(pkg), unit_for, target_mode); - // Root units are not a dependency of anything, so they are always - // DepKind::Normal. Their features are driven by the command-line - // arguments. let features = Vec::from(resolved_features.activated_features( pkg.package_id(), - DepKind::Normal, - kind, + false, // Root units are never build dependencies. )); bcx.units.intern( pkg, @@ -903,12 +898,7 @@ fn generate_targets<'a>( let unavailable_features = match target.required_features() { Some(rf) => { let features = features_map.entry(pkg).or_insert_with(|| { - resolve_all_features( - resolve, - resolved_features, - pkg.package_id(), - default_arch_kind, - ) + resolve_all_features(resolve, resolved_features, pkg.package_id()) }); rf.iter().filter(|f| !features.contains(*f)).collect() } @@ -946,10 +936,9 @@ fn resolve_all_features( resolve_with_overrides: &Resolve, resolved_features: &features::ResolvedFeatures, package_id: PackageId, - default_arch_kind: CompileKind, ) -> HashSet { let mut features: HashSet = resolved_features - .activated_features(package_id, DepKind::Normal, default_arch_kind) + .activated_features(package_id, false) .iter() .map(|s| s.to_string()) .collect(); @@ -957,9 +946,7 @@ fn resolve_all_features( // Include features enabled for use by dependencies so targets can also use them with the // required-features field when deciding whether to be built or skipped. for (dep_id, deps) in resolve_with_overrides.deps(package_id) { - for feature in - resolved_features.activated_features(dep_id, DepKind::Normal, default_arch_kind) - { + for feature in resolved_features.activated_features(dep_id, false) { for dep in deps { features.insert(dep.name_in_toml().to_string() + "/" + &feature); } diff --git a/src/cargo/ops/cargo_output_metadata.rs b/src/cargo/ops/cargo_output_metadata.rs index d1f05f65dd3..3bdddd047ef 100644 --- a/src/cargo/ops/cargo_output_metadata.rs +++ b/src/cargo/ops/cargo_output_metadata.rs @@ -173,7 +173,7 @@ fn build_resolve_graph_r( if node_map.contains_key(&pkg_id) { return; } - let features = resolve.features(pkg_id).into_iter().cloned().collect(); + let features = resolve.features(pkg_id).iter().cloned().collect(); let deps: Vec = resolve .deps(pkg_id) diff --git a/src/cargo/ops/resolve.rs b/src/cargo/ops/resolve.rs index 905efee8436..da9c7059133 100644 --- a/src/cargo/ops/resolve.rs +++ b/src/cargo/ops/resolve.rs @@ -117,7 +117,7 @@ pub fn resolve_ws_with_opts<'a>( let resolved_with_overrides = resolve_with_previous( &mut registry, ws, - &opts, + opts, resolve.as_ref(), None, specs, diff --git a/tests/testsuite/profile_config.rs b/tests/testsuite/profile_config.rs index 5f62c90619f..e9928c5ae5b 100644 --- a/tests/testsuite/profile_config.rs +++ b/tests/testsuite/profile_config.rs @@ -413,7 +413,7 @@ fn named_config_profile() { assert_eq!(p.overflow_checks, true); // "dev" built-in (ignore package override) // build-override - let bo = profiles.get_profile(a_pkg, true, UnitFor::new_build(), CompileMode::Build); + let bo = profiles.get_profile(a_pkg, true, UnitFor::new_build(false), CompileMode::Build); assert_eq!(bo.name, "foo"); assert_eq!(bo.codegen_units, Some(6)); // "foo" build override from config assert_eq!(bo.opt_level, "1"); // SAME as normal From 6f337386e7d3cc08f75974970d30a56de950eaba Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Sun, 2 Feb 2020 13:21:31 -0800 Subject: [PATCH 13/22] Fix `cargo clean -p` crashing with -Zfeatures. --- src/cargo/core/resolver/features.rs | 38 ++++++++++++++++++++++------- src/cargo/ops/cargo_clean.rs | 8 ++++-- 2 files changed, 35 insertions(+), 11 deletions(-) diff --git a/src/cargo/core/resolver/features.rs b/src/cargo/core/resolver/features.rs index 490ff58a670..f0925af2e02 100644 --- a/src/cargo/core/resolver/features.rs +++ b/src/cargo/core/resolver/features.rs @@ -151,16 +151,37 @@ impl RequestedFeatures { impl ResolvedFeatures { /// Returns the list of features that are enabled for the given package. pub fn activated_features(&self, pkg_id: PackageId, is_build: bool) -> Vec { + self.activated_features_int(pkg_id, is_build, true) + } + + /// Variant of `activated_features` that returns an empty Vec if this is + /// not a valid pkg_id/is_build combination. Used by `cargo clean` which + /// doesn't know the exact set. + pub fn activated_features_unverified( + &self, + pkg_id: PackageId, + is_build: bool, + ) -> Vec { + self.activated_features_int(pkg_id, is_build, false) + } + + fn activated_features_int( + &self, + pkg_id: PackageId, + is_build: bool, + verify: bool, + ) -> Vec { if let Some(legacy) = &self.legacy { legacy.get(&pkg_id).map_or_else(Vec::new, |v| v.clone()) } else { let is_build = self.opts.decouple_build_deps && is_build; - // TODO: Remove panic, return empty set. - let fs = self - .activated_features - .get(&(pkg_id, is_build)) - .unwrap_or_else(|| panic!("features did not find {:?} {:?}", pkg_id, is_build)); - fs.iter().cloned().collect() + if let Some(fs) = self.activated_features.get(&(pkg_id, is_build)) { + fs.iter().cloned().collect() + } else if verify { + panic!("features did not find {:?} {:?}", pkg_id, is_build) + } else { + Vec::new() + } } } } @@ -436,9 +457,8 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> { .dep_platform_activated(dep, CompileKind::Host); } // Not a build dependency, and not for a build script, so must be Target. - return self - .target_data - .dep_platform_activated(dep, self.requested_target); + self.target_data + .dep_platform_activated(dep, self.requested_target) }; self.resolve .deps(pkg_id) diff --git a/src/cargo/ops/cargo_clean.rs b/src/cargo/ops/cargo_clean.rs index ea60bd31f00..92eb24a0a71 100644 --- a/src/cargo/ops/cargo_clean.rs +++ b/src/cargo/ops/cargo_clean.rs @@ -116,8 +116,12 @@ pub fn clean(ws: &Workspace<'_>, opts: &CleanOptions<'_>) -> CargoResult<()> { *mode, ) }; - let features = features - .activated_features(pkg.package_id(), unit_for.is_for_build_dep()); + // Use unverified here since this is being more + // exhaustive than what is actually needed. + let features = features.activated_features_unverified( + pkg.package_id(), + unit_for.is_for_build_dep(), + ); units.push(bcx.units.intern( pkg, target, profile, *kind, *mode, features, /*is_std*/ false, )); From 8973b959220c59d0f2bdfb13a6f8de228ab6b75d Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Sun, 2 Feb 2020 14:27:16 -0800 Subject: [PATCH 14/22] Fix bug where required-features didn't work with -Zfeatures=build_dep correctly. --- src/cargo/ops/cargo_compile.rs | 4 +-- tests/testsuite/features2.rs | 56 ++++++++++++++++++++++++++++++++++ 2 files changed, 58 insertions(+), 2 deletions(-) diff --git a/src/cargo/ops/cargo_compile.rs b/src/cargo/ops/cargo_compile.rs index b6300055e98..4302d6573d3 100644 --- a/src/cargo/ops/cargo_compile.rs +++ b/src/cargo/ops/cargo_compile.rs @@ -946,8 +946,8 @@ fn resolve_all_features( // Include features enabled for use by dependencies so targets can also use them with the // required-features field when deciding whether to be built or skipped. for (dep_id, deps) in resolve_with_overrides.deps(package_id) { - for feature in resolved_features.activated_features(dep_id, false) { - for dep in deps { + for dep in deps { + for feature in resolved_features.activated_features(dep_id, dep.is_build()) { features.insert(dep.name_in_toml().to_string() + "/" + &feature); } } diff --git a/tests/testsuite/features2.rs b/tests/testsuite/features2.rs index 55b2baec42f..92825867b5c 100644 --- a/tests/testsuite/features2.rs +++ b/tests/testsuite/features2.rs @@ -765,3 +765,59 @@ fn all_feature_opts() { .env("EXPECTED_FEATS", "5") .run(); } + +#[cargo_test] +fn required_features_build_dep() { + // Check that required-features handles build-dependencies correctly. + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + edition = "2018" + + [[bin]] + name = "x" + required-features = ["bdep/f1"] + + [build-dependencies] + bdep = {path="bdep"} + "#, + ) + .file("build.rs", "fn main() {}") + .file( + "src/bin/x.rs", + r#" + fn main() {} + "#, + ) + .file( + "bdep/Cargo.toml", + r#" + [package] + name = "bdep" + version = "0.1.0" + + [features] + f1 = [] + "#, + ) + .file("bdep/src/lib.rs", "") + .build(); + + p.cargo("run") + .with_status(101) + .with_stderr( + "\ +[ERROR] target `x` in package `foo` requires the features: `bdep/f1` +Consider enabling them by passing, e.g., `--features=\"bdep/f1\"` +", + ) + .run(); + + p.cargo("run --features bdep/f1 -Zfeatures=build_dep") + .masquerade_as_nightly_cargo() + .run(); +} From e0d64f94610f2c309a843b27eee786004784621e Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Wed, 19 Feb 2020 14:40:46 -0800 Subject: [PATCH 15/22] Change have_dev_units from a bool to enum HasDevUnits for clarity. --- src/cargo/core/compiler/standard_lib.rs | 12 +++++++++--- src/cargo/core/resolver/features.rs | 24 +++++++++++++++--------- src/cargo/core/resolver/mod.rs | 1 + src/cargo/ops/cargo_clean.rs | 4 ++-- src/cargo/ops/cargo_compile.rs | 7 +++++-- src/cargo/ops/cargo_doc.rs | 12 +++++++++--- src/cargo/ops/cargo_install.rs | 4 ++-- src/cargo/ops/cargo_output_metadata.rs | 5 ++--- src/cargo/ops/resolve.rs | 4 ++-- 9 files changed, 47 insertions(+), 26 deletions(-) diff --git a/src/cargo/core/compiler/standard_lib.rs b/src/cargo/core/compiler/standard_lib.rs index 80048f9780d..e41f0a42597 100644 --- a/src/cargo/core/compiler/standard_lib.rs +++ b/src/cargo/core/compiler/standard_lib.rs @@ -3,7 +3,7 @@ use crate::core::compiler::{BuildContext, CompileKind, CompileMode, RustcTargetData, Unit}; use crate::core::profiles::UnitFor; use crate::core::resolver::features::ResolvedFeatures; -use crate::core::resolver::ResolveOpts; +use crate::core::resolver::{HasDevUnits, ResolveOpts}; use crate::core::{Dependency, PackageId, PackageSet, Resolve, SourceId, Workspace}; use crate::ops::{self, Packages}; use crate::util::errors::CargoResult; @@ -102,8 +102,14 @@ pub fn resolve_std<'cfg>( /*dev_deps*/ false, &features, /*all_features*/ false, /*uses_default_features*/ true, ); - let resolve = - ops::resolve_ws_with_opts(&std_ws, target_data, requested_target, &opts, &specs, false)?; + let resolve = ops::resolve_ws_with_opts( + &std_ws, + target_data, + requested_target, + &opts, + &specs, + HasDevUnits::No, + )?; Ok(( resolve.pkg_set, resolve.targeted_resolve, diff --git a/src/cargo/core/resolver/features.rs b/src/cargo/core/resolver/features.rs index f0925af2e02..d82dccbb73a 100644 --- a/src/cargo/core/resolver/features.rs +++ b/src/cargo/core/resolver/features.rs @@ -56,8 +56,20 @@ struct FeatureOpts { compare: bool, } +/// Flag to indicate if Cargo is building *any* dev units (tests, examples, etc.). +/// +/// This disables decoupling of dev dependencies. It may be possible to relax +/// this in the future, but it will require significant changes to how unit +/// dependencies are computed, and can result in longer build times with +/// `cargo test` because the lib may need to be built 3 times instead of +/// twice. +pub enum HasDevUnits { + Yes, + No, +} + impl FeatureOpts { - fn new(config: &Config, has_dev_units: bool) -> CargoResult { + fn new(config: &Config, has_dev_units: HasDevUnits) -> CargoResult { let mut opts = FeatureOpts::default(); let unstable_flags = config.cli_unstable(); opts.package_features = unstable_flags.package_features; @@ -93,13 +105,7 @@ impl FeatureOpts { enable(&env_opts)?; } } - if has_dev_units { - // Decoupling of dev deps is not allowed if any test/bench/example - // is being built. It may be possible to relax this in the future, - // but it will require significant changes to how unit - // dependencies are computed, and can result in longer build times - // with `cargo test` because the lib may need to be built 3 times - // instead of twice. + if let HasDevUnits::Yes = has_dev_units { opts.decouple_dev_deps = false; } Ok(opts) @@ -211,7 +217,7 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> { requested_features: &RequestedFeatures, specs: &[PackageIdSpec], requested_target: CompileKind, - has_dev_units: bool, + has_dev_units: HasDevUnits, ) -> CargoResult { use crate::util::profile; let _p = profile::start("resolve features"); diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index 0cb197e0fdc..7646fc3ac8a 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/src/cargo/core/resolver/mod.rs @@ -69,6 +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::HasDevUnits; pub use self::resolve::{Resolve, ResolveVersion}; pub use self::types::ResolveOpts; diff --git a/src/cargo/ops/cargo_clean.rs b/src/cargo/ops/cargo_clean.rs index 92eb24a0a71..9629c445c1a 100644 --- a/src/cargo/ops/cargo_clean.rs +++ b/src/cargo/ops/cargo_clean.rs @@ -7,7 +7,7 @@ use crate::core::compiler::unit_dependencies; use crate::core::compiler::{BuildConfig, BuildContext, CompileKind, CompileMode, Context}; use crate::core::compiler::{RustcTargetData, UnitInterner}; use crate::core::profiles::{Profiles, UnitFor}; -use crate::core::resolver::features::{FeatureResolver, RequestedFeatures}; +use crate::core::resolver::features::{FeatureResolver, HasDevUnits, RequestedFeatures}; use crate::core::{PackageIdSpec, Workspace}; use crate::ops; use crate::util::errors::{CargoResult, CargoResultExt}; @@ -86,7 +86,7 @@ pub fn clean(ws: &Workspace<'_>, opts: &CleanOptions<'_>) -> CargoResult<()> { &requested_features, &specs, bcx.build_config.requested_kind, - true, + HasDevUnits::Yes, )?; let mut units = Vec::new(); diff --git a/src/cargo/ops/cargo_compile.rs b/src/cargo/ops/cargo_compile.rs index 4302d6573d3..ec1517ac03d 100644 --- a/src/cargo/ops/cargo_compile.rs +++ b/src/cargo/ops/cargo_compile.rs @@ -35,7 +35,7 @@ use crate::core::compiler::{CompileKind, CompileMode, RustcTargetData, Unit}; use crate::core::compiler::{DefaultExecutor, Executor, UnitInterner}; use crate::core::profiles::{Profiles, UnitFor}; use crate::core::resolver::features; -use crate::core::resolver::{Resolve, ResolveOpts}; +use crate::core::resolver::{HasDevUnits, Resolve, ResolveOpts}; use crate::core::{LibKind, Package, PackageSet, Target}; use crate::core::{PackageId, PackageIdSpec, TargetKind, Workspace}; use crate::ops; @@ -312,7 +312,10 @@ pub fn compile_ws<'a>( 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, features, all_features, !no_default_features); - let has_dev_units = filter.need_dev_deps(build_config.mode); + let has_dev_units = match filter.need_dev_deps(build_config.mode) { + true => HasDevUnits::Yes, + false => HasDevUnits::No, + }; let resolve = ops::resolve_ws_with_opts( ws, &target_data, diff --git a/src/cargo/ops/cargo_doc.rs b/src/cargo/ops/cargo_doc.rs index f932a11cac8..466c1333258 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::ResolveOpts; +use crate::core::resolver::{HasDevUnits, ResolveOpts}; use crate::core::{Shell, Workspace}; use crate::ops; use crate::util::CargoResult; @@ -27,8 +27,14 @@ pub fn doc(ws: &Workspace<'_>, options: &DocOptions<'_>) -> CargoResult<()> { ); let requested_kind = options.compile_opts.build_config.requested_kind; let target_data = RustcTargetData::new(ws, requested_kind)?; - let ws_resolve = - ops::resolve_ws_with_opts(ws, &target_data, requested_kind, &opts, &specs, false)?; + let ws_resolve = ops::resolve_ws_with_opts( + ws, + &target_data, + requested_kind, + &opts, + &specs, + HasDevUnits::No, + )?; let ids = specs .iter() diff --git a/src/cargo/ops/cargo_install.rs b/src/cargo/ops/cargo_install.rs index 394c5651286..0ff2edbc6b2 100644 --- a/src/cargo/ops/cargo_install.rs +++ b/src/cargo/ops/cargo_install.rs @@ -8,7 +8,7 @@ use tempfile::Builder as TempFileBuilder; use crate::core::compiler::Freshness; use crate::core::compiler::{CompileKind, DefaultExecutor, Executor, RustcTargetData}; -use crate::core::resolver::ResolveOpts; +use crate::core::resolver::{HasDevUnits, ResolveOpts}; use crate::core::{Edition, Package, PackageId, PackageIdSpec, Source, SourceId, Workspace}; use crate::ops; use crate::ops::common_for_install_and_uninstall::*; @@ -506,7 +506,7 @@ fn check_yanked_install(ws: &Workspace<'_>) -> CargoResult<()> { CompileKind::Host, &ResolveOpts::everything(), &specs, - false, + HasDevUnits::No, )?; let mut sources = ws_resolve.pkg_set.sources_mut(); diff --git a/src/cargo/ops/cargo_output_metadata.rs b/src/cargo/ops/cargo_output_metadata.rs index 3bdddd047ef..c1e5c720394 100644 --- a/src/cargo/ops/cargo_output_metadata.rs +++ b/src/cargo/ops/cargo_output_metadata.rs @@ -1,7 +1,6 @@ use crate::core::compiler::{CompileKind, CompileTarget, RustcTargetData}; -use crate::core::resolver::{Resolve, ResolveOpts}; - use crate::core::dependency::DepKind; +use crate::core::resolver::{HasDevUnits, Resolve, ResolveOpts}; use crate::core::{Dependency, InternedString, Package, PackageId, Workspace}; use crate::ops::{self, Packages}; use crate::util::CargoResult; @@ -125,7 +124,7 @@ fn build_resolve_graph( requested_kind, &resolve_opts, &specs, - true, + HasDevUnits::Yes, )?; // Download all Packages. This is needed to serialize the information // for every package. In theory this could honor target filtering, diff --git a/src/cargo/ops/resolve.rs b/src/cargo/ops/resolve.rs index da9c7059133..2c11b68d45f 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::{FeatureResolver, ResolvedFeatures}; -use crate::core::resolver::{self, Resolve, ResolveOpts}; +use crate::core::resolver::{self, HasDevUnits, Resolve, ResolveOpts}; use crate::core::summary::Summary; use crate::core::Feature; use crate::core::{PackageId, PackageIdSpec, PackageSet, Source, SourceId, Workspace}; @@ -78,7 +78,7 @@ pub fn resolve_ws_with_opts<'a>( requested_target: CompileKind, opts: &ResolveOpts, specs: &[PackageIdSpec], - has_dev_units: bool, + has_dev_units: HasDevUnits, ) -> CargoResult> { let mut registry = PackageRegistry::new(ws.config())?; let mut add_patches = true; From 137642c4a3e6365800de739efd4c91a892c03539 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Wed, 19 Feb 2020 15:05:43 -0800 Subject: [PATCH 16/22] Change for_build_dep from a bool to enum FeaturesFor for clarity. --- src/cargo/core/compiler/standard_lib.rs | 5 +++-- src/cargo/core/compiler/unit_dependencies.rs | 16 ++++++++++---- src/cargo/core/resolver/features.rs | 23 +++++++++++++++----- src/cargo/ops/cargo_clean.rs | 14 +++++++----- src/cargo/ops/cargo_compile.rs | 12 ++++++---- 5 files changed, 49 insertions(+), 21 deletions(-) diff --git a/src/cargo/core/compiler/standard_lib.rs b/src/cargo/core/compiler/standard_lib.rs index e41f0a42597..405a9524110 100644 --- a/src/cargo/core/compiler/standard_lib.rs +++ b/src/cargo/core/compiler/standard_lib.rs @@ -2,7 +2,7 @@ use crate::core::compiler::{BuildContext, CompileKind, CompileMode, RustcTargetData, Unit}; use crate::core::profiles::UnitFor; -use crate::core::resolver::features::ResolvedFeatures; +use crate::core::resolver::features::{FeaturesFor, ResolvedFeatures}; use crate::core::resolver::{HasDevUnits, ResolveOpts}; use crate::core::{Dependency, PackageId, PackageSet, Resolve, SourceId, Workspace}; use crate::ops::{self, Packages}; @@ -154,7 +154,8 @@ pub fn generate_std_roots<'a>( unit_for, mode, ); - let features = std_features.activated_features(pkg.package_id(), false); + let features = + std_features.activated_features(pkg.package_id(), FeaturesFor::NormalOrDev); Ok(bcx.units.intern( pkg, lib, profile, kind, mode, features, /*is_std*/ true, )) diff --git a/src/cargo/core/compiler/unit_dependencies.rs b/src/cargo/core/compiler/unit_dependencies.rs index b84ff7e7098..6a0ed3370cb 100644 --- a/src/cargo/core/compiler/unit_dependencies.rs +++ b/src/cargo/core/compiler/unit_dependencies.rs @@ -20,7 +20,7 @@ use crate::core::compiler::{BuildContext, CompileKind, CompileMode}; use crate::core::dependency::DepKind; use crate::core::package::Downloads; use crate::core::profiles::{Profile, UnitFor}; -use crate::core::resolver::features::ResolvedFeatures; +use crate::core::resolver::features::{FeaturesFor, ResolvedFeatures}; use crate::core::resolver::Resolve; use crate::core::{InternedString, Package, PackageId, Target}; use crate::CargoResult; @@ -586,7 +586,11 @@ fn new_unit_dep_with_profile<'a>( let public = state .resolve() .is_public_dep(parent.pkg.package_id(), pkg.package_id()); - let features = state.activated_features(pkg.package_id(), unit_for.is_for_build_dep()); + let features_for = match unit_for.is_for_build_dep() { + true => FeaturesFor::BuildDep, + false => FeaturesFor::NormalOrDev, + }; + let features = state.activated_features(pkg.package_id(), features_for); let unit = state .bcx .units @@ -691,13 +695,17 @@ impl<'a, 'cfg> State<'a, 'cfg> { } } - fn activated_features(&self, pkg_id: PackageId, for_build_dep: bool) -> Vec { + fn activated_features( + &self, + pkg_id: PackageId, + features_for: FeaturesFor, + ) -> Vec { let features = if self.is_std { self.std_features.unwrap() } else { self.usr_features }; - features.activated_features(pkg_id, for_build_dep) + features.activated_features(pkg_id, features_for) } fn get(&mut self, id: PackageId) -> CargoResult> { diff --git a/src/cargo/core/resolver/features.rs b/src/cargo/core/resolver/features.rs index d82dccbb73a..eefb2044dbd 100644 --- a/src/cargo/core/resolver/features.rs +++ b/src/cargo/core/resolver/features.rs @@ -68,6 +68,13 @@ pub enum HasDevUnits { No, } +/// Flag to indicate if features are requested for a build dependency or not. +#[derive(PartialEq)] +pub enum FeaturesFor { + NormalOrDev, + BuildDep, +} + impl FeatureOpts { fn new(config: &Config, has_dev_units: HasDevUnits) -> CargoResult { let mut opts = FeatureOpts::default(); @@ -156,8 +163,12 @@ impl RequestedFeatures { impl ResolvedFeatures { /// Returns the list of features that are enabled for the given package. - pub fn activated_features(&self, pkg_id: PackageId, is_build: bool) -> Vec { - self.activated_features_int(pkg_id, is_build, true) + pub fn activated_features( + &self, + pkg_id: PackageId, + features_for: FeaturesFor, + ) -> Vec { + self.activated_features_int(pkg_id, features_for, true) } /// Variant of `activated_features` that returns an empty Vec if this is @@ -166,21 +177,21 @@ impl ResolvedFeatures { pub fn activated_features_unverified( &self, pkg_id: PackageId, - is_build: bool, + features_for: FeaturesFor, ) -> Vec { - self.activated_features_int(pkg_id, is_build, false) + self.activated_features_int(pkg_id, features_for, false) } fn activated_features_int( &self, pkg_id: PackageId, - is_build: bool, + features_for: FeaturesFor, verify: bool, ) -> Vec { if let Some(legacy) = &self.legacy { legacy.get(&pkg_id).map_or_else(Vec::new, |v| v.clone()) } else { - let is_build = self.opts.decouple_build_deps && is_build; + let is_build = self.opts.decouple_build_deps && features_for == FeaturesFor::BuildDep; if let Some(fs) = self.activated_features.get(&(pkg_id, is_build)) { fs.iter().cloned().collect() } else if verify { diff --git a/src/cargo/ops/cargo_clean.rs b/src/cargo/ops/cargo_clean.rs index 9629c445c1a..16a6de24ce1 100644 --- a/src/cargo/ops/cargo_clean.rs +++ b/src/cargo/ops/cargo_clean.rs @@ -7,7 +7,9 @@ use crate::core::compiler::unit_dependencies; use crate::core::compiler::{BuildConfig, BuildContext, CompileKind, CompileMode, Context}; use crate::core::compiler::{RustcTargetData, UnitInterner}; use crate::core::profiles::{Profiles, UnitFor}; -use crate::core::resolver::features::{FeatureResolver, HasDevUnits, RequestedFeatures}; +use crate::core::resolver::features::{ + FeatureResolver, FeaturesFor, HasDevUnits, RequestedFeatures, +}; use crate::core::{PackageIdSpec, Workspace}; use crate::ops; use crate::util::errors::{CargoResult, CargoResultExt}; @@ -118,10 +120,12 @@ pub fn clean(ws: &Workspace<'_>, opts: &CleanOptions<'_>) -> CargoResult<()> { }; // Use unverified here since this is being more // exhaustive than what is actually needed. - let features = features.activated_features_unverified( - pkg.package_id(), - unit_for.is_for_build_dep(), - ); + let features_for = match unit_for.is_for_build_dep() { + true => FeaturesFor::BuildDep, + false => FeaturesFor::NormalOrDev, + }; + let features = + features.activated_features_unverified(pkg.package_id(), features_for); units.push(bcx.units.intern( pkg, target, profile, *kind, *mode, features, /*is_std*/ false, )); diff --git a/src/cargo/ops/cargo_compile.rs b/src/cargo/ops/cargo_compile.rs index ec1517ac03d..fc6dffd7544 100644 --- a/src/cargo/ops/cargo_compile.rs +++ b/src/cargo/ops/cargo_compile.rs @@ -34,7 +34,7 @@ use crate::core::compiler::{BuildConfig, BuildContext, Compilation, Context}; use crate::core::compiler::{CompileKind, CompileMode, RustcTargetData, Unit}; use crate::core::compiler::{DefaultExecutor, Executor, UnitInterner}; use crate::core::profiles::{Profiles, UnitFor}; -use crate::core::resolver::features; +use crate::core::resolver::features::{self, FeaturesFor}; use crate::core::resolver::{HasDevUnits, Resolve, ResolveOpts}; use crate::core::{LibKind, Package, PackageSet, Target}; use crate::core::{PackageId, PackageIdSpec, TargetKind, Workspace}; @@ -750,7 +750,7 @@ fn generate_targets<'a>( let features = Vec::from(resolved_features.activated_features( pkg.package_id(), - false, // Root units are never build dependencies. + FeaturesFor::NormalOrDev, // Root units are never build dependencies. )); bcx.units.intern( pkg, @@ -941,7 +941,7 @@ fn resolve_all_features( package_id: PackageId, ) -> HashSet { let mut features: HashSet = resolved_features - .activated_features(package_id, false) + .activated_features(package_id, FeaturesFor::NormalOrDev) .iter() .map(|s| s.to_string()) .collect(); @@ -950,7 +950,11 @@ fn resolve_all_features( // required-features field when deciding whether to be built or skipped. for (dep_id, deps) in resolve_with_overrides.deps(package_id) { for dep in deps { - for feature in resolved_features.activated_features(dep_id, dep.is_build()) { + let features_for = match dep.is_build() { + true => FeaturesFor::BuildDep, + false => FeaturesFor::NormalOrDev, + }; + for feature in resolved_features.activated_features(dep_id, features_for) { features.insert(dep.name_in_toml().to_string() + "/" + &feature); } } From b7c0e2e1c86623e667e2151a8d1f142012c1ab19 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Wed, 19 Feb 2020 15:12:34 -0800 Subject: [PATCH 17/22] Compute dep_for_build in one place. --- src/cargo/core/resolver/features.rs | 37 +++++++++++++---------------- 1 file changed, 17 insertions(+), 20 deletions(-) diff --git a/src/cargo/core/resolver/features.rs b/src/cargo/core/resolver/features.rs index eefb2044dbd..ffa38fe8109 100644 --- a/src/cargo/core/resolver/features.rs +++ b/src/cargo/core/resolver/features.rs @@ -297,17 +297,13 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> { return Ok(()); } for (dep_pkg_id, deps) in self.deps(pkg_id, for_build) { - for dep in deps { + for (dep, dep_for_build) in deps { if dep.is_optional() { continue; } // Recurse into the dependency. let fvs = self.fvs_from_dependency(dep_pkg_id, dep); - self.activate_pkg( - dep_pkg_id, - &fvs, - for_build || (self.opts.decouple_build_deps && dep.is_build()), - )?; + self.activate_pkg(dep_pkg_id, &fvs, dep_for_build)?; } } Ok(()) @@ -329,14 +325,10 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> { self.activate_rec(pkg_id, *dep_name, for_build)?; // Activate the optional dep. for (dep_pkg_id, deps) in self.deps(pkg_id, for_build) { - for dep in deps { + for (dep, dep_for_build) in deps { if dep.name_in_toml() == *dep_name { let fvs = self.fvs_from_dependency(dep_pkg_id, dep); - self.activate_pkg( - dep_pkg_id, - &fvs, - for_build || (self.opts.decouple_build_deps && dep.is_build()), - )?; + self.activate_pkg(dep_pkg_id, &fvs, dep_for_build)?; } } } @@ -344,7 +336,7 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> { FeatureValue::CrateFeature(dep_name, dep_feature) => { // Activate a feature within a dependency. for (dep_pkg_id, deps) in self.deps(pkg_id, for_build) { - for dep in deps { + for (dep, dep_for_build) in deps { if dep.name_in_toml() == *dep_name { if dep.is_optional() { // Activate the crate on self. @@ -354,11 +346,7 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> { // Activate the feature on the dependency. let summary = self.resolve.summary(dep_pkg_id); let fv = FeatureValue::new(*dep_feature, summary); - self.activate_fv( - dep_pkg_id, - &fv, - for_build || (self.opts.decouple_build_deps && dep.is_build()), - )?; + self.activate_fv(dep_pkg_id, &fv, dep_for_build)?; } } } @@ -438,7 +426,7 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> { // Top-level requested features can never apply to // build-dependencies, so for_build is `false` here. for (_dep_pkg_id, deps) in self.deps(pkg_id, false) { - for dep in deps { + for (dep, _dep_for_build) in deps { if dep.is_optional() { // This may result in duplicates, but that should be ok. fvs.push(FeatureValue::Crate(dep.name_in_toml())); @@ -462,7 +450,11 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> { } /// Returns the dependencies for a package, filtering out inactive targets. - fn deps(&self, pkg_id: PackageId, for_build: bool) -> Vec<(PackageId, Vec<&'a Dependency>)> { + fn deps( + &self, + pkg_id: PackageId, + for_build: bool, + ) -> Vec<(PackageId, Vec<(&'a Dependency, bool)>)> { // Helper for determining if a platform is activated. let platform_activated = |dep: &Dependency| -> bool { // We always care about build-dependencies, and they are always @@ -494,6 +486,11 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> { } true }) + .map(|dep| { + let dep_for_build = + for_build || (self.opts.decouple_build_deps && dep.is_build()); + (dep, dep_for_build) + }) .collect::>(); (dep_id, deps) }) From ffb13a513383c2a1ec0f7e4b41fddf8422429d8f Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Wed, 19 Feb 2020 15:45:42 -0800 Subject: [PATCH 18/22] Try to remove some rightwards drift. --- src/cargo/core/resolver/features.rs | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/src/cargo/core/resolver/features.rs b/src/cargo/core/resolver/features.rs index ffa38fe8109..8ace40d0243 100644 --- a/src/cargo/core/resolver/features.rs +++ b/src/cargo/core/resolver/features.rs @@ -326,10 +326,11 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> { // Activate the optional dep. for (dep_pkg_id, deps) in self.deps(pkg_id, for_build) { for (dep, dep_for_build) in deps { - if dep.name_in_toml() == *dep_name { - let fvs = self.fvs_from_dependency(dep_pkg_id, dep); - self.activate_pkg(dep_pkg_id, &fvs, dep_for_build)?; + if dep.name_in_toml() != *dep_name { + continue; } + let fvs = self.fvs_from_dependency(dep_pkg_id, dep); + self.activate_pkg(dep_pkg_id, &fvs, dep_for_build)?; } } } @@ -337,17 +338,18 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> { // Activate a feature within a dependency. for (dep_pkg_id, deps) in self.deps(pkg_id, for_build) { for (dep, dep_for_build) in deps { - if dep.name_in_toml() == *dep_name { - if dep.is_optional() { - // Activate the crate on self. - let fv = FeatureValue::Crate(*dep_name); - self.activate_fv(pkg_id, &fv, for_build)?; - } - // Activate the feature on the dependency. - let summary = self.resolve.summary(dep_pkg_id); - let fv = FeatureValue::new(*dep_feature, summary); - self.activate_fv(dep_pkg_id, &fv, dep_for_build)?; + if dep.name_in_toml() != *dep_name { + continue; } + if dep.is_optional() { + // Activate the crate on self. + let fv = FeatureValue::Crate(*dep_name); + self.activate_fv(pkg_id, &fv, for_build)?; + } + // Activate the feature on the dependency. + let summary = self.resolve.summary(dep_pkg_id); + let fv = FeatureValue::new(*dep_feature, summary); + self.activate_fv(dep_pkg_id, &fv, dep_for_build)?; } } } From 2cec46ec8336278f41952158a6a130119ef28c37 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Wed, 19 Feb 2020 15:58:20 -0800 Subject: [PATCH 19/22] Try to clarify some things about UnitFor build flags. --- src/cargo/core/profiles.rs | 28 +++++++++++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-) diff --git a/src/cargo/core/profiles.rs b/src/cargo/core/profiles.rs index 9c079a7cdd3..ff3e5283ba6 100644 --- a/src/cargo/core/profiles.rs +++ b/src/cargo/core/profiles.rs @@ -766,6 +766,8 @@ pub struct UnitFor { /// A target for `build.rs` or any of its dependencies, or a proc-macro or /// any of its dependencies. This enables `build-override` profiles for /// these targets. + /// + /// An invariant is that if `build_dep` is true, `host` must be true. host: bool, /// A target for a build dependency (or any of its dependencies). This is /// used for computing features of build dependencies independently of @@ -775,7 +777,28 @@ pub struct UnitFor { /// for a non-host package sets this to `false` because it wants the /// features of the non-host package (whereas `host` is true because the /// build script is being built for the host). `build_dep` becomes `true` - /// for build-dependencies, or any of their dependencies. + /// for build-dependencies, or any of their dependencies. For example, with + /// this dependency tree: + /// + /// ```text + /// foo + /// ├── foo build.rs + /// │ └── shared_dep (BUILD dependency) + /// │ └── shared_dep build.rs + /// └── shared_dep (Normal dependency) + /// └── shared_dep build.rs + /// ``` + /// + /// In this example, `foo build.rs` is HOST=true, BUILD_DEP=false. This is + /// so that `foo build.rs` gets the profile settings for build scripts + /// (HOST=true) and features of foo (BUILD_DEP=false) because build scripts + /// need to know which features their package is being built with. + /// + /// But in the case of `shared_dep`, when built as a build dependency, + /// both flags are true (it only wants the build-dependency features). + /// When `shared_dep` is built as a normal dependency, then `shared_dep + /// build.rs` is HOST=true, BUILD_DEP=false for the same reasons that + /// foo's build script is set that way. build_dep: bool, /// How Cargo processes the `panic` setting or profiles. This is done to /// handle test/benches inheriting from dev/release, as well as forcing @@ -883,6 +906,9 @@ impl UnitFor { /// This is part of the machinery responsible for handling feature /// decoupling for build dependencies in the new feature resolver. pub fn with_build_dep(mut self, build_dep: bool) -> UnitFor { + if build_dep { + assert!(self.host); + } self.build_dep = self.build_dep || build_dep; self } From df0ae1867e7a2406bb1fb9d374d5a63966bf86ce Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Thu, 20 Feb 2020 08:56:41 -0800 Subject: [PATCH 20/22] Add comment trying to explain recursion and optional dependencies. --- src/cargo/core/resolver/features.rs | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/src/cargo/core/resolver/features.rs b/src/cargo/core/resolver/features.rs index 8ace40d0243..d4f4cd90b4f 100644 --- a/src/cargo/core/resolver/features.rs +++ b/src/cargo/core/resolver/features.rs @@ -293,12 +293,26 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> { self.activate_fv(pkg_id, fv, for_build)?; } if !self.processed_deps.insert((pkg_id, for_build)) { - // Already processed dependencies. + // Already processed dependencies. There's no need to process them + // again. This is primarily to avoid cycles, but also helps speed + // things up. + // + // This is safe because if another package comes along and adds a + // feature on this package, it will immediately add it (in + // `activate_fv`), and recurse as necessary right then and there. + // For example, consider we've already processed our dependencies, + // and another package comes along and enables one of our optional + // dependencies, it will do so immediately in the + // `FeatureValue::CrateFeature` branch, and then immediately + // recurse into that optional dependency. This also holds true for + // features that enable other features. return Ok(()); } for (dep_pkg_id, deps) in self.deps(pkg_id, for_build) { for (dep, dep_for_build) in deps { if dep.is_optional() { + // Optional dependencies are enabled in `activate_fv` when + // a feature enables it. continue; } // Recurse into the dependency. From 3d6b5b1ffc41ce2c80eb5480ef3e7d4ca4e737c1 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Thu, 20 Feb 2020 09:05:17 -0800 Subject: [PATCH 21/22] Add comment on relationship of RunCustomBuild and UnitFor::host. --- src/cargo/core/compiler/unit_dependencies.rs | 25 ++++++++++++++++++++ src/cargo/core/profiles.rs | 9 +++++++ 2 files changed, 34 insertions(+) diff --git a/src/cargo/core/compiler/unit_dependencies.rs b/src/cargo/core/compiler/unit_dependencies.rs index 6a0ed3370cb..48841defd6f 100644 --- a/src/cargo/core/compiler/unit_dependencies.rs +++ b/src/cargo/core/compiler/unit_dependencies.rs @@ -515,6 +515,31 @@ fn dep_build_script<'a>( .bcx .profiles .get_profile_run_custom_build(&unit.profile); + // UnitFor::new_build is used because we want the `host` flag set + // for all of our build dependencies (so they all get + // build-override profiles), including compiling the build.rs + // script itself. + // + // If `is_for_build_dep` here is `false`, that means we are a + // build.rs script for a normal dependency and we want to set the + // CARGO_FEATURE_* environment variables to the features as a + // normal dep. + // + // If `is_for_build_dep` here is `true`, that means that this + // package is being used as a build dependency, and so we only + // want to set CARGO_FEATURE_* variables for the build-dependency + // side of the graph. + // + // Keep in mind that the RunCustomBuild unit and the Compile + // build.rs unit use the same features. This is because some + // people use `cfg!` and `#[cfg]` expressions to check for enabled + // features instead of just checking `CARGO_FEATURE_*` at runtime. + // In the case with `-Zfeatures=build_dep`, and a shared + // dependency has different features enabled for normal vs. build, + // then the build.rs script will get compiled twice. I believe it + // is not feasible to only build it once because it would break a + // large number of scripts (they would think they have the wrong + // set of features enabled). let script_unit_for = UnitFor::new_build(unit_for.is_for_build_dep()); new_unit_dep_with_profile( state, diff --git a/src/cargo/core/profiles.rs b/src/cargo/core/profiles.rs index ff3e5283ba6..2f83aba2211 100644 --- a/src/cargo/core/profiles.rs +++ b/src/cargo/core/profiles.rs @@ -768,6 +768,15 @@ pub struct UnitFor { /// these targets. /// /// An invariant is that if `build_dep` is true, `host` must be true. + /// + /// Note that this is `true` for `RunCustomBuild` units, even though that + /// unit should *not* use build-override profiles. This is a bit of a + /// special case. When computing the `RunCustomBuild` unit, it manually + /// uses the `get_profile_run_custom_build` method to get the correct + /// profile information for the unit. `host` needs to be true so that all + /// of the dependencies of that `RunCustomBuild` unit have this flag be + /// sticky (and forced to `true` for all further dependencies) — which is + /// the whole point of `UnitFor`. host: bool, /// A target for a build dependency (or any of its dependencies). This is /// used for computing features of build dependencies independently of From 4d0fda7cd9efb14055915e755b09ee3cf64586ff Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Thu, 20 Feb 2020 11:42:53 -0800 Subject: [PATCH 22/22] Fix bug where an optional dependency is disabled in one fork, and enabled in another. --- src/cargo/core/compiler/unit_dependencies.rs | 16 +++++ tests/testsuite/features2.rs | 72 ++++++++++++++++++++ 2 files changed, 88 insertions(+) diff --git a/src/cargo/core/compiler/unit_dependencies.rs b/src/cargo/core/compiler/unit_dependencies.rs index 48841defd6f..7f19c917539 100644 --- a/src/cargo/core/compiler/unit_dependencies.rs +++ b/src/cargo/core/compiler/unit_dependencies.rs @@ -264,11 +264,27 @@ fn compute_deps<'a, 'cfg>( return false; } + // If this is an optional dependency, and the new feature resolver + // did not enable it, don't include it. + if dep.is_optional() { + let features_for = match unit_for.is_for_build_dep() { + true => FeaturesFor::BuildDep, + false => FeaturesFor::NormalOrDev, + }; + + let feats = state.activated_features(id, features_for); + if !feats.contains(&dep.name_in_toml()) { + return false; + } + } + // If we've gotten past all that, then this dependency is // actually used! true }) }); + // Separate line to avoid rustfmt indentation. Must collect due to `state` capture. + let filtered_deps: Vec<_> = filtered_deps.collect(); let mut ret = Vec::new(); for (id, _) in filtered_deps { diff --git a/tests/testsuite/features2.rs b/tests/testsuite/features2.rs index 92825867b5c..85866cb6646 100644 --- a/tests/testsuite/features2.rs +++ b/tests/testsuite/features2.rs @@ -821,3 +821,75 @@ Consider enabling them by passing, e.g., `--features=\"bdep/f1\"` .masquerade_as_nightly_cargo() .run(); } + +#[cargo_test] +fn disabled_shared_build_dep() { + // Check for situation where an optional dep of a shared dep is enabled in + // a normal dependency, but disabled in an optional one. The unit tree is: + // foo + // ├── foo build.rs + // | └── common (BUILD dependency, NO FEATURES) + // └── common (Normal dependency, default features) + // └── somedep + Package::new("somedep", "1.0.0") + .file( + "src/lib.rs", + r#" + pub fn f() { println!("hello from somedep"); } + "#, + ) + .publish(); + Package::new("common", "1.0.0") + .feature("default", &["somedep"]) + .add_dep(Dependency::new("somedep", "1.0").optional(true)) + .file( + "src/lib.rs", + r#" + pub fn check_somedep() -> bool { + #[cfg(feature="somedep")] + { + extern crate somedep; + somedep::f(); + true + } + #[cfg(not(feature="somedep"))] + { + println!("no somedep"); + false + } + } + "#, + ) + .publish(); + + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "1.0.0" + edition = "2018" + + [dependencies] + common = "1.0" + + [build-dependencies] + common = {version = "1.0", default-features = false} + "#, + ) + .file( + "src/main.rs", + "fn main() { assert!(common::check_somedep()); }", + ) + .file( + "build.rs", + "fn main() { assert!(!common::check_somedep()); }", + ) + .build(); + + p.cargo("run -Zfeatures=build_dep -v") + .masquerade_as_nightly_cargo() + .with_stdout("hello from somedep") + .run(); +}