From 7de30dd21a6b78c718c9ee88b661bd0575542884 Mon Sep 17 00:00:00 2001 From: Ximin Luo Date: Mon, 29 Jan 2018 05:03:51 +0100 Subject: [PATCH 1/7] Don't check for dev-dependencies when not needed --- src/cargo/core/resolver/mod.rs | 21 +++++++++++++++++--- src/cargo/ops/cargo_compile.rs | 28 ++++++++++++++++++++------- src/cargo/ops/mod.rs | 2 +- src/cargo/ops/resolve.rs | 35 ++++++++++++++++++---------------- 4 files changed, 59 insertions(+), 27 deletions(-) diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index dc109191d0b..7b33cba2cd4 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/src/cargo/core/resolver/mod.rs @@ -96,14 +96,26 @@ pub struct DepsNotReplaced<'a> { #[derive(Clone, Copy)] pub enum Method<'a> { - Everything, + Everything, // equivalent to Required { dev_deps: true, all_features: true, .. } Required { dev_deps: bool, features: &'a [String], + all_features: bool, uses_default_features: bool, }, } +impl<'r> Method<'r> { + pub fn split_features(features: &[String]) -> Vec { + features.iter() + .flat_map(|s| s.split_whitespace()) + .flat_map(|s| s.split(',')) + .filter(|s| !s.is_empty()) + .map(|s| s.to_string()) + .collect::>() + } +} + // Information about the dependencies for a crate, a tuple of: // // (dependency info, candidates, features activated) @@ -731,6 +743,7 @@ fn activate_deps_loop<'a>(mut cx: Context<'a>, let method = Method::Required { dev_deps: false, features: &features, + all_features: false, uses_default_features: dep.uses_default_features(), }; trace!("{}[{}]>{} trying {}", parent.name(), cur, dep.name(), @@ -1006,7 +1019,8 @@ fn build_requirements<'a, 'b: 'a>(s: &'a Summary, method: &'b Method) -> CargoResult> { let mut reqs = Requirements::new(s); match *method { - Method::Everything => { + Method::Everything | + Method::Required { all_features: true, .. } => { for key in s.features().keys() { reqs.require_feature(key)?; } @@ -1052,10 +1066,11 @@ impl<'a> Context<'a> { } debug!("checking if {} is already activated", summary.package_id()); let (features, use_default) = match *method { + Method::Everything | + Method::Required { all_features: true, .. } => return false, Method::Required { features, uses_default_features, .. } => { (features, uses_default_features) } - Method::Everything => return false, }; let has_default_feature = summary.features().contains_key("default"); diff --git a/src/cargo/ops/cargo_compile.rs b/src/cargo/ops/cargo_compile.rs index cc3400268f9..a212a64552e 100644 --- a/src/cargo/ops/cargo_compile.rs +++ b/src/cargo/ops/cargo_compile.rs @@ -29,7 +29,7 @@ use std::sync::Arc; use core::{Source, Package, Target}; use core::{Profile, TargetKind, Profiles, Workspace, PackageId, PackageIdSpec}; -use core::resolver::Resolve; +use core::resolver::{Resolve, Method}; use ops::{self, BuildOutput, Executor, DefaultExecutor}; use util::config::Config; use util::{CargoResult, profile}; @@ -226,12 +226,18 @@ pub fn compile_ws<'a>(ws: &Workspace<'a>, let profiles = ws.profiles(); let specs = spec.into_package_id_specs(ws)?; - let resolve = ops::resolve_ws_precisely(ws, - source, - features, - all_features, - no_default_features, - &specs)?; + let features = Method::split_features(features); + let method = Method::Required { + dev_deps: filter.need_dev_deps(), + features: &features, + all_features, + uses_default_features: !no_default_features, + }; + let resolve = ops::resolve_ws_with_method(ws, + source, + method, + &specs, + )?; let (packages, resolve_with_overrides) = resolve; if specs.is_empty() { @@ -413,6 +419,14 @@ impl<'a> CompileFilter<'a> { } } + pub fn need_dev_deps(&self) -> bool { + match *self { + CompileFilter::Default { .. } => true, + CompileFilter::Only { examples, tests, benches, .. } => + examples.is_specific() || tests.is_specific() || benches.is_specific() + } + } + pub fn matches(&self, target: &Target) -> bool { match *self { CompileFilter::Default { .. } => true, diff --git a/src/cargo/ops/mod.rs b/src/cargo/ops/mod.rs index 7c4a33d4ba2..5da0781ec87 100644 --- a/src/cargo/ops/mod.rs +++ b/src/cargo/ops/mod.rs @@ -22,7 +22,7 @@ pub use self::registry::{modify_owners, yank, OwnersOptions, PublishOpts}; pub use self::registry::configure_http_handle; pub use self::cargo_fetch::fetch; pub use self::cargo_pkgid::pkgid; -pub use self::resolve::{resolve_ws, resolve_ws_precisely, resolve_with_previous}; +pub use self::resolve::{resolve_ws, resolve_ws_precisely, resolve_ws_with_method, resolve_with_previous}; pub use self::cargo_output_metadata::{output_metadata, OutputMetadataOptions, ExportInfo}; mod cargo_clean; diff --git a/src/cargo/ops/resolve.rs b/src/cargo/ops/resolve.rs index 8a49de85dc4..7a2c3d48bca 100644 --- a/src/cargo/ops/resolve.rs +++ b/src/cargo/ops/resolve.rs @@ -29,13 +29,25 @@ pub fn resolve_ws_precisely<'a>(ws: &Workspace<'a>, no_default_features: bool, specs: &[PackageIdSpec]) -> CargoResult<(PackageSet<'a>, Resolve)> { - let features = features.iter() - .flat_map(|s| s.split_whitespace()) - .flat_map(|s| s.split(',')) - .filter(|s| !s.is_empty()) - .map(|s| s.to_string()) - .collect::>(); + let features = Method::split_features(features); + let method = if all_features { + Method::Everything + } else { + Method::Required { + dev_deps: true, + features: &features, + all_features: false, + uses_default_features: !no_default_features, + } + }; + resolve_ws_with_method(ws, source, method, specs) +} +pub fn resolve_ws_with_method<'a>(ws: &Workspace<'a>, + source: Option>, + method: Method, + specs: &[PackageIdSpec]) + -> CargoResult<(PackageSet<'a>, Resolve)> { let mut registry = PackageRegistry::new(ws.config())?; if let Some(source) = source { registry.add_preloaded(source); @@ -68,16 +80,6 @@ pub fn resolve_ws_precisely<'a>(ws: &Workspace<'a>, None }; - let method = if all_features { - Method::Everything - } else { - Method::Required { - dev_deps: true, // TODO: remove this option? - features: &features, - uses_default_features: !no_default_features, - } - }; - let resolved_with_overrides = ops::resolve_with_previous(&mut registry, ws, @@ -236,6 +238,7 @@ pub fn resolve_with_previous<'a>(registry: &mut PackageRegistry, let base = Method::Required { dev_deps: dev_deps, features: &[], + all_features: false, uses_default_features: true, }; let member_id = member.package_id(); From ee78456afbf56b27128955c90329552977ef0b24 Mon Sep 17 00:00:00 2001 From: Ximin Luo Date: Mon, 5 Feb 2018 16:15:13 +0100 Subject: [PATCH 2/7] Only avoid dev deps in `cargo install` and `cargo build --avoid-dev-deps` --- src/bin/build.rs | 7 ++++++- src/cargo/core/workspace.rs | 5 +++++ src/cargo/ops/cargo_compile.rs | 2 +- src/cargo/ops/cargo_install.rs | 6 +++++- 4 files changed, 17 insertions(+), 3 deletions(-) diff --git a/src/bin/build.rs b/src/bin/build.rs index 889052068e4..9b576e92205 100644 --- a/src/bin/build.rs +++ b/src/bin/build.rs @@ -12,6 +12,7 @@ pub struct Options { flag_features: Vec, flag_all_features: bool, flag_no_default_features: bool, + flag_avoid_dev_deps: bool, flag_target: Option, flag_manifest_path: Option, flag_verbose: u32, @@ -63,6 +64,7 @@ Options: --features FEATURES Space-separated list of features to also build --all-features Build all available features --no-default-features Do not build the `default` feature + --avoid-dev-deps Avoid installing dev-dependencies if possible --target TRIPLE Build for the target triple --manifest-path PATH Path to the manifest to compile -v, --verbose ... Use verbose output (-vv very verbose/build.rs output) @@ -98,7 +100,10 @@ pub fn execute(options: Options, config: &mut Config) -> CliResult { &options.flag_z)?; let root = find_root_manifest_for_wd(options.flag_manifest_path, config.cwd())?; - let ws = Workspace::new(&root, config)?; + let mut ws = Workspace::new(&root, config)?; + if options.flag_avoid_dev_deps { + ws.set_require_optional_deps(false); + } let spec = Packages::from_flags(options.flag_all, &options.flag_exclude, diff --git a/src/cargo/core/workspace.rs b/src/cargo/core/workspace.rs index 5d21dd8d41a..00e93a92743 100644 --- a/src/cargo/core/workspace.rs +++ b/src/cargo/core/workspace.rs @@ -300,6 +300,11 @@ impl<'cfg> Workspace<'cfg> { self.require_optional_deps } + pub fn set_require_optional_deps<'a>(&'a mut self, require_optional_deps: bool) -> &mut Workspace<'cfg> { + self.require_optional_deps = require_optional_deps; + self + } + /// Finds the root of a workspace for the crate whose manifest is located /// at `manifest_path`. /// diff --git a/src/cargo/ops/cargo_compile.rs b/src/cargo/ops/cargo_compile.rs index a212a64552e..bf905c3afd1 100644 --- a/src/cargo/ops/cargo_compile.rs +++ b/src/cargo/ops/cargo_compile.rs @@ -228,7 +228,7 @@ pub fn compile_ws<'a>(ws: &Workspace<'a>, let specs = spec.into_package_id_specs(ws)?; let features = Method::split_features(features); let method = Method::Required { - dev_deps: filter.need_dev_deps(), + dev_deps: ws.require_optional_deps() || filter.need_dev_deps(), features: &features, all_features, uses_default_features: !no_default_features, diff --git a/src/cargo/ops/cargo_install.rs b/src/cargo/ops/cargo_install.rs index c85923e453b..703a96da44f 100644 --- a/src/cargo/ops/cargo_install.rs +++ b/src/cargo/ops/cargo_install.rs @@ -175,7 +175,11 @@ fn install_one(root: &Filesystem, let ws = match overidden_target_dir { Some(dir) => Workspace::ephemeral(pkg, config, Some(dir), false)?, - None => Workspace::new(pkg.manifest_path(), config)?, + None => { + let mut ws = Workspace::new(pkg.manifest_path(), config)?; + ws.set_require_optional_deps(false); + ws + } }; let pkg = ws.current()?; From 3fc0715d33ccea567526242d2247576930db4a7c Mon Sep 17 00:00:00 2001 From: Ximin Luo Date: Mon, 5 Feb 2018 17:04:11 +0100 Subject: [PATCH 3/7] Make the default behaviour of `cargo build` match the documentation cargo build --help says: --all-targets Build all targets (lib and bin targets by default) but the old default behaviour was actually doing --all-targets. This meant that --avoid-dev-deps was only effectively if one gave --lib --bins explicitly. With this commit, `cargo build --avoid-dev-deps` with no other flags, will build lib and bins and avoid installing dev-dependencies. --- src/cargo/ops/cargo_compile.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/cargo/ops/cargo_compile.rs b/src/cargo/ops/cargo_compile.rs index bf905c3afd1..d7c08c9c456 100644 --- a/src/cargo/ops/cargo_compile.rs +++ b/src/cargo/ops/cargo_compile.rs @@ -421,7 +421,7 @@ impl<'a> CompileFilter<'a> { pub fn need_dev_deps(&self) -> bool { match *self { - CompileFilter::Default { .. } => true, + CompileFilter::Default { .. } => false, CompileFilter::Only { examples, tests, benches, .. } => examples.is_specific() || tests.is_specific() || benches.is_specific() } @@ -429,7 +429,11 @@ impl<'a> CompileFilter<'a> { pub fn matches(&self, target: &Target) -> bool { match *self { - CompileFilter::Default { .. } => true, + CompileFilter::Default { .. } => match *target.kind() { + TargetKind::Bin => true, + TargetKind::Lib(..) => true, + _ => false, + }, CompileFilter::Only { lib, bins, examples, tests, benches, .. } => { let rule = match *target.kind() { TargetKind::Bin => bins, From df5f7d688c7e6af2b2d445dfb04028ee9f41be08 Mon Sep 17 00:00:00 2001 From: Ximin Luo Date: Thu, 15 Feb 2018 15:31:12 +0100 Subject: [PATCH 4/7] Turn --avoid-dev-deps into a -Z unstable flag --- src/bin/build.rs | 4 +--- src/cargo/core/features.rs | 2 ++ src/cargo/core/workspace.rs | 3 ++- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/bin/build.rs b/src/bin/build.rs index 9b576e92205..b388c36f5ce 100644 --- a/src/bin/build.rs +++ b/src/bin/build.rs @@ -12,7 +12,6 @@ pub struct Options { flag_features: Vec, flag_all_features: bool, flag_no_default_features: bool, - flag_avoid_dev_deps: bool, flag_target: Option, flag_manifest_path: Option, flag_verbose: u32, @@ -64,7 +63,6 @@ Options: --features FEATURES Space-separated list of features to also build --all-features Build all available features --no-default-features Do not build the `default` feature - --avoid-dev-deps Avoid installing dev-dependencies if possible --target TRIPLE Build for the target triple --manifest-path PATH Path to the manifest to compile -v, --verbose ... Use verbose output (-vv very verbose/build.rs output) @@ -101,7 +99,7 @@ pub fn execute(options: Options, config: &mut Config) -> CliResult { let root = find_root_manifest_for_wd(options.flag_manifest_path, config.cwd())?; let mut ws = Workspace::new(&root, config)?; - if options.flag_avoid_dev_deps { + if config.cli_unstable().avoid_dev_deps { ws.set_require_optional_deps(false); } diff --git a/src/cargo/core/features.rs b/src/cargo/core/features.rs index 414e6a4a8b6..875f4e2dfa1 100644 --- a/src/cargo/core/features.rs +++ b/src/cargo/core/features.rs @@ -234,6 +234,7 @@ pub struct CliUnstable { pub unstable_options: bool, pub offline: bool, pub no_index_update: bool, + pub avoid_dev_deps: bool, } impl CliUnstable { @@ -266,6 +267,7 @@ impl CliUnstable { "unstable-options" => self.unstable_options = true, "offline" => self.offline = true, "no-index-update" => self.no_index_update = true, + "avoid-dev-deps" => self.avoid_dev_deps = true, _ => bail!("unknown `-Z` flag specified: {}", k), } diff --git a/src/cargo/core/workspace.rs b/src/cargo/core/workspace.rs index 00e93a92743..c2e1a3e4d62 100644 --- a/src/cargo/core/workspace.rs +++ b/src/cargo/core/workspace.rs @@ -64,7 +64,8 @@ pub struct Workspace<'cfg> { // True if this workspace should enforce optional dependencies even when // not needed; false if this workspace should only enforce dependencies - // needed by the current configuration (such as in cargo install). + // needed by the current configuration (such as in cargo install). In some + // cases `false` also results in the non-enforcement of dev-dependencies. require_optional_deps: bool, } From da41b4e0391f23d282981e8d0f2c700b3fe4972d Mon Sep 17 00:00:00 2001 From: Ximin Luo Date: Thu, 15 Feb 2018 16:01:36 +0100 Subject: [PATCH 5/7] Add some tests to check that dev-dependencies are not resolved --- tests/build.rs | 21 ++++++++++++++++++++ tests/install.rs | 51 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 72 insertions(+) diff --git a/tests/build.rs b/tests/build.rs index 14237e4c9b1..ee09ff0beef 100644 --- a/tests/build.rs +++ b/tests/build.rs @@ -4188,3 +4188,24 @@ fn no_linkable_target() { [WARNING] The package `the_lib` provides no linkable [..] \ while compiling `foo`. [..] in `the_lib`'s Cargo.toml. [..]")); } + +#[test] +fn avoid_dev_deps() { + Package::new("foo", "1.0.0").publish(); + let p = project("foo") + .file("Cargo.toml", r#" + [package] + name = "bar" + version = "0.1.0" + authors = [] + + [dev-dependencies] + baz = "1.0.0" + "#) + .file("src/main.rs", "fn main() {}") + .build(); + + assert_that(p.cargo("build"), execs().with_status(101)); + assert_that(p.cargo("build").masquerade_as_nightly_cargo() + .arg("-Zavoid-dev-deps"), execs().with_status(0)); +} diff --git a/tests/install.rs b/tests/install.rs index 1ae6806c447..14b64c6c623 100644 --- a/tests/install.rs +++ b/tests/install.rs @@ -6,6 +6,7 @@ use std::fs::{self, File, OpenOptions}; use std::io::prelude::*; use cargo::util::ProcessBuilder; +use cargotest::ChannelChanger; use cargotest::install::{cargo_home, has_installed_exe}; use cargotest::support::git; use cargotest::support::paths; @@ -907,6 +908,56 @@ fn use_path_workspace() { assert!(lock == lock2, "different lockfiles"); } +#[test] +fn dev_dependencies_no_check() { + Package::new("foo", "1.0.0").publish(); + let p = project("foo") + .file("Cargo.toml", r#" + [package] + name = "bar" + version = "0.1.0" + authors = [] + + [dev-dependencies] + baz = "1.0.0" + "#) + .file("src/main.rs", "fn main() {}") + .build(); + + assert_that(p.cargo("build"), execs().with_status(101)); + assert_that(p.cargo("install"), execs().with_status(0)); +} + +#[test] +fn dev_dependencies_lock_file_untouched() { + Package::new("foo", "1.0.0").publish(); + let p = project("foo") + .file("Cargo.toml", r#" + [package] + name = "foo" + version = "0.1.0" + authors = [] + + [dev-dependencies] + bar = { path = "a" } + "#) + .file("src/main.rs", "fn main() {}") + .file("a/Cargo.toml", r#" + [package] + name = "bar" + version = "0.1.0" + authors = [] + "#) + .file("a/src/lib.rs", "") + .build(); + + assert_that(p.cargo("build"), execs().with_status(0)); + let lock = p.read_lockfile(); + assert_that(p.cargo("install"), execs().with_status(0)); + let lock2 = p.read_lockfile(); + assert!(lock == lock2, "different lockfiles"); +} + #[test] fn vers_precise() { pkg("foo", "0.1.1"); From 70170d1bd3ddd015ad28dc99b51b715596a7a972 Mon Sep 17 00:00:00 2001 From: Ximin Luo Date: Tue, 6 Mar 2018 20:49:20 +0100 Subject: [PATCH 6/7] Revert "Make the default behaviour of `cargo build` match the documentation" This reverts commit 3fc0715d33ccea567526242d2247576930db4a7c. --- src/cargo/ops/cargo_compile.rs | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/cargo/ops/cargo_compile.rs b/src/cargo/ops/cargo_compile.rs index 4d343b230d8..484c0d32a33 100644 --- a/src/cargo/ops/cargo_compile.rs +++ b/src/cargo/ops/cargo_compile.rs @@ -422,7 +422,7 @@ impl<'a> CompileFilter<'a> { pub fn need_dev_deps(&self) -> bool { match *self { - CompileFilter::Default { .. } => false, + CompileFilter::Default { .. } => true, CompileFilter::Only { examples, tests, benches, .. } => examples.is_specific() || tests.is_specific() || benches.is_specific() } @@ -430,11 +430,7 @@ impl<'a> CompileFilter<'a> { pub fn matches(&self, target: &Target) -> bool { match *self { - CompileFilter::Default { .. } => match *target.kind() { - TargetKind::Bin => true, - TargetKind::Lib(..) => true, - _ => false, - }, + CompileFilter::Default { .. } => true, CompileFilter::Only { lib, bins, examples, tests, benches, .. } => { let rule = match *target.kind() { TargetKind::Bin => bins, From d46db71b3ff17dfc0f4be6308c8b94613d65a572 Mon Sep 17 00:00:00 2001 From: Ximin Luo Date: Wed, 7 Mar 2018 14:46:43 +0100 Subject: [PATCH 7/7] Work around #5134 for now --- tests/testsuite/build.rs | 10 +++++++--- tests/testsuite/install.rs | 14 ++++++++++---- 2 files changed, 17 insertions(+), 7 deletions(-) diff --git a/tests/testsuite/build.rs b/tests/testsuite/build.rs index e4237d864dc..669c17d4153 100644 --- a/tests/testsuite/build.rs +++ b/tests/testsuite/build.rs @@ -4307,7 +4307,11 @@ fn avoid_dev_deps() { .file("src/main.rs", "fn main() {}") .build(); - assert_that(p.cargo("build"), execs().with_status(101)); - assert_that(p.cargo("build").masquerade_as_nightly_cargo() - .arg("-Zavoid-dev-deps"), execs().with_status(0)); + // --bins is needed because of #5134 + assert_that(p.cargo("build").arg("--bins"), + execs().with_status(101)); + assert_that(p.cargo("build").arg("--bins") + .masquerade_as_nightly_cargo() + .arg("-Zavoid-dev-deps"), + execs().with_status(0)); } diff --git a/tests/testsuite/install.rs b/tests/testsuite/install.rs index bd58c6e6cd7..d8308ce5cb9 100644 --- a/tests/testsuite/install.rs +++ b/tests/testsuite/install.rs @@ -921,8 +921,11 @@ fn dev_dependencies_no_check() { .file("src/main.rs", "fn main() {}") .build(); - assert_that(p.cargo("build"), execs().with_status(101)); - assert_that(p.cargo("install"), execs().with_status(0)); + // --bins is needed because of #5134 + assert_that(p.cargo("build").arg("--bins"), + execs().with_status(101)); + assert_that(p.cargo("install").arg("--bins"), + execs().with_status(0)); } #[test] @@ -948,9 +951,12 @@ fn dev_dependencies_lock_file_untouched() { .file("a/src/lib.rs", "") .build(); - assert_that(p.cargo("build"), execs().with_status(0)); + // --bins is needed because of #5134 + assert_that(p.cargo("build").arg("--bins"), + execs().with_status(0)); let lock = p.read_lockfile(); - assert_that(p.cargo("install"), execs().with_status(0)); + assert_that(p.cargo("install").arg("--bins"), + execs().with_status(0)); let lock2 = p.read_lockfile(); assert!(lock == lock2, "different lockfiles"); }