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

diff --git a/src/cargo/ops/resolve.rs b/src/cargo/ops/resolve.rs
index 470e36f48ba1d0ca957b575c0fc91c94fda49ac0..89b87715000905eaa9d60c93dbe6a866708e588b 100644 (file)
--- 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 246533821ae676d01b74966f4eff872a2ba2416b..597e19c5cb85ecfdc8da95e7bb2d2ee69c185b7b 100644 (file)
--- 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] [..]"),
+    );
+}