From 2f88a70407499796f69b2f5241bf929cda01d903 Mon Sep 17 00:00:00 2001
From: Alex Crichton <alex@alexcrichton.com>
Date: Fri, 4 May 2018 09:19:30 -0700
Subject: [PATCH] Fix optional deps in multiple sections

This commit fixes an issue where an optional dependency was listed multiple
times in a manifest (multiple sections). This regression was introduced by #5415
and happened because in the resolver we didn't record a `Dependency` as it was
accidentally deduplicated too soon.

The fix here was to ensure that all `Dependency` annotations make their way into
`Resolve` now that we rely on the listed `Dependency` values for correctness.

Closes #5475
---
 src/cargo/core/resolver/context.rs | 33 +++++++++------
 tests/testsuite/build_script.rs    | 65 ++++++++++++++++++++++++++++++
 2 files changed, 86 insertions(+), 12 deletions(-)

diff --git a/src/cargo/core/resolver/context.rs b/src/cargo/core/resolver/context.rs
index a41f482c1..4b089dbe1 100644
--- a/src/cargo/core/resolver/context.rs
+++ b/src/cargo/core/resolver/context.rs
@@ -187,18 +187,23 @@ impl Context {
         } else {
             vec![]
         };
-        let mut reqs = build_requirements(s, method, &values)?;
+        let reqs = build_requirements(s, method, &values)?;
         let mut ret = Vec::new();
+        let mut used_features = HashSet::new();
+        let default_dep = (false, Vec::new());
 
         // Next, collect all actually enabled dependencies and their features.
         for dep in deps {
-            // Skip optional dependencies, but not those enabled through a feature
+            // Skip optional dependencies, but not those enabled through a
+            // feature
             if dep.is_optional() && !reqs.deps.contains_key(&*dep.name()) {
                 continue;
             }
-            // So we want this dependency.  Move the features we want from `feature_deps`
-            // to `ret`.
-            let base = reqs.deps.remove(&*dep.name()).unwrap_or((false, vec![]));
+            // So we want this dependency. Move the features we want from
+            // `feature_deps` to `ret` and register ourselves as using this
+            // name.
+            let base = reqs.deps.get(&*dep.name()).unwrap_or(&default_dep);
+            used_features.insert(dep.name().as_str());
             if !dep.is_optional() && base.0 {
                 self.warnings.push(format!(
                     "Package `{}` does not have feature `{}`. It has a required dependency \
@@ -209,7 +214,7 @@ impl Context {
                     dep.name()
                 ));
             }
-            let mut base = base.1;
+            let mut base = base.1.clone();
             base.extend(dep.features().iter());
             for feature in base.iter() {
                 if feature.contains('/') {
@@ -221,12 +226,16 @@ impl Context {
             ret.push((dep.clone(), base));
         }
 
-        // Any remaining entries in feature_deps are bugs in that the package does not actually
-        // have those dependencies.  We classified them as dependencies in the first place
-        // because there is no such feature, either.
-        if !reqs.deps.is_empty() {
-            let unknown = reqs.deps.keys().map(|s| &s[..]).collect::<Vec<&str>>();
-            let features = unknown.join(", ");
+        // Any entries in `reqs.dep` which weren't used are bugs in that the
+        // package does not actually have those dependencies. We classified
+        // them as dependencies in the first place because there is no such
+        // feature, either.
+        let remaining = reqs.deps.keys()
+            .cloned()
+            .filter(|s| !used_features.contains(s))
+            .collect::<Vec<_>>();
+        if !remaining.is_empty() {
+            let features = remaining.join(", ");
             return Err(match parent {
                 None => format_err!(
                     "Package `{}` does not have these features: `{}`",
diff --git a/tests/testsuite/build_script.rs b/tests/testsuite/build_script.rs
index b3cf382e2..1add770c2 100644
--- a/tests/testsuite/build_script.rs
+++ b/tests/testsuite/build_script.rs
@@ -3833,3 +3833,68 @@ fn rename_with_link_search_path() {
         ),
     );
 }
+
+#[test]
+fn optional_build_script_dep() {
+    let p = project("foo")
+        .file(
+            "Cargo.toml",
+            r#"
+                [project]
+                name = "foo"
+                version = "0.5.0"
+                authors = []
+
+                [dependencies]
+                bar = { path = "bar", optional = true }
+
+                [build-dependencies]
+                bar = { path = "bar", optional = true }
+            "#,
+        )
+        .file("build.rs", r#"
+            #[cfg(feature = "bar")]
+            extern crate bar;
+
+            fn main() {
+                #[cfg(feature = "bar")] {
+                    println!("cargo:rustc-env=FOO={}", bar::bar());
+                    return
+                }
+                println!("cargo:rustc-env=FOO=0");
+            }
+        "#)
+        .file(
+            "src/main.rs",
+            r#"
+                #[cfg(feature = "bar")]
+                extern crate bar;
+
+                fn main() {
+                    println!("{}", env!("FOO"));
+                }
+            "#,
+        )
+        .file(
+            "bar/Cargo.toml",
+            r#"
+                [project]
+                name = "bar"
+                version = "0.5.0"
+                authors = []
+            "#,
+        )
+        .file(
+            "bar/src/lib.rs",
+            r#"
+                pub fn bar() -> u32 { 1 }
+            "#,
+        );
+    let p = p.build();
+
+    assert_that(p.cargo("run"), execs().with_status(0).with_stdout("0\n"));
+    assert_that(
+        p.cargo("run --features bar"),
+        execs().with_status(0).with_stdout("1\n"),
+    );
+}
-- 
2.30.2