]> git.proxmox.com Git - cargo.git/commitdiff
Warn about path overrides that won't work
authorAlex Crichton <alex@alexcrichton.com>
Thu, 29 Sep 2016 23:35:22 +0000 (16:35 -0700)
committerAlex Crichton <alex@alexcrichton.com>
Thu, 29 Sep 2016 23:51:12 +0000 (16:51 -0700)
Cargo has a long-standing [bug] in path overrides where they will cause spurious
rebuilds of crates in the crate graph. This can be very difficult to diagnose
and be incredibly frustrating as well. Unfortunately, though, it's behavior that
fundamentally can't be fixed in Cargo.

The crux of the problem happens when a `path` replacement changes something
about the list of dependencies of the crate that it's replacing. This alteration
to the list of dependencies *cannot be tracked by Cargo* as the lock file was
previously emitted. In the best case this ends up causing random recompiles. In
the worst case it cause infinite registry updates that always result in
recompiles.

A solution to this intention, changing the dependency graph of an overridden
dependency, was [implemented] with the `[replace]` feature in Cargo awhile back.
With that in mind, this commit implements a *warning* whenever a bad dependency
replacement is detected. The message here is pretty wordy, but it's intended to
convey that you should switch to using `[replace]` for a more robust
impelmentation, and it can also give security to anyone using `path` overrides
that if they get past this warning everything should work as intended.

[bug]: https://github.com/rust-lang/cargo/issues/2041
[implemented]: http://doc.crates.io/specifying-dependencies.html#overriding-dependencies

Closes #2041

src/cargo/core/registry.rs
src/cargo/sources/config.rs
tests/overrides.rs

index 90a1b76d244aee516481ded3f389118c69c153d3..ecaac98157bde33afd9d05c4ead761bce77d4d11 100644 (file)
@@ -1,4 +1,4 @@
-use std::collections::{HashSet, HashMap};
+use std::collections::HashMap;
 
 use core::{Source, SourceId, SourceMap, Summary, Dependency, PackageId, Package};
 use core::PackageSet;
@@ -188,17 +188,16 @@ impl<'cfg> PackageRegistry<'cfg> {
     }
 
     fn query_overrides(&mut self, dep: &Dependency)
-                       -> CargoResult<Vec<Summary>> {
-        let mut seen = HashSet::new();
-        let mut ret = Vec::new();
+                       -> CargoResult<Option<Summary>> {
         for s in self.overrides.iter() {
             let src = self.sources.get_mut(s).unwrap();
             let dep = Dependency::new_override(dep.name(), s);
-            ret.extend(try!(src.query(&dep)).into_iter().filter(|s| {
-                seen.insert(s.name().to_string())
-            }));
+            let mut results = try!(src.query(&dep));
+            if results.len() > 0 {
+                return Ok(Some(results.remove(0)))
+            }
         }
-        Ok(ret)
+        Ok(None)
     }
 
     // This function is used to transform a summary to another locked summary if
@@ -277,25 +276,89 @@ impl<'cfg> PackageRegistry<'cfg> {
             }
         })
     }
