Include the index version in the index cache.

This is intended to help prevent the following scenario from happening:

1. Old cargo builds an index cache.
2. Old cargo finds an index entry it cannot parse, skipping it,
   and saving the cache without the entry.
3. New cargo loads the cache with the missing entry, and never sees
   the new entries that it understands.

This may result in more cache thrashing, but that seems better than
having new cargos missing entries.

Author: ehuss

src/cargo/core/summary.rs

@@ -37,6 +37,9 @@ impl Summary {
         features: &BTreeMap<InternedString, Vec<InternedString>>,
         links: Option<impl Into<InternedString>>,
     ) -> CargoResult<Summary> {
+        // ****CAUTION**** If you change anything here than may raise a new
+        // error, be sure to coordinate that change with either the index
+        // schema field or the SummariesCache version.
         let mut has_overlapping_features = None;
         for dep in dependencies.iter() {
             let dep_name = dep.name_in_toml();

src/cargo/sources/registry/index.rs

@@ -68,14 +68,15 @@
 
 use crate::core::dependency::Dependency;
 use crate::core::{PackageId, SourceId, Summary};
-use crate::sources::registry::{RegistryData, RegistryPackage};
+use crate::sources::registry::{RegistryData, RegistryPackage, INDEX_V_MAX};
 use crate::util::interning::InternedString;
 use crate::util::paths;
 use crate::util::{internal, CargoResult, Config, Filesystem, ToSemver};
 use anyhow::bail;
 use log::{debug, info};
 use semver::{Version, VersionReq};
 use std::collections::{HashMap, HashSet};
+use std::convert::TryInto;
 use std::fs;
 use std::path::Path;
 use std::str;
@@ -309,7 +310,7 @@ impl<'cfg> RegistryIndex<'cfg> {
         // necessary.
         let raw_data = &summaries.raw_data;
         let max_version = if namespaced_features || weak_dep_features {
-            2
+            INDEX_V_MAX
         } else {
             1
         };
@@ -571,6 +572,14 @@ impl Summaries {
                 let summary = match IndexSummary::parse(config, line, source_id) {
                     Ok(summary) => summary,
                     Err(e) => {
+                        // This should only happen when there is an index
+                        // entry from a future version of cargo that this
+                        // version doesn't understand. Hopefully, those future
+                        // versions of cargo correctly set INDEX_V_MAX and
+                        // CURRENT_CACHE_VERSION, otherwise this will skip
+                        // entries in the cache preventing those newer
+                        // versions from reading them (that is, until the
+                        // cache is rebuilt).
                         log::info!("failed to parse {:?} registry package: {}", relative, e);
                         continue;
                     }
@@ -658,9 +667,9 @@ impl Summaries {
 // Implementation of serializing/deserializing the cache of summaries on disk.
 // Currently the format looks like:
 //
-// +--------------+-------------+---+
-// | version byte | git sha rev | 0 |
-// +--------------+-------------+---+
+// +--------------------+----------------------+-------------+---+
+// | cache version byte | index format version | git sha rev | 0 |
+// +--------------------+----------------------+-------------+---+
 //
 // followed by...
 //
@@ -677,8 +686,14 @@ impl Summaries {
 // versions of Cargo share the same cache they don't get too confused. The git
 // sha lets us know when the file needs to be regenerated (it needs regeneration
 // whenever the index itself updates).
+//
+// Cache versions:
+// * `1`: The original version.
+// * `2`: Added the "index format version" field so that if the index format
+//   changes, different versions of cargo won't get confused reading each
+//   other's caches.
 
-const CURRENT_CACHE_VERSION: u8 = 1;
+const CURRENT_CACHE_VERSION: u8 = 2;
 
 impl<'a> SummariesCache<'a> {
     fn parse(data: &'a [u8], last_index_update: &str) -> CargoResult<SummariesCache<'a>> {
@@ -689,6 +704,19 @@ impl<'a> SummariesCache<'a> {
         if *first_byte != CURRENT_CACHE_VERSION {
             bail!("looks like a different Cargo's cache, bailing out");
         }
+        let index_v_bytes = rest
+            .get(..4)
+            .ok_or_else(|| anyhow::anyhow!("cache expected 4 bytes for index version"))?;
+        let index_v = u32::from_le_bytes(index_v_bytes.try_into().unwrap());
+        if index_v > INDEX_V_MAX {
+            bail!(
+                "index format version {} is greater than the newest version I know ({})",
+                index_v,
+                INDEX_V_MAX
+            );
+        }
+        let rest = &rest[4..];
+
         let mut iter = split(rest, 0);
         if let Some(update) = iter.next() {
             if update != last_index_update.as_bytes() {
@@ -720,6 +748,7 @@ impl<'a> SummariesCache<'a> {
             .sum();
         let mut contents = Vec::with_capacity(size);
         contents.push(CURRENT_CACHE_VERSION);
+        contents.extend(&u32::to_le_bytes(INDEX_V_MAX));
         contents.extend_from_slice(index_version.as_bytes());
         contents.push(0);
         for (version, data) in self.versions.iter() {
@@ -769,6 +798,12 @@ impl IndexSummary {
     ///
     /// The `line` provided is expected to be valid JSON.
     fn parse(config: &Config, line: &[u8], source_id: SourceId) -> CargoResult<IndexSummary> {
+        // ****CAUTION**** Please be extremely careful with returning errors
+        // from this function. Entries that error are not included in the
+        // index cache, and can cause cargo to get confused when switching
+        // between different versions that understand the index differently.
+        // Make sure to consider the INDEX_V_MAX and CURRENT_CACHE_VERSION
+        // values carefully when making changes here.
         let RegistryPackage {
             name,
             vers,

src/cargo/sources/registry/mod.rs

@@ -250,6 +250,10 @@ pub struct RegistryConfig {
     pub api: Option<String>,
 }
 
+/// The maximum version of the `v` field in the index this version of cargo
+/// understands.
+pub(crate) const INDEX_V_MAX: u32 = 2;
+
 /// A single line in the index representing a single version of a package.
 #[derive(Deserialize)]
 pub struct RegistryPackage<'a> {

tests/testsuite/old_cargos.rs

@@ -14,7 +14,7 @@ use cargo::util::{ProcessBuilder, ProcessError};
 use cargo::CargoResult;
 use cargo_test_support::paths::CargoPathExt;
 use cargo_test_support::registry::{self, Dependency, Package};
-use cargo_test_support::{cargo_exe, paths, process, project, rustc_host};
+use cargo_test_support::{cargo_exe, execs, paths, process, project, rustc_host};
 use semver::Version;
 use std::fs;
 
@@ -499,3 +499,88 @@ fn new_features() {
         panic!("at least one toolchain did not run as expected");
     }
 }
+
+#[cargo_test]
+#[ignore]
+fn index_cache_rebuild() {
+    // Checks that the index cache gets rebuilt.
+    //
+    // 1.48 will not cache entries with features with the same name as a
+    // dependency. If the cache does not get rebuilt, then running with
+    // `-Znamespaced-features` would prevent the new cargo from seeing those
+    // entries. The index cache version was changed to prevent this from
+    // happening, and switching between versions should work correctly
+    // (although it will thrash the cash, that's better than not working
+    // correctly.
+    Package::new("baz", "1.0.0").publish();
+    Package::new("bar", "1.0.0").publish();
+    Package::new("bar", "1.0.1")
+        .add_dep(Dependency::new("baz", "1.0").optional(true))
+        .feature("baz", &["dep:baz"])
+        .publish();
+
+    let p = project()
+        .file(
+            "Cargo.toml",
+            r#"
+                [package]
+                name = "foo"
+                version = "0.1.0"
+
+                [dependencies]
+                bar = "1.0"
+            "#,
+        )
+        .file("src/lib.rs", "")
+        .build();
+
+    // This version of Cargo errors on index entries that have overlapping
+    // feature names, so 1.0.1 will be missing.
+    execs()
+        .with_process_builder(tc_process("cargo", "1.48.0"))
+        .arg("check")
+        .cwd(p.root())
+        .with_stderr(
+            "\
+[UPDATING] [..]
+[DOWNLOADING] crates ...
+[DOWNLOADED] bar v1.0.0 [..]
+[CHECKING] bar v1.0.0
+[CHECKING] foo v0.1.0 [..]
+[FINISHED] [..]
+",
+        )
+        .run();
+
+    fs::remove_file(p.root().join("Cargo.lock")).unwrap();
+
+    // This should rebuild the cache and use 1.0.1.
+    p.cargo("check -Znamespaced-features")
+        .masquerade_as_nightly_cargo()
+        .with_stderr(
+            "\
+[UPDATING] [..]
+[DOWNLOADING] crates ...
+[DOWNLOADED] bar v1.0.1 [..]
+[CHECKING] bar v1.0.1
+[CHECKING] foo v0.1.0 [..]
+[FINISHED] [..]
+",
+        )
+        .run();
+
+    fs::remove_file(p.root().join("Cargo.lock")).unwrap();
+
+    // Verify 1.48 can still resolve, and is at 1.0.0.
+    execs()
+        .with_process_builder(tc_process("cargo", "1.48.0"))
+        .arg("tree")
+        .cwd(p.root())
+        .with_stdout(
+            "\
+foo v0.1.0 [..]
+└── bar v1.0.0
+",
+        )
+        .run();
+}