From 0790db9888cdd160cd2a87414de62266e19c3243 Mon Sep 17 00:00:00 2001
From: Alex Crichton <alex@alexcrichton.com>
Date: Tue, 3 Apr 2018 11:42:44 -0700
Subject: [PATCH] Less aggressively poison sources on builds

Discovered in #5257 the changes in #5215 were slightly too aggressively
poisoning sources to require updates, thinking that a manifest changed when it
actually hadn't.

Non-workspace-member path dependencies with optional/dev-dependencies
don't show up in the lock file, so the previous logic would recognize this and
think that the dependency missing from the lock file was just added and would
require a registry update.

The fix in this commit effectively just skips all of these dependencies in
non-workspace members. This means that this will be slightly buggy if an
optional dependency that's activated is added, but that's hopefully something we
can tackle later.

Closes #5257
---
 src/cargo/ops/resolve.rs     | 24 +++++++++++++--
 tests/testsuite/freshness.rs | 58 ++++++++++++++++++++++++++++++++++++
 2 files changed, 79 insertions(+), 3 deletions(-)

diff --git a/src/cargo/ops/resolve.rs b/src/cargo/ops/resolve.rs
index 470e36f48ba..89b87715000 100644
--- a/src/cargo/ops/resolve.rs
+++ b/src/cargo/ops/resolve.rs
@@ -455,12 +455,30 @@ fn register_previous_locks<'a>(
             }
 
             // If we match *anything* in the dependency graph then we consider
-            // ourselves A-OK and assume that we'll resolve to that. If,
-            // however, nothing matches, then we poison the source of this
-            // dependencies and the previous lock file.
+            // ourselves A-OK and assume that we'll resolve to that.
             if resolve.iter().any(|id| dep.matches_ignoring_source(id)) {
                 continue;
             }
+
+            // If this dependency didn't match anything special then we may want
+            // to poison the source as it may have been added. If this path
+            // dependencies is *not* a workspace member, however, and it's an
+            // optional/non-transitive dependency then it won't be necessarily
+            // be in our lock file. If this shows up then we avoid poisoning
+            // this source as otherwise we'd repeatedly update the registry.
+            //
+            // TODO: this breaks adding an optional dependency in a
+            //       non-workspace member and then simultaneously editing the
+            //       dependency on that crate to enable the feature. For now
+            //       this bug is better than the always updating registry
+            //       though...
+            if !ws.members().any(|pkg| pkg.package_id() == member.package_id()) &&
+                (dep.is_optional() || !dep.is_transitive()) {
+                continue
+            }
+
+            // Ok if nothing matches, then we poison the source of this
+            // dependencies and the previous lock file.
             for id in resolve.iter().filter(|id| id.source_id() == source) {
                 add_deps(resolve, id, &mut avoid_locking);
             }
diff --git a/tests/testsuite/freshness.rs b/tests/testsuite/freshness.rs
index 246533821ae..597e19c5cb8 100644
--- a/tests/testsuite/freshness.rs
+++ b/tests/testsuite/freshness.rs
@@ -4,6 +4,7 @@ use std::io::prelude::*;
 use cargotest::sleep_ms;
 use cargotest::support::{execs, project, path2url};
 use cargotest::support::paths::CargoPathExt;
+use cargotest::support::registry::Package;
 use hamcrest::{assert_that, existing_file};
 
 #[test]
@@ -1007,3 +1008,60 @@ fn no_rebuild_when_rename_dir() {
             .with_stderr("[FINISHED] dev [unoptimized + debuginfo] target(s) in [..]"),
     );
 }
+
+#[test]
+fn unused_optional_dep() {
+    Package::new("registry1", "0.1.0").publish();
+    Package::new("registry2", "0.1.0").publish();
+    Package::new("registry3", "0.1.0").publish();
+
+    let p = project("p")
+        .file(
+            "Cargo.toml",
+            r#"
+                [package]
+                name = "p"
+                authors = []
+                version = "0.1.0"
+
+                [dependencies]
+                foo = { path = "foo" }
+                bar = { path = "bar" }
+                registry1 = "*"
+            "#,
+        )
+        .file("src/lib.rs", "")
+        .file(
+            "foo/Cargo.toml",
+            r#"
+                [package]
+                name = "foo"
+                version = "0.1.1"
+                authors = []
+
+                [dev-dependencies]
+                registry2 = "*"
+            "#,
+        )
+        .file("foo/src/lib.rs", "")
+        .file(
+            "bar/Cargo.toml",
+            r#"
+                [package]
+                name = "bar"
+                version = "0.1.1"
+                authors = []
+
+                [dependencies]
+                registry3 = { version = "*", optional = true }
+            "#,
+        )
+        .file("bar/src/lib.rs", "")
+        .build();
+
+    assert_that(p.cargo("build"), execs().with_status(0));
+    assert_that(
+        p.cargo("build"),
+        execs().with_status(0).with_stderr("[FINISHED] [..]"),
+    );
+}