Skip to content

Commit 0d4137f

Browse files
committed
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.
1 parent be28b58 commit 0d4137f

File tree

4 files changed

+134
-7
lines changed

4 files changed

+134
-7
lines changed

src/cargo/core/summary.rs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,9 @@ impl Summary {
3737
features: &BTreeMap<InternedString, Vec<InternedString>>,
3838
links: Option<impl Into<InternedString>>,
3939
) -> CargoResult<Summary> {
40+
// ****CAUTION**** If you change anything here than may raise a new
41+
// error, be sure to coordinate that change with either the index
42+
// schema field or the SummariesCache version.
4043
let mut has_overlapping_features = None;
4144
for dep in dependencies.iter() {
4245
let dep_name = dep.name_in_toml();

src/cargo/sources/registry/index.rs

Lines changed: 41 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -68,14 +68,15 @@
6868
6969
use crate::core::dependency::Dependency;
7070
use crate::core::{PackageId, SourceId, Summary};
71-
use crate::sources::registry::{RegistryData, RegistryPackage};
71+
use crate::sources::registry::{RegistryData, RegistryPackage, INDEX_V_MAX};
7272
use crate::util::interning::InternedString;
7373
use crate::util::paths;
7474
use crate::util::{internal, CargoResult, Config, Filesystem, ToSemver};
7575
use anyhow::bail;
7676
use log::{debug, info};
7777
use semver::{Version, VersionReq};
7878
use std::collections::{HashMap, HashSet};
79+
use std::convert::TryInto;
7980
use std::fs;
8081
use std::path::Path;
8182
use std::str;
@@ -309,7 +310,7 @@ impl<'cfg> RegistryIndex<'cfg> {
309310
// necessary.
310311
let raw_data = &summaries.raw_data;
311312
let max_version = if namespaced_features || weak_dep_features {
312-
2
313+
INDEX_V_MAX
313314
} else {
314315
1
315316
};
@@ -571,6 +572,14 @@ impl Summaries {
571572
let summary = match IndexSummary::parse(config, line, source_id) {
572573
Ok(summary) => summary,
573574
Err(e) => {
575+
// This should only happen when there is an index
576+
// entry from a future version of cargo that this
577+
// version doesn't understand. Hopefully, those future
578+
// versions of cargo correctly set INDEX_V_MAX and
579+
// CURRENT_CACHE_VERSION, otherwise this will skip
580+
// entries in the cache preventing those newer
581+
// versions from reading them (that is, until the
582+
// cache is rebuilt).
574583
log::info!("failed to parse {:?} registry package: {}", relative, e);
575584
continue;
576585
}
@@ -658,9 +667,9 @@ impl Summaries {
658667
// Implementation of serializing/deserializing the cache of summaries on disk.
659668
// Currently the format looks like:
660669
//
661-
// +--------------+-------------+---+
662-
// | version byte | git sha rev | 0 |
663-
// +--------------+-------------+---+
670+
// +--------------------+----------------------+-------------+---+
671+
// | cache version byte | index format version | git sha rev | 0 |
672+
// +--------------------+----------------------+-------------+---+
664673
//
665674
// followed by...
666675
//
@@ -677,8 +686,14 @@ impl Summaries {
677686
// versions of Cargo share the same cache they don't get too confused. The git
678687
// sha lets us know when the file needs to be regenerated (it needs regeneration
679688
// whenever the index itself updates).
689+
//
690+
// Cache versions:
691+
// * `1`: The original version.
692+
// * `2`: Added the "index format version" field so that if the index format
693+
// changes, different versions of cargo won't get confused reading each
694+
// other's caches.
680695

681-
const CURRENT_CACHE_VERSION: u8 = 1;
696+
const CURRENT_CACHE_VERSION: u8 = 2;
682697

683698
impl<'a> SummariesCache<'a> {
684699
fn parse(data: &'a [u8], last_index_update: &str) -> CargoResult<SummariesCache<'a>> {
@@ -689,6 +704,19 @@ impl<'a> SummariesCache<'a> {
689704
if *first_byte != CURRENT_CACHE_VERSION {
690705
bail!("looks like a different Cargo's cache, bailing out");
691706
}
707+
let index_v_bytes = rest
708+
.get(..4)
709+
.ok_or_else(|| anyhow::anyhow!("cache expected 4 bytes for index version"))?;
710+
let index_v = u32::from_le_bytes(index_v_bytes.try_into().unwrap());
711+
if index_v > INDEX_V_MAX {
712+
bail!(
713+
"index format version {} is greater than the newest version I know ({})",
714+
index_v,
715+
INDEX_V_MAX
716+
);
717+
}
718+
let rest = &rest[4..];
719+
692720
let mut iter = split(rest, 0);
693721
if let Some(update) = iter.next() {
694722
if update != last_index_update.as_bytes() {
@@ -720,6 +748,7 @@ impl<'a> SummariesCache<'a> {
720748
.sum();
721749
let mut contents = Vec::with_capacity(size);
722750
contents.push(CURRENT_CACHE_VERSION);
751+
contents.extend(&u32::to_le_bytes(INDEX_V_MAX));
723752
contents.extend_from_slice(index_version.as_bytes());
724753
contents.push(0);
725754
for (version, data) in self.versions.iter() {
@@ -769,6 +798,12 @@ impl IndexSummary {
769798
///
770799
/// The `line` provided is expected to be valid JSON.
771800
fn parse(config: &Config, line: &[u8], source_id: SourceId) -> CargoResult<IndexSummary> {
801+
// ****CAUTION**** Please be extremely careful with returning errors
802+
// from this function. Entries that error are not included in the
803+
// index cache, and can cause cargo to get confused when switching
804+
// between different versions that understand the index differently.
805+
// Make sure to consider the INDEX_V_MAX and CURRENT_CACHE_VERSION
806+
// values carefully when making changes here.
772807
let RegistryPackage {
773808
name,
774809
vers,

src/cargo/sources/registry/mod.rs

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -250,6 +250,10 @@ pub struct RegistryConfig {
250250
pub api: Option<String>,
251251
}
252252

253+
/// The maximum version of the `v` field in the index this version of cargo
254+
/// understands.
255+
pub(crate) const INDEX_V_MAX: u32 = 2;
256+
253257
/// A single line in the index representing a single version of a package.
254258
#[derive(Deserialize)]
255259
pub struct RegistryPackage<'a> {

tests/testsuite/old_cargos.rs

Lines changed: 86 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ use cargo::util::{ProcessBuilder, ProcessError};
1414
use cargo::CargoResult;
1515
use cargo_test_support::paths::CargoPathExt;
1616
use cargo_test_support::registry::{self, Dependency, Package};
17-
use cargo_test_support::{cargo_exe, paths, process, project, rustc_host};
17+
use cargo_test_support::{cargo_exe, execs, paths, process, project, rustc_host};
1818
use semver::Version;
1919
use std::fs;
2020

@@ -499,3 +499,88 @@ fn new_features() {
499499
panic!("at least one toolchain did not run as expected");
500500
}
501501
}
502+
503+
#[cargo_test]
504+
#[ignore]
505+
fn index_cache_rebuild() {
506+
// Checks that the index cache gets rebuilt.
507+
//
508+
// 1.48 will not cache entries with features with the same name as a
509+
// dependency. If the cache does not get rebuilt, then running with
510+
// `-Znamespaced-features` would prevent the new cargo from seeing those
511+
// entries. The index cache version was changed to prevent this from
512+
// happening, and switching between versions should work correctly
513+
// (although it will thrash the cash, that's better than not working
514+
// correctly.
515+
Package::new("baz", "1.0.0").publish();
516+
Package::new("bar", "1.0.0").publish();
517+
Package::new("bar", "1.0.1")
518+
.add_dep(Dependency::new("baz", "1.0").optional(true))
519+
.feature("baz", &["dep:baz"])
520+
.publish();
521+
522+
let p = project()
523+
.file(
524+
"Cargo.toml",
525+
r#"
526+
[package]
527+
name = "foo"
528+
version = "0.1.0"
529+
530+
[dependencies]
531+
bar = "1.0"
532+
"#,
533+
)
534+
.file("src/lib.rs", "")
535+
.build();
536+
537+
// This version of Cargo errors on index entries that have overlapping
538+
// feature names, so 1.0.1 will be missing.
539+
execs()
540+
.with_process_builder(tc_process("cargo", "1.48.0"))
541+
.arg("check")
542+
.cwd(p.root())
543+
.with_stderr(
544+
"\
545+
[UPDATING] [..]
546+
[DOWNLOADING] crates ...
547+
[DOWNLOADED] bar v1.0.0 [..]
548+
[CHECKING] bar v1.0.0
549+
[CHECKING] foo v0.1.0 [..]
550+
[FINISHED] [..]
551+
",
552+
)
553+
.run();
554+
555+
fs::remove_file(p.root().join("Cargo.lock")).unwrap();
556+
557+
// This should rebuild the cache and use 1.0.1.
558+
p.cargo("check -Znamespaced-features")
559+
.masquerade_as_nightly_cargo()
560+
.with_stderr(
561+
"\
562+
[UPDATING] [..]
563+
[DOWNLOADING] crates ...
564+
[DOWNLOADED] bar v1.0.1 [..]
565+
[CHECKING] bar v1.0.1
566+
[CHECKING] foo v0.1.0 [..]
567+
[FINISHED] [..]
568+
",
569+
)
570+
.run();
571+
572+
fs::remove_file(p.root().join("Cargo.lock")).unwrap();
573+
574+
// Verify 1.48 can still resolve, and is at 1.0.0.
575+
execs()
576+
.with_process_builder(tc_process("cargo", "1.48.0"))
577+
.arg("tree")
578+
.cwd(p.root())
579+
.with_stdout(
580+
"\
581+
foo v0.1.0 [..]
582+
└── bar v1.0.0
583+
",
584+
)
585+
.run();
586+
}

0 commit comments

Comments
 (0)