Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add schema field and features2 to the index. #9161

Merged
merged 4 commits into from
Feb 22, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/cargo/core/summary.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
47 changes: 41 additions & 6 deletions src/cargo/sources/registry/index.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
};
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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...
//
Expand All @@ -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>> {
Expand All @@ -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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that this check may actually want to be != instead of > perhaps? If Cargo version 3 reads a cache file from Cargo version 2, I think that's the case I was worried about? (where version 2 didn't cache any version 3 entries in this summary file)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes... 😡 😡 sorry, been making several elementary mistakes.

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() {
Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -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,
Expand Down
4 changes: 4 additions & 0 deletions src/cargo/sources/registry/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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> {
Expand Down
87 changes: 86 additions & 1 deletion tests/testsuite/old_cargos.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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();
}