From db71d878fb4909575338f9f433b613ba94257d8b Mon Sep 17 00:00:00 2001 From: Josh Triplett Date: Wed, 1 Mar 2017 18:19:43 -0800 Subject: [PATCH] In "cargo install" directly from registry, don't require optional dependencies When building with a directory registry that contains only the subset of crates required to build an application crate, cargo fails if that subset doesn't include optional dependencies pulled in for every possible feature of the root crate, even when the install doesn't enable those features. This prevents Linux distributions from building with a minimal set of dependencies (omitting, for instance, packages for unstable/nightly features). Introduce a new workspace flag "require_optional_deps", disabled for install and enabled for everything else. Skip the initial Method::Everything resolve in this case, and modify resolve_with_previous to support running a Method::Required resolve without a previous resolution. This also skips adding path overrides, as those won't make sense (and won't work) for an install directly from a registry. Introduce a set of tests for "cargo install" directly from a directory registry. --- src/cargo/core/workspace.rs | 15 +++- src/cargo/ops/cargo_install.rs | 2 +- src/cargo/ops/cargo_package.rs | 2 +- src/cargo/ops/resolve.rs | 28 ++++--- tests/directory.rs | 139 +++++++++++++++++++++++++++++++++ 5 files changed, 170 insertions(+), 16 deletions(-) diff --git a/src/cargo/core/workspace.rs b/src/cargo/core/workspace.rs index 8a4cb0165d0..00fd7da2920 100644 --- a/src/cargo/core/workspace.rs +++ b/src/cargo/core/workspace.rs @@ -44,6 +44,11 @@ pub struct Workspace<'cfg> { // True, if this is a temporary workspace created for the purposes of // cargo install or cargo package. is_ephemeral: bool, + + // 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). + require_optional_deps: bool, } // Separate structure for tracking loaded packages (to avoid loading anything @@ -99,6 +104,7 @@ impl<'cfg> Workspace<'cfg> { target_dir: target_dir, members: Vec::new(), is_ephemeral: false, + require_optional_deps: true, }; ws.root_manifest = ws.find_root(manifest_path)?; ws.find_members()?; @@ -115,8 +121,8 @@ impl<'cfg> Workspace<'cfg> { /// /// This is currently only used in niche situations like `cargo install` or /// `cargo package`. - pub fn ephemeral(package: Package, config: &'cfg Config, target_dir: Option) - -> CargoResult> { + pub fn ephemeral(package: Package, config: &'cfg Config, target_dir: Option, + require_optional_deps: bool) -> CargoResult> { let mut ws = Workspace { config: config, current_manifest: package.manifest_path().to_path_buf(), @@ -128,6 +134,7 @@ impl<'cfg> Workspace<'cfg> { target_dir: None, members: Vec::new(), is_ephemeral: true, + require_optional_deps: require_optional_deps, }; { let key = ws.current_manifest.parent().unwrap(); @@ -219,6 +226,10 @@ impl<'cfg> Workspace<'cfg> { self.is_ephemeral } + pub fn require_optional_deps(&self) -> bool { + self.require_optional_deps + } + /// Finds the root of a workspace for the crate whose manifest is located /// at `manifest_path`. /// diff --git a/src/cargo/ops/cargo_install.rs b/src/cargo/ops/cargo_install.rs index 5ad84e376ff..fddb3b1ec23 100644 --- a/src/cargo/ops/cargo_install.rs +++ b/src/cargo/ops/cargo_install.rs @@ -97,7 +97,7 @@ pub fn install(root: Option<&str>, }; let ws = match overidden_target_dir { - Some(dir) => Workspace::ephemeral(pkg, config, Some(dir))?, + Some(dir) => Workspace::ephemeral(pkg, config, Some(dir), false)?, None => Workspace::new(pkg.manifest_path(), config)?, }; let pkg = ws.current()?; diff --git a/src/cargo/ops/cargo_package.rs b/src/cargo/ops/cargo_package.rs index a93fc234ab5..510412c01b9 100644 --- a/src/cargo/ops/cargo_package.rs +++ b/src/cargo/ops/cargo_package.rs @@ -284,7 +284,7 @@ fn run_verify(ws: &Workspace, tar: &File, opts: &PackageOpts) -> CargoResult<()> let new_pkg = Package::new(new_manifest, &manifest_path); // Now that we've rewritten all our path dependencies, compile it! - let ws = Workspace::ephemeral(new_pkg, config, None)?; + let ws = Workspace::ephemeral(new_pkg, config, None, true)?; ops::compile_ws(&ws, None, &ops::CompileOptions { config: config, jobs: opts.jobs, diff --git a/src/cargo/ops/resolve.rs b/src/cargo/ops/resolve.rs index c4caf63e0d6..03825e13e0c 100644 --- a/src/cargo/ops/resolve.rs +++ b/src/cargo/ops/resolve.rs @@ -37,16 +37,22 @@ pub fn resolve_ws_precisely<'a>(ws: &Workspace<'a>, registry.add_preloaded(source); } - // First, resolve the root_package's *listed* dependencies, as well as - // downloading and updating all remotes and such. - let resolve = resolve_with_registry(ws, &mut registry)?; + let resolve = if ws.require_optional_deps() { + // First, resolve the root_package's *listed* dependencies, as well as + // downloading and updating all remotes and such. + let resolve = resolve_with_registry(ws, &mut registry)?; + + // Second, resolve with precisely what we're doing. Filter out + // transitive dependencies if necessary, specify features, handle + // overrides, etc. + let _p = profile::start("resolving w/ overrides..."); - // Second, resolve with precisely what we're doing. Filter out - // transitive dependencies if necessary, specify features, handle - // overrides, etc. - let _p = profile::start("resolving w/ overrides..."); + add_overrides(&mut registry, ws)?; - add_overrides(&mut registry, ws)?; + Some(resolve) + } else { + None + }; let method = if all_features { Method::Everything @@ -60,7 +66,7 @@ pub fn resolve_ws_precisely<'a>(ws: &Workspace<'a>, let resolved_with_overrides = ops::resolve_with_previous(&mut registry, ws, - method, Some(&resolve), None, + method, resolve.as_ref(), None, specs)?; for &(ref replace_spec, _) in ws.root_replace() { @@ -159,8 +165,7 @@ pub fn resolve_with_previous<'a>(registry: &mut PackageRegistry, // members in the workspace, so propagate the `Method::Everything`. Method::Everything => Method::Everything, - // If we're not resolving everything though then the workspace is - // already resolved and now we're drilling down from that to the + // 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. @@ -176,7 +181,6 @@ pub fn resolve_with_previous<'a>(registry: &mut PackageRegistry, // base method with no features specified but using default features // for any other packages specified with `-p`. Method::Required { dev_deps, .. } => { - assert!(previous.is_some()); let base = Method::Required { dev_deps: dev_deps, features: &[], diff --git a/tests/directory.rs b/tests/directory.rs index 9d6dcf1477f..433d6eb56ad 100644 --- a/tests/directory.rs +++ b/tests/directory.rs @@ -10,6 +10,7 @@ use std::str; use rustc_serialize::json; +use cargotest::cargo_process; use cargotest::support::{project, execs, ProjectBuilder}; use cargotest::support::paths; use cargotest::support::registry::{Package, cksum}; @@ -105,6 +106,144 @@ fn simple() { ")); } +#[test] +fn simple_install() { + setup(); + + VendorPackage::new("foo") + .file("Cargo.toml", r#" + [package] + name = "foo" + version = "0.1.0" + authors = [] + "#) + .file("src/lib.rs", "pub fn foo() {}") + .build(); + + VendorPackage::new("bar") + .file("Cargo.toml", r#" + [package] + name = "bar" + version = "0.1.0" + authors = [] + + [dependencies] + foo = "0.1.0" + "#) + .file("src/main.rs", r#" + extern crate foo; + + pub fn main() { + foo::foo(); + } + "#) + .build(); + + assert_that(cargo_process().arg("install").arg("bar"), + execs().with_status(0).with_stderr( +" Installing bar v0.1.0 + Compiling foo v0.1.0 + Compiling bar v0.1.0 + Finished release [optimized] target(s) in [..] secs + Installing [..]bar[..] +warning: be sure to add `[..]` to your PATH to be able to run the installed binaries +")); +} + +#[test] +fn simple_install_fail() { + setup(); + + VendorPackage::new("foo") + .file("Cargo.toml", r#" + [package] + name = "foo" + version = "0.1.0" + authors = [] + "#) + .file("src/lib.rs", "pub fn foo() {}") + .build(); + + VendorPackage::new("bar") + .file("Cargo.toml", r#" + [package] + name = "bar" + version = "0.1.0" + authors = [] + + [dependencies] + foo = "0.1.0" + baz = "9.8.7" + "#) + .file("src/main.rs", r#" + extern crate foo; + + pub fn main() { + foo::foo(); + } + "#) + .build(); + + assert_that(cargo_process().arg("install").arg("bar"), + execs().with_status(101).with_stderr( +" Installing bar v0.1.0 +error: failed to compile `bar v0.1.0`, intermediate artifacts can be found at `[..]` + +Caused by: + no matching package named `baz` found (required by `bar`) +location searched: registry https://github.com/rust-lang/crates.io-index +version required: ^9.8.7 +")); +} + +#[test] +fn install_without_feature_dep() { + setup(); + + VendorPackage::new("foo") + .file("Cargo.toml", r#" + [package] + name = "foo" + version = "0.1.0" + authors = [] + "#) + .file("src/lib.rs", "pub fn foo() {}") + .build(); + + VendorPackage::new("bar") + .file("Cargo.toml", r#" + [package] + name = "bar" + version = "0.1.0" + authors = [] + + [dependencies] + foo = "0.1.0" + baz = { version = "9.8.7", optional = true } + + [features] + wantbaz = ["baz"] + "#) + .file("src/main.rs", r#" + extern crate foo; + + pub fn main() { + foo::foo(); + } + "#) + .build(); + + assert_that(cargo_process().arg("install").arg("bar"), + execs().with_status(0).with_stderr( +" Installing bar v0.1.0 + Compiling foo v0.1.0 + Compiling bar v0.1.0 + Finished release [optimized] target(s) in [..] secs + Installing [..]bar[..] +warning: be sure to add `[..]` to your PATH to be able to run the installed binaries +")); +} + #[test] fn not_there() { setup();