Auto merge of #9196 - ehuss:backports, r=alexcrichton

[beta] backports for 1.51

Beta backports for the following:

* Fix panic with doc collision orphan. (#9142)
    * This is an important regression that is fairly easy to hit.
* Do not exit prematurely if anything failed installing. (#9185)
    * This is not a regression, but I think an important fix.
* Add schema field to the index (#9161)
    * This is only the first commit from the PR which checks for the `v` field in the index, and skips entries that are not understood. The reason to backport is to get this in as early as possible so that if we do decide to start using it in the future, it works as early as possible.  This otherwise doesn't do anything, so I think it should be safe.
* Fix warnings of the new non_fmt_panic lint (#9148)
    * Fixes CI for a new warning in nightly.

crates/cargo-test-support/src/cross_compile.rs

@@ -177,7 +177,7 @@ rustup does not appear to be installed. Make sure that the appropriate
         },
     }
 
-    panic!(message);
+    panic!("{}", message);
 }
 
 /// The alternate target-triple to build with.

crates/cargo-test-support/src/registry.rs

@@ -327,6 +327,7 @@ pub struct Package {
     links: Option<String>,
     rust_version: Option<String>,
     cargo_features: Vec<String>,
+    v: Option<u32>,
 }
 
 #[derive(Clone)]
@@ -401,6 +402,7 @@ impl Package {
             links: None,
             rust_version: None,
             cargo_features: Vec::new(),
+            v: None,
         }
     }
 
@@ -554,6 +556,14 @@ impl Package {
         self
     }
 
+    /// Sets the index schema version for this package.
+    ///
+    /// See [`cargo::sources::registry::RegistryPackage`] for more information.
+    pub fn schema_version(&mut self, version: u32) -> &mut Package {
+        self.v = Some(version);
+        self
+    }
+
     /// Creates the package and place it in the registry.
     ///
     /// This does not actually use Cargo's publishing system, but instead
@@ -599,16 +609,19 @@ impl Package {
         } else {
             serde_json::json!(self.name)
         };
