Auto merge of #5288 - alexcrichton:another-fix, r=matklad

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

Author: bors

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);
             }

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] [..]"),
+    );
+}