+
+    fn warn_bad_override(&self,
+                         override_summary: &Summary,
+                         real_summary: &Summary) -> CargoResult<()> {
+        let real = real_summary.package_id();
+        let map = try!(self.locked.get(real.source_id()).chain_error(|| {
+            human(format!("failed to find lock source of {}", real))
+        }));
+        let list = try!(map.get(real.name()).chain_error(|| {
+            human(format!("failed to find lock name of {}", real))
+        }));
+        let &(_, ref real_deps) = try!(list.iter().find(|&&(ref id, _)| {
+            real == id
+        }).chain_error(|| {
+            human(format!("failed to find lock version of {}", real))
+        }));
+        let mut real_deps = real_deps.clone();
+
+        let boilerplate = "\
+This is currently allowed but is known to produce buggy behavior with spurious
+recompiles and changes to the crate graph. Path overrides unfortunately were
+never intended to support this feature, so for now this message is just a
+warning. In the future, however, this message will become a hard error.
+
+To change the dependency graph via an override it's recommended to use the
+`[replace]` feature of Cargo instead of the path override feature. This is
+documented online at the url below for more information.
+
+http://doc.crates.io/specifying-dependencies.html#overriding-dependencies
+";
+
+        for dep in override_summary.dependencies() {
+            if let Some(i) = real_deps.iter().position(|id| dep.matches_id(id)) {
+                real_deps.remove(i);
+                continue
+            }
+            let msg = format!("\
+                path override for crate `{}` has altered the original list of\n\
+                dependencies; the dependency on `{}` was either added or\n\
+                modified to not match the previously resolved version\n\n\
+                {}", override_summary.package_id().name(), dep.name(), boilerplate);
+            try!(self.source_config.config().shell().warn(&msg));
+            return Ok(())
+        }
+
+        for id in real_deps {
+            let msg = format!("\
+                path override for crate `{}` has altered the original list of
+                dependencies; the dependency on `{}` was removed\n\n
+                {}", override_summary.package_id().name(), id.name(), boilerplate);
+            try!(self.source_config.config().shell().warn(&msg));
+            return Ok(())
+        }
+
+        Ok(())
+    }
 }
 
 impl<'cfg> Registry for PackageRegistry<'cfg> {
     fn query(&mut self, dep: &Dependency) -> CargoResult<Vec<Summary>> {
-        let overrides = try!(self.query_overrides(&dep));
-
-        let ret = if overrides.is_empty() {
-            // Ensure the requested source_id is loaded
-            try!(self.ensure_loaded(dep.source_id(), Kind::Normal).chain_error(|| {
-                human(format!("failed to load source for a dependency \
-                               on `{}`", dep.name()))
-            }));
-
-            match self.sources.get_mut(dep.source_id()) {
-                Some(src) => try!(src.query(&dep)),
-                None => Vec::new(),
+        // Ensure the requested source_id is loaded
+        try!(self.ensure_loaded(dep.source_id(), Kind::Normal).chain_error(|| {
+            human(format!("failed to load source for a dependency \
+                           on `{}`", dep.name()))
+        }));
+
+        let override_summary = try!(self.query_overrides(&dep));
+        let real_summaries = match self.sources.get_mut(dep.source_id()) {
+            Some(src) => Some(try!(src.query(&dep))),
+            None => None,
+        };
+
+        let ret = match (override_summary, real_summaries) {
+            (Some(candidate), Some(summaries)) => {
+                if summaries.len() != 1 {
+                    bail!("found an override with a non-locked list");
+                }
+                try!(self.warn_bad_override(&candidate, &summaries[0]));
+                vec![candidate]
             }
-        } else {
-            overrides
+            (Some(_), None) => bail!("override found but no real ones"),
+            (None, Some(summaries)) => summaries,
+            (None, None) => Vec::new(),
         };
 
         // post-process all returned summaries to ensure that we lock all
index 01205ff51b7e22b88c1bcb5406698a7225e305a6..ac254f147ecb9f19f7d033b4b90d341ffaebb482 100644 (file)
@@ -62,6 +62,10 @@ impl<'cfg> SourceConfigMap<'cfg> {
         Ok(base)
     }
 
+    pub fn config(&self) -> &'cfg Config {
+        self.config
+    }
+
     pub fn load(&self, id: &SourceId) -> CargoResult<Box<Source + 'cfg>> {
         debug!("loading: {}", id);
         let mut name = match self.id2name.get(id) {
index 5eebed0bfa134785a074548213627c07e30934e4..7ae8538efc889abec45d65a736b923959c67dca0 100644 (file)
@@ -682,3 +682,70 @@ fn no_override_self() {
     assert_that(p.cargo_process("build").arg("--verbose"),
                 execs().with_status(0));
 }
+
+#[test]
+fn broken_path_override_warns() {
+    Package::new("foo", "0.1.0").publish();
+    Package::new("foo", "0.2.0").publish();
+
+    let p = project("local")
+        .file("Cargo.toml", r#"
+            [package]
+            name = "local"
+            version = "0.0.1"
+            authors = []
+
+            [dependencies]
+            a = { path = "a1" }
+        "#)
+        .file("src/lib.rs", "")
+        .file("a1/Cargo.toml", r#"
+            [package]
+            name = "a"
+            version = "0.0.1"
+            authors = []
+
+            [dependencies]
+            foo = "0.1"
+        "#)
+        .file("a1/src/lib.rs", "")
+        .file("a2/Cargo.toml", r#"
+            [package]
+            name = "a"
+            version = "0.0.1"
+            authors = []
+
+            [dependencies]
+            foo = "0.2"
+        "#)
+        .file("a2/src/lib.rs", "")
+        .file(".cargo/config", r#"
+            paths = ["a2"]
+        "#);
+
+    assert_that(p.cargo_process("build"),
+                execs().with_status(0)
+                       .with_stderr("\
+[UPDATING] [..]
+warning: path override for crate `a` has altered the original list of
+dependencies; the dependency on `foo` was either added or
+modified to not match the previously resolved version
+
+This is currently allowed but is known to produce buggy behavior with spurious
+recompiles and changes to the crate graph. Path overrides unfortunately were
+never intended to support this feature, so for now this message is just a
+warning. In the future, however, this message will become a hard error.
+
+To change the dependency graph via an override it's recommended to use the
+`[replace]` feature of Cargo instead of the path override feature. This is
+documented online at the url below for more information.
+
+http://doc.crates.io/specifying-dependencies.html#overriding-dependencies
+
+[DOWNLOADING] [..]
+[COMPILING] [..]
+[COMPILING] [..]
+[COMPILING] [..]
+[FINISHED] [..]
+"));
+}