Auto merge of #3369 - joshtriplett:cargo-install-only-required-depend…

…encies, r=alexcrichton

cargo fails if it can't find optional dependencies, even if corresponding feature not enabled

I have a directory registry containing all the crate sources needed to build an application crate (for instance, ripgrep), and a `$CARGO_HOME/config` file that looks like this:

```toml
[source.crates-io]
replace-with = "dh-cargo-registry"

[source.dh-cargo-registry]
directory = "/usr/share/cargo/registry/"
```

When I attempt to build ripgrep via "cargo install ripgrep" from that directory registry, I get this error:

```
error: failed to compile `ripgrep v0.3.1`, intermediate artifacts can be found at `/tmp/cargo-install.rmKApOw9BwAL`

Caused by:
  no matching package named `simd` found (required by `bytecount`)
location searched: registry /~https://github.com/rust-lang/crates.io-index
version required: ^0.1.1
```

The directory registry indeed does not contain "simd"; however, bytecount doesn't require simd.  It has an optional dependency on simd, and nothing enables the feature that requires that dependency.

Placing the simd crate sources into the directory registry allows ripgrep to build; the resulting build does not actually build the simd crate.

I can reproduce this by just trying to build the "bytecount" crate directly, using the same `$CARGO_HOME`:

```
error: no matching package named `simd` found (required by `bytecount`)
location searched: registry /~https://github.com/rust-lang/crates.io-index
version required: = 0.1.1
```
(Incidentally, that "version required" seems wrong: bytecount has an optional dependency on simd `^0.1.1`, not `=0.1.1`.)

However, this doesn't seem consistent with other crates in the same dependency tree.  For instance, ripgrep also depends on clap, and clap has an optional dependency on yaml-rust, yet cargo does not complain about the missing yaml-rust.

I'd *guess* that the difference occurs because ripgrep has an optional feature `simd-accel` that depends on `bytecount/simd-accel`, so cargo wants to compute what packages it needs for that case too, even when building without that feature. (Similar to #3233.)

However, this makes it impossible to build a package while installing only the packaged dependencies for the enabled features.  Could `cargo install` ignore any dependencies not actually required by the enabled feature?  (That behavior would make no sense for "cargo build", which builds a Cargo.lock file that should remain consistent regardless of enabled features, but it makes sense for "cargo install cratename", which doesn't build a Cargo.lock file.)

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<Filesystem>)
-                     -> CargoResult<Workspace<'cfg>> {
+    pub fn ephemeral(package: Package, config: &'cfg Config, target_dir: Option<Filesystem>,
+                     require_optional_deps: bool) -> CargoResult<Workspace<'cfg>> {
         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`.
     ///

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()?;

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,

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: &[],

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};
@@ -104,6 +105,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();