-        let line = serde_json::json!({
+        let mut json = serde_json::json!({
             "name": name,
             "vers": self.vers,
             "deps": deps,
             "cksum": cksum,
             "features": self.features,
             "yanked": self.yanked,
             "links": self.links,
-        })
-        .to_string();
+        });
+        if let Some(v) = self.v {
+            json["v"] = serde_json::json!(v);
+        }
+        let line = json.to_string();
 
         let file = match self.name.len() {
             1 => format!("1/{}", self.name),

crates/crates-io/lib.rs

@@ -56,6 +56,8 @@ pub struct NewCrate {
     pub repository: Option<String>,
     pub badges: BTreeMap<String, BTreeMap<String, String>>,
     pub links: Option<String>,
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub v: Option<u32>,
 }
 
 #[derive(Serialize)]

src/cargo/ops/cargo_compile.rs

@@ -507,7 +507,7 @@ pub fn create_bcx<'a, 'cfg>(
     // TODO: In theory, Cargo should also dedupe the roots, but I'm uncertain
     // what heuristics to use in that case.
     if build_config.mode == (CompileMode::Doc { deps: true }) {
-        remove_duplicate_doc(build_config, &mut unit_graph);
+        remove_duplicate_doc(build_config, &units, &mut unit_graph);
     }
 
     if build_config
@@ -1508,14 +1508,11 @@ fn opt_patterns_and_names(
 /// - Different sources. See `collision_doc_sources` test.
 ///
 /// Ideally this would not be necessary.
-fn remove_duplicate_doc(build_config: &BuildConfig, unit_graph: &mut UnitGraph) {
-    // NOTE: There is some risk that this can introduce problems because it
-    // may create orphans in the unit graph (parts of the tree get detached
-    // from the roots). I currently can't think of any ways this will cause a
-    // problem because all other parts of Cargo traverse the graph starting
-    // from the roots. Perhaps this should scan for detached units and remove
-    // them too?
-    //
+fn remove_duplicate_doc(
+    build_config: &BuildConfig,
+    root_units: &[Unit],
+    unit_graph: &mut UnitGraph,
+) {
     // First, create a mapping of crate_name -> Unit so we can see where the
     // duplicates are.
     let mut all_docs: HashMap<String, Vec<Unit>> = HashMap::new();
@@ -1601,4 +1598,18 @@ fn remove_duplicate_doc(build_config: &BuildConfig, unit_graph: &mut UnitGraph)
     for unit_deps in unit_graph.values_mut() {
         unit_deps.retain(|unit_dep| !removed_units.contains(&unit_dep.unit));
     }
+    // Remove any orphan units that were detached from the graph.
+    let mut visited = HashSet::new();
+    fn visit(unit: &Unit, graph: &UnitGraph, visited: &mut HashSet<Unit>) {
+        if !visited.insert(unit.clone()) {
+            return;
+        }
+        for dep in &graph[unit] {
+            visit(&dep.unit, graph, visited);
+        }
+    }
+    for unit in root_units {
+        visit(unit, unit_graph, &mut visited);
+    }
+    unit_graph.retain(|unit, _| visited.contains(unit));
 }

src/cargo/ops/cargo_install.rs

@@ -119,17 +119,15 @@ pub fn install(
         // able to run these commands.
         let dst = root.join("bin").into_path_unlocked();
         let path = env::var_os("PATH").unwrap_or_default();
-        for path in env::split_paths(&path) {
-            if path == dst {
-                return Ok(());
-            }
-        }
+        let dst_in_path = env::split_paths(&path).any(|path| path == dst);
 
-        config.shell().warn(&format!(
-            "be sure to add `{}` to your PATH to be \
+        if !dst_in_path {
+            config.shell().warn(&format!(
+                "be sure to add `{}` to your PATH to be \
              able to run the installed binaries",
-            dst.display()
-        ))?;
+                dst.display()
+            ))?;
+        }
     }
 
     if scheduled_error {

src/cargo/ops/registry.rs

@@ -305,6 +305,7 @@ fn transmit(
             license_file: license_file.clone(),
             badges: badges.clone(),
             links: links.clone(),
+            v: None,
         },
         tarball,
     );

src/cargo/sources/registry/index.rs

@@ -72,7 +72,8 @@ use crate::sources::registry::{RegistryData, RegistryPackage};
 use crate::util::interning::InternedString;
 use crate::util::paths;
 use crate::util::{internal, CargoResult, Config, Filesystem, ToSemver};
-use log::info;
+use anyhow::bail;
+use log::{debug, info};
 use semver::{Version, VersionReq};
 use std::collections::{HashMap, HashSet};
 use std::fs;
@@ -233,6 +234,8 @@ enum MaybeIndexSummary {
 pub struct IndexSummary {
     pub summary: Summary,
     pub yanked: bool,
+    /// Schema version, see [`RegistryPackage`].
+    v: u32,
 }
 
 /// A representation of the cache on disk that Cargo maintains of summaries.
@@ -305,6 +308,7 @@ impl<'cfg> RegistryIndex<'cfg> {
         // minimize the amount of work being done here and parse as little as
         // necessary.
         let raw_data = &summaries.raw_data;
+        let max_version = 1;
         Ok(summaries
             .versions
             .iter_mut()
@@ -318,6 +322,19 @@ impl<'cfg> RegistryIndex<'cfg> {
                     }
                 },
             )
+            .filter(move |is| {
+                if is.v > max_version {
+                    debug!(
+                        "unsupported schema version {} ({} {})",
+                        is.v,
+                        is.summary.name(),
+                        is.summary.version()
+                    );
+                    false
+                } else {
+                    true
+                }
+            })
             .filter(move |is| {
                 is.summary
                     .unstable_gate(namespaced_features, weak_dep_features)
@@ -578,7 +595,14 @@ impl Summaries {
         // actually happens to verify that our cache is indeed fresh and
         // computes exactly the same value as before.
         if cfg!(debug_assertions) && cache_contents.is_some() {
-            assert_eq!(cache_bytes, cache_contents);
+            if cache_bytes != cache_contents {
+                panic!(
+                    "original cache contents:\n{:?}\n\
+                     does not equal new cache contents:\n{:?}\n",
+                    cache_contents.as_ref().map(|s| String::from_utf8_lossy(s)),
+                    cache_bytes.as_ref().map(|s| String::from_utf8_lossy(s)),
+                );
+            }
         }
 
         // Once we have our `cache_bytes` which represents the `Summaries` we're
@@ -659,19 +683,19 @@ impl<'a> SummariesCache<'a> {
             .split_first()
             .ok_or_else(|| anyhow::format_err!("malformed cache"))?;
         if *first_byte != CURRENT_CACHE_VERSION {
-            anyhow::bail!("looks like a different Cargo's cache, bailing out");
+            bail!("looks like a different Cargo's cache, bailing out");
         }
         let mut iter = split(rest, 0);
         if let Some(update) = iter.next() {
             if update != last_index_update.as_bytes() {
-                anyhow::bail!(
+                bail!(
                     "cache out of date: current index ({}) != cache ({})",
                     last_index_update,
                     str::from_utf8(update)?,
                 )
             }
         } else {
-            anyhow::bail!("malformed file");
+            bail!("malformed file");
         }
         let mut ret = SummariesCache::default();
         while let Some(version) = iter.next() {
@@ -749,7 +773,9 @@ impl IndexSummary {
             features,
             yanked,
             links,
+            v,
         } = serde_json::from_slice(line)?;
+        let v = v.unwrap_or(1);
         log::trace!("json parsed registry {}/{}", name, vers);
         let pkgid = PackageId::new(name, &vers, source_id)?;
         let deps = deps
@@ -761,6 +787,7 @@ impl IndexSummary {
         Ok(IndexSummary {
             summary,
             yanked: yanked.unwrap_or(false),
+            v,
         })
     }
 }

src/cargo/sources/registry/mod.rs

@@ -269,6 +269,24 @@ pub struct RegistryPackage<'a> {
     /// Added early 2018 (see <https://github.com/rust-lang/cargo/pull/4978>),
     /// can be `None` if published before then.
     links: Option<InternedString>,
+    /// The schema version for this entry.
+    ///
+    /// If this is None, it defaults to version 1. Entries with unknown
+    /// versions are ignored.
+    ///
+    /// This provides a method to safely introduce changes to index entries
+    /// and allow older versions of cargo to ignore newer entries it doesn't
+    /// understand. This is honored as of 1.51, so unfortunately older
+    /// versions will ignore it, and potentially misinterpret version 1 and
+    /// newer entries.
+    ///
+    /// The intent is that versions older than 1.51 will work with a
+    /// pre-existing `Cargo.lock`, but they may not correctly process `cargo
+    /// update` or build a lock from scratch. In that case, cargo may
+    /// incorrectly select a new package that uses a new index format. A
+    /// workaround is to downgrade any packages that are incompatible with the
+    /// `--precise` flag of `cargo update`.
+    v: Option<u32>,
 }
 
 #[test]

tests/testsuite/cargo_command.rs

@@ -365,5 +365,5 @@ fn closed_output_ok() {
         .unwrap();
     let status = child.wait().unwrap();
     assert!(status.success());
-    assert!(s.is_empty(), s);
+    assert!(s.is_empty(), "{}", s);
 }

tests/testsuite/collisions.rs

@@ -3,9 +3,8 @@
 //! Ideally these should never happen, but I don't think we'll ever be able to
 //! prevent all collisions.
 
-use cargo_test_support::basic_manifest;
-use cargo_test_support::project;
 use cargo_test_support::registry::Package;
+use cargo_test_support::{basic_manifest, cross_compile, project};
 use std::env;
 
 #[cargo_test]
@@ -431,3 +430,51 @@ the same path; see <https://github.com/rust-lang/cargo/issues/6313>.
         )
         .run();
 }
+
+#[cargo_test]
+fn collision_doc_target() {
+    // collision in doc with --target, doesn't fail due to orphans
+    if cross_compile::disabled() {
+        return;
+    }
+
+    Package::new("orphaned", "1.0.0").publish();
+    Package::new("bar", "1.0.0")
+        .dep("orphaned", "1.0")
+        .publish();
+    Package::new("bar", "2.0.0").publish();
+    let p = project()
+        .file(
+            "Cargo.toml",
+            r#"
+                [package]
+                name = "foo"
+                version = "0.1.0"
+
+                [dependencies]
+                bar2 = { version = "2.0", package="bar" }
+                bar = "1.0"
+            "#,
+        )
+        .file("src/lib.rs", "")
+        .build();
+
+    p.cargo("doc --target")
+        .arg(cross_compile::alternate())
+        .with_stderr_unordered(
+            "\
+[UPDATING] [..]
+[DOWNLOADING] crates ...
+[DOWNLOADED] orphaned v1.0.0 [..]
+[DOWNLOADED] bar v2.0.0 [..]
+[DOWNLOADED] bar v1.0.0 [..]
+[CHECKING] orphaned v1.0.0
+[DOCUMENTING] bar v2.0.0
+[CHECKING] bar v2.0.0
+[CHECKING] bar v1.0.0
+[DOCUMENTING] foo v0.1.0 [..]
+[FINISHED] [..]
+",
+        )
+        .run();
+}