From 196673bb1ec0b8276d57525dbeb805815e90b99e Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Wed, 10 Feb 2021 10:58:07 -0800 Subject: [PATCH 1/4] Add a schema version to the index. --- crates/cargo-test-support/src/registry.rs | 19 ++++++++++-- crates/crates-io/lib.rs | 2 ++ src/cargo/ops/registry.rs | 1 + src/cargo/sources/registry/index.rs | 37 ++++++++++++++++++++--- src/cargo/sources/registry/mod.rs | 18 +++++++++++ tests/testsuite/main.rs | 1 + tests/testsuite/registry.rs | 30 ++++++++++++++++++ 7 files changed, 100 insertions(+), 8 deletions(-) diff --git a/crates/cargo-test-support/src/registry.rs b/crates/cargo-test-support/src/registry.rs index 4a5ff4955df..1be91502868 100644 --- a/crates/cargo-test-support/src/registry.rs +++ b/crates/cargo-test-support/src/registry.rs @@ -327,6 +327,7 @@ pub struct Package { links: Option, rust_version: Option, cargo_features: Vec, + v: Option, } #[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,7 +609,7 @@ 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, @@ -607,8 +617,11 @@ impl Package { "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), diff --git a/crates/crates-io/lib.rs b/crates/crates-io/lib.rs index 3f76b4b3451..4ae5069de02 100644 --- a/crates/crates-io/lib.rs +++ b/crates/crates-io/lib.rs @@ -56,6 +56,8 @@ pub struct NewCrate { pub repository: Option, pub badges: BTreeMap>, pub links: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub v: Option, } #[derive(Serialize)] diff --git a/src/cargo/ops/registry.rs b/src/cargo/ops/registry.rs index afb1adbf9d2..7032ae13090 100644 --- a/src/cargo/ops/registry.rs +++ b/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, ); diff --git a/src/cargo/sources/registry/index.rs b/src/cargo/sources/registry/index.rs index c88726402f5..2489491c974 100644 --- a/src/cargo/sources/registry/index.rs +++ b/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, }) } } diff --git a/src/cargo/sources/registry/mod.rs b/src/cargo/sources/registry/mod.rs index 495df40bfce..3142e719b45 100644 --- a/src/cargo/sources/registry/mod.rs +++ b/src/cargo/sources/registry/mod.rs @@ -269,6 +269,24 @@ pub struct RegistryPackage<'a> { /// Added early 2018 (see ), /// can be `None` if published before then. links: Option, + /// 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, } #[test] diff --git a/tests/testsuite/main.rs b/tests/testsuite/main.rs index 4df5679189b..dd3e0498c14 100644 --- a/tests/testsuite/main.rs +++ b/tests/testsuite/main.rs @@ -78,6 +78,7 @@ mod multitarget; mod net_config; mod new; mod offline; +mod old_cargos; mod out_dir; mod owner; mod package; diff --git a/tests/testsuite/registry.rs b/tests/testsuite/registry.rs index acba4a2c413..441e965744a 100644 --- a/tests/testsuite/registry.rs +++ b/tests/testsuite/registry.rs @@ -2170,3 +2170,33 @@ fn package_lock_inside_package_is_overwritten() { assert_eq!(ok.metadata().unwrap().len(), 2); } + +#[cargo_test] +fn ignores_unknown_index_version() { + // If the version field is not understood, it is ignored. + Package::new("bar", "1.0.0").publish(); + Package::new("bar", "1.0.1").schema_version(9999).publish(); + + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + + [dependencies] + bar = "1.0" + "#, + ) + .file("src/lib.rs", "") + .build(); + + p.cargo("tree") + .with_stdout( + "foo v0.1.0 [..]\n\ + └── bar v1.0.0\n\ + ", + ) + .run(); +} From be28b58b45fb9a436d269200eb561625073e5247 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Wed, 10 Feb 2021 11:15:19 -0800 Subject: [PATCH 2/4] Add `features2` to the index. --- crates/cargo-test-support/src/registry.rs | 34 +- src/cargo/sources/registry/index.rs | 14 +- src/cargo/sources/registry/mod.rs | 11 +- tests/testsuite/features_namespaced.rs | 102 +++++ tests/testsuite/old_cargos.rs | 501 ++++++++++++++++++++++ tests/testsuite/weak_dep_features.rs | 101 ++++- 6 files changed, 755 insertions(+), 8 deletions(-) create mode 100644 tests/testsuite/old_cargos.rs diff --git a/crates/cargo-test-support/src/registry.rs b/crates/cargo-test-support/src/registry.rs index 1be91502868..39001ba5ac5 100644 --- a/crates/cargo-test-support/src/registry.rs +++ b/crates/cargo-test-support/src/registry.rs @@ -4,7 +4,7 @@ use cargo::sources::CRATES_IO_INDEX; use cargo::util::Sha256; use flate2::write::GzEncoder; use flate2::Compression; -use std::collections::HashMap; +use std::collections::BTreeMap; use std::fmt::Write as _; use std::fs::{self, File}; use std::io::{BufRead, BufReader, Write}; @@ -319,7 +319,7 @@ pub struct Package { deps: Vec, files: Vec, yanked: bool, - features: HashMap>, + features: FeatureMap, local: bool, alternative: bool, invalid_json: bool, @@ -330,6 +330,8 @@ pub struct Package { v: Option, } +type FeatureMap = BTreeMap>; + #[derive(Clone)] pub struct Dependency { name: String, @@ -394,7 +396,7 @@ impl Package { deps: Vec::new(), files: Vec::new(), yanked: false, - features: HashMap::new(), + features: BTreeMap::new(), local: false, alternative: false, invalid_json: false, @@ -609,15 +611,21 @@ impl Package { } else { serde_json::json!(self.name) }; + // This emulates what crates.io may do in the future. + let (features, features2) = split_index_features(self.features.clone()); let mut json = serde_json::json!({ "name": name, "vers": self.vers, "deps": deps, "cksum": cksum, - "features": self.features, + "features": features, "yanked": self.yanked, "links": self.links, }); + if let Some(f2) = &features2 { + json["features2"] = serde_json::json!(f2); + json["v"] = serde_json::json!(2); + } if let Some(v) = self.v { json["v"] = serde_json::json!(v); } @@ -850,3 +858,21 @@ impl Dependency { self } } + +fn split_index_features(mut features: FeatureMap) -> (FeatureMap, Option) { + let mut features2 = FeatureMap::new(); + for (feat, values) in features.iter_mut() { + if values + .iter() + .any(|value| value.starts_with("dep:") || value.contains("?/")) + { + let new_values = values.drain(..).collect(); + features2.insert(feat.clone(), new_values); + } + } + if features2.is_empty() { + (features, None) + } else { + (features, Some(features2)) + } +} diff --git a/src/cargo/sources/registry/index.rs b/src/cargo/sources/registry/index.rs index 2489491c974..83850c59223 100644 --- a/src/cargo/sources/registry/index.rs +++ b/src/cargo/sources/registry/index.rs @@ -308,7 +308,11 @@ 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; + let max_version = if namespaced_features || weak_dep_features { + 2 + } else { + 1 + }; Ok(summaries .versions .iter_mut() @@ -770,7 +774,8 @@ impl IndexSummary { vers, cksum, deps, - features, + mut features, + features2, yanked, links, v, @@ -782,6 +787,11 @@ impl IndexSummary { .into_iter() .map(|dep| dep.into_dep(source_id)) .collect::>>()?; + if let Some(features2) = features2 { + for (name, values) in features2 { + features.entry(name).or_default().extend(values); + } + } let mut summary = Summary::new(config, pkgid, deps, &features, links)?; summary.set_checksum(cksum); Ok(IndexSummary { diff --git a/src/cargo/sources/registry/mod.rs b/src/cargo/sources/registry/mod.rs index 3142e719b45..c6e50bf53b4 100644 --- a/src/cargo/sources/registry/mod.rs +++ b/src/cargo/sources/registry/mod.rs @@ -258,6 +258,13 @@ pub struct RegistryPackage<'a> { #[serde(borrow)] deps: Vec>, features: BTreeMap>, + /// This field contains features with new, extended syntax. Specifically, + /// namespaced features (`dep:`) and weak dependencies (`pkg?/feat`). + /// + /// This is separated from `features` because versions older than 1.19 + /// will fail to load due to not being able to parse the new syntax, even + /// with a `Cargo.lock` file. + features2: Option>>, cksum: String, /// If `true`, Cargo will skip this version when resolving. /// @@ -274,10 +281,12 @@ pub struct RegistryPackage<'a> { /// If this is None, it defaults to version 1. Entries with unknown /// versions are ignored. /// + /// Version `2` format adds the `features2` field. + /// /// 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 + /// versions will ignore it, and potentially misinterpret version 2 and /// newer entries. /// /// The intent is that versions older than 1.51 will work with a diff --git a/tests/testsuite/features_namespaced.rs b/tests/testsuite/features_namespaced.rs index 55078f1c28e..f3ac72208f6 100644 --- a/tests/testsuite/features_namespaced.rs +++ b/tests/testsuite/features_namespaced.rs @@ -1106,3 +1106,105 @@ feat = ["opt-dep1"] )], ); } + +#[cargo_test] +fn publish() { + // Publish behavior with explicit dep: syntax. + Package::new("bar", "1.0.0").publish(); + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + description = "foo" + license = "MIT" + homepage = "https://example.com/" + + [dependencies] + bar = { version = "1.0", optional = true } + + [features] + feat1 = [] + feat2 = ["dep:bar"] + feat3 = ["feat2"] + "#, + ) + .file("src/lib.rs", "") + .build(); + + p.cargo("publish --token sekrit -Z namespaced-features") + .masquerade_as_nightly_cargo() + .with_stderr( + "\ +[UPDATING] [..] +[PACKAGING] foo v0.1.0 [..] +[VERIFYING] foo v0.1.0 [..] +[COMPILING] foo v0.1.0 [..] +[FINISHED] [..] +[UPLOADING] foo v0.1.0 [..] +", + ) + .run(); + + publish::validate_upload_with_contents( + r#" + { + "authors": [], + "badges": {}, + "categories": [], + "deps": [ + { + "default_features": true, + "features": [], + "kind": "normal", + "name": "bar", + "optional": true, + "registry": "https://github.com/rust-lang/crates.io-index", + "target": null, + "version_req": "^1.0" + } + ], + "description": "foo", + "documentation": null, + "features": { + "feat1": [], + "feat2": ["dep:bar"], + "feat3": ["feat2"] + }, + "homepage": "https://example.com/", + "keywords": [], + "license": "MIT", + "license_file": null, + "links": null, + "name": "foo", + "readme": null, + "readme_file": null, + "repository": null, + "vers": "0.1.0" + } + "#, + "foo-0.1.0.crate", + &["Cargo.toml", "Cargo.toml.orig", "src/lib.rs"], + &[( + "Cargo.toml", + r#"[..] +[package] +name = "foo" +version = "0.1.0" +description = "foo" +homepage = "https://example.com/" +license = "MIT" +[dependencies.bar] +version = "1.0" +optional = true + +[features] +feat1 = [] +feat2 = ["dep:bar"] +feat3 = ["feat2"] +"#, + )], + ); +} diff --git a/tests/testsuite/old_cargos.rs b/tests/testsuite/old_cargos.rs new file mode 100644 index 00000000000..870c5f0b9c7 --- /dev/null +++ b/tests/testsuite/old_cargos.rs @@ -0,0 +1,501 @@ +//! Tests for checking behavior of old cargos. +//! +//! These tests are ignored because it is intended to be run on a developer +//! system with a bunch of toolchains installed. This requires `rustup` to be +//! installed. It will iterate over installed toolchains, and run some tests +//! over each one, producing a report at the end. As of this writing, I have +//! tested 1.0 to 1.51. Run this with: +//! +//! ```console +//! cargo test --test testsuite -- old_cargos --nocapture --ignored +//! ``` + +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 semver::Version; +use std::fs; + +fn tc_process(cmd: &str, toolchain: &str) -> ProcessBuilder { + if toolchain == "this" { + if cmd == "cargo" { + process(&cargo_exe()) + } else { + process(cmd) + } + } else { + let mut cmd = process(cmd); + cmd.arg(format!("+{}", toolchain)); + cmd + } +} + +/// Returns a sorted list of all toolchains. +/// +/// The returned value includes the parsed version, and the rustup toolchain +/// name as a string. +fn collect_all_toolchains() -> Vec<(Version, String)> { + let rustc_version = |tc| { + let mut cmd = tc_process("rustc", tc); + cmd.arg("-V"); + let output = cmd.exec_with_output().expect("rustc installed"); + let version = std::str::from_utf8(&output.stdout).unwrap(); + let parts: Vec<_> = version.split_whitespace().collect(); + assert_eq!(parts[0], "rustc"); + assert!(parts[1].starts_with("1.")); + Version::parse(parts[1]).expect("valid version") + }; + + // Provide a way to override the list. + if let Ok(tcs) = std::env::var("OLD_CARGO") { + return tcs + .split(',') + .map(|tc| (rustc_version(tc), tc.to_string())) + .collect(); + } + + let host = rustc_host(); + // I tend to have lots of toolchains installed, but I don't want to test + // all of them (like dated nightlies, or toolchains for non-host targets). + let valid_names = &[ + format!("stable-{}", host), + format!("beta-{}", host), + format!("nightly-{}", host), + ]; + + let output = cargo::util::process("rustup") + .args(&["toolchain", "list"]) + .exec_with_output() + .expect("rustup should be installed"); + let stdout = std::str::from_utf8(&output.stdout).unwrap(); + let mut toolchains: Vec<_> = stdout + .lines() + .map(|line| { + // Some lines say things like (default), just get the version. + line.split_whitespace().next().expect("non-empty line") + }) + .filter(|line| { + line.ends_with(&host) + && (line.starts_with("1.") || valid_names.iter().any(|name| name == line)) + }) + .map(|line| (rustc_version(line), line.to_string())) + .collect(); + + // Also include *this* cargo. + toolchains.push((rustc_version("this"), "this".to_string())); + toolchains.sort_by(|a, b| a.0.cmp(&b.0)); + toolchains +} + +// This is a test for exercising the behavior of older versions of cargo with +// the new feature syntax. +// +// The test involves a few dependencies with different feature requirements: +// +// * `bar` 1.0.0 is the base version that does not use the new syntax. +// * `bar` 1.0.1 has a feature with the new syntax, but the feature is unused. +// The optional dependency `new-baz-dep` should not be activated. +// * `bar` 1.0.2 has a dependency on `baz` that *requires* the new feature +// syntax. +#[ignore] +#[cargo_test] +fn new_features() { + if std::process::Command::new("rustup").output().is_err() { + panic!("old_cargos requires rustup to be installed"); + } + Package::new("new-baz-dep", "1.0.0").publish(); + + Package::new("baz", "1.0.0").publish(); + let baz101_cksum = Package::new("baz", "1.0.1") + .add_dep(Dependency::new("new-baz-dep", "1.0").optional(true)) + .feature("new-feat", &["dep:new-baz-dep"]) + .publish(); + + let bar100_cksum = Package::new("bar", "1.0.0") + .add_dep(Dependency::new("baz", "1.0").optional(true)) + .feature("feat", &["baz"]) + .publish(); + let bar101_cksum = Package::new("bar", "1.0.1") + .add_dep(Dependency::new("baz", "1.0").optional(true)) + .feature("feat", &["dep:baz"]) + .publish(); + let bar102_cksum = Package::new("bar", "1.0.2") + .add_dep(Dependency::new("baz", "1.0").enable_features(&["new-feat"])) + .publish(); + + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + + [dependencies] + bar = "1.0" + "#, + ) + .file("src/lib.rs", "") + .build(); + + let lock_bar_to = |toolchain_version: &Version, bar_version| { + let lock = if toolchain_version < &Version::new(1, 12, 0) { + let url = registry::registry_url(); + match bar_version { + 100 => format!( + r#" + [root] + name = "foo" + version = "0.1.0" + dependencies = [ + "bar 1.0.0 (registry+{url})", + ] + + [[package]] + name = "bar" + version = "1.0.0" + source = "registry+{url}" + "#, + url = url + ), + 101 => format!( + r#" + [root] + name = "foo" + version = "0.1.0" + dependencies = [ + "bar 1.0.1 (registry+{url})", + ] + + [[package]] + name = "bar" + version = "1.0.1" + source = "registry+{url}" + "#, + url = url + ), + 102 => format!( + r#" + [root] + name = "foo" + version = "0.1.0" + dependencies = [ + "bar 1.0.2 (registry+{url})", + ] + + [[package]] + name = "bar" + version = "1.0.2" + source = "registry+{url}" + dependencies = [ + "baz 1.0.1 (registry+{url})", + ] + + [[package]] + name = "baz" + version = "1.0.1" + source = "registry+{url}" + "#, + url = url + ), + _ => panic!("unexpected version"), + } + } else { + match bar_version { + 100 => format!( + r#" + [root] + name = "foo" + version = "0.1.0" + dependencies = [ + "bar 1.0.0 (registry+https://github.com/rust-lang/crates.io-index)", + ] + + [[package]] + name = "bar" + version = "1.0.0" + source = "registry+https://github.com/rust-lang/crates.io-index" + + [metadata] + "checksum bar 1.0.0 (registry+https://github.com/rust-lang/crates.io-index)" = "{}" + "#, + bar100_cksum + ), + 101 => format!( + r#" + [root] + name = "foo" + version = "0.1.0" + dependencies = [ + "bar 1.0.1 (registry+https://github.com/rust-lang/crates.io-index)", + ] + + [[package]] + name = "bar" + version = "1.0.1" + source = "registry+https://github.com/rust-lang/crates.io-index" + + [metadata] + "checksum bar 1.0.1 (registry+https://github.com/rust-lang/crates.io-index)" = "{}" + "#, + bar101_cksum + ), + 102 => format!( + r#" + [root] + name = "foo" + version = "0.1.0" + dependencies = [ + "bar 1.0.2 (registry+https://github.com/rust-lang/crates.io-index)", + ] + + [[package]] + name = "bar" + version = "1.0.2" + source = "registry+https://github.com/rust-lang/crates.io-index" + dependencies = [ + "baz 1.0.1 (registry+https://github.com/rust-lang/crates.io-index)", + ] + + [[package]] + name = "baz" + version = "1.0.1" + source = "registry+https://github.com/rust-lang/crates.io-index" + + [metadata] + "checksum bar 1.0.2 (registry+https://github.com/rust-lang/crates.io-index)" = "{bar102_cksum}" + "checksum baz 1.0.1 (registry+https://github.com/rust-lang/crates.io-index)" = "{baz101_cksum}" + "#, + bar102_cksum = bar102_cksum, + baz101_cksum = baz101_cksum + ), + _ => panic!("unexpected version"), + } + }; + p.change_file("Cargo.lock", &lock); + }; + + let toolchains = collect_all_toolchains(); + + let config_path = paths::home().join(".cargo/config"); + let lock_path = p.root().join("Cargo.lock"); + + struct ToolchainBehavior { + bar: Option, + baz: Option, + new_baz_dep: Option, + } + + // Collect errors to print at the end. One entry per toolchain, a list of + // strings to print. + let mut unexpected_results: Vec> = Vec::new(); + + for (version, toolchain) in &toolchains { + let mut tc_result = Vec::new(); + // Write a config appropriate for this version. + if version < &Version::new(1, 12, 0) { + fs::write( + &config_path, + format!( + r#" + [registry] + index = "{}" + "#, + registry::registry_url() + ), + ) + .unwrap(); + } else { + fs::write( + &config_path, + format!( + " + [source.crates-io] + registry = 'https://wut' # only needed by 1.12 + replace-with = 'dummy-registry' + + [source.dummy-registry] + registry = '{}' + ", + registry::registry_url() + ), + ) + .unwrap(); + } + + // Fetches the version of a package in the lock file. + let pkg_version = |pkg| -> Option { + let output = tc_process("cargo", toolchain) + .args(&["pkgid", pkg]) + .cwd(p.root()) + .exec_with_output() + .ok()?; + let stdout = std::str::from_utf8(&output.stdout).unwrap(); + let version = stdout + .trim() + .rsplitn(2, ':') + .next() + .expect("version after colon"); + Some(Version::parse(version).expect("parseable version")) + }; + + // Runs `cargo build` and returns the versions selected in the lock. + let run_cargo = || -> CargoResult { + match tc_process("cargo", toolchain) + .args(&["build", "--verbose"]) + .cwd(p.root()) + .exec_with_output() + { + Ok(_output) => { + eprintln!("{} ok", toolchain); + let bar = pkg_version("bar"); + let baz = pkg_version("baz"); + let new_baz_dep = pkg_version("new-baz-dep"); + Ok(ToolchainBehavior { + bar, + baz, + new_baz_dep, + }) + } + Err(e) => { + eprintln!("{} err {}", toolchain, e); + Err(e) + } + } + }; + + macro_rules! check_lock { + ($tc_result:ident, $pkg:expr, $which:expr, $actual:expr, None) => { + check_lock!(= $tc_result, $pkg, $which, $actual, None); + }; + ($tc_result:ident, $pkg:expr, $which:expr, $actual:expr, $expected:expr) => { + check_lock!(= $tc_result, $pkg, $which, $actual, Some(Version::parse($expected).unwrap())); + }; + (= $tc_result:ident, $pkg:expr, $which:expr, $actual:expr, $expected:expr) => { + let exp: Option = $expected; + if $actual != $expected { + $tc_result.push(format!( + "{} for {} saw {:?} but expected {:?}", + $which, $pkg, $actual, exp + )); + } + }; + } + + let check_err_contains = |tc_result: &mut Vec<_>, err: anyhow::Error, contents| { + if let Some(ProcessError { + stderr: Some(stderr), + .. + }) = err.downcast_ref::() + { + let stderr = std::str::from_utf8(&stderr).unwrap(); + if !stderr.contains(contents) { + tc_result.push(format!( + "{} expected to see error contents:\n{}\nbut saw:\n{}", + toolchain, contents, stderr + )); + } + } else { + panic!("{} unexpected error {}", toolchain, err); + } + }; + + // Unlocked behavior. + let which = "unlocked"; + lock_path.rm_rf(); + p.build_dir().rm_rf(); + match run_cargo() { + Ok(behavior) => { + // TODO: Switch to 51 after backport. + if version < &Version::new(1, 52, 0) && toolchain != "this" { + check_lock!(tc_result, "bar", which, behavior.bar, "1.0.2"); + check_lock!(tc_result, "baz", which, behavior.baz, "1.0.1"); + check_lock!(tc_result, "new-baz-dep", which, behavior.new_baz_dep, None); + } else { + check_lock!(tc_result, "bar", which, behavior.bar, "1.0.0"); + check_lock!(tc_result, "baz", which, behavior.baz, None); + check_lock!(tc_result, "new-baz-dep", which, behavior.new_baz_dep, None); + } + } + Err(e) => { + tc_result.push(format!("unlocked build failed: {}", e)); + } + } + + let which = "locked bar 1.0.0"; + lock_bar_to(&version, 100); + match run_cargo() { + Ok(behavior) => { + check_lock!(tc_result, "bar", which, behavior.bar, "1.0.0"); + check_lock!(tc_result, "baz", which, behavior.baz, None); + check_lock!(tc_result, "new-baz-dep", which, behavior.new_baz_dep, None); + } + Err(e) => { + tc_result.push(format!("bar 1.0.0 locked build failed: {}", e)); + } + } + + let which = "locked bar 1.0.1"; + lock_bar_to(&version, 101); + match run_cargo() { + Ok(behavior) => { + check_lock!(tc_result, "bar", which, behavior.bar, "1.0.1"); + check_lock!(tc_result, "baz", which, behavior.baz, None); + check_lock!(tc_result, "new-baz-dep", which, behavior.new_baz_dep, None); + } + Err(e) => { + if toolchain == "this" { + // 1.0.1 can't be used without -Znamespaced-features + // It gets filtered out of the index. + check_err_contains(&mut tc_result, e, + "error: failed to select a version for the requirement `bar = \"=1.0.1\"`\n\ + candidate versions found which didn't match: 1.0.2, 1.0.0" + ); + } else { + tc_result.push(format!("bar 1.0.1 locked build failed: {}", e)); + } + } + } + + let which = "locked bar 1.0.2"; + lock_bar_to(&version, 102); + match run_cargo() { + Ok(behavior) => { + check_lock!(tc_result, "bar", which, behavior.bar, "1.0.2"); + check_lock!(tc_result, "baz", which, behavior.baz, "1.0.1"); + check_lock!(tc_result, "new-baz-dep", which, behavior.new_baz_dep, None); + } + Err(e) => { + if toolchain == "this" { + // baz can't lock to 1.0.1, it requires -Znamespaced-features + check_err_contains(&mut tc_result, e, + "error: failed to select a version for the requirement `baz = \"=1.0.1\"`\n\ + candidate versions found which didn't match: 1.0.0" + ); + } else { + tc_result.push(format!("bar 1.0.2 locked build failed: {}", e)); + } + } + } + + unexpected_results.push(tc_result); + } + + // Generate a report. + let mut has_err = false; + for ((tc_vers, tc_name), errs) in toolchains.iter().zip(unexpected_results) { + if errs.is_empty() { + continue; + } + eprintln!("error: toolchain {} (version {}):", tc_name, tc_vers); + for err in errs { + eprintln!(" {}", err); + } + has_err = true; + } + if has_err { + panic!("at least one toolchain did not run as expected"); + } +} diff --git a/tests/testsuite/weak_dep_features.rs b/tests/testsuite/weak_dep_features.rs index 255e5628e24..338bf8968fa 100644 --- a/tests/testsuite/weak_dep_features.rs +++ b/tests/testsuite/weak_dep_features.rs @@ -1,7 +1,7 @@ //! Tests for weak-dep-features. -use cargo_test_support::project; use cargo_test_support::registry::{Dependency, Package}; +use cargo_test_support::{project, publish}; use std::fmt::Write; // Helper to create lib.rs files that check features. @@ -565,3 +565,102 @@ bar v1.0.0 ) .run(); } + +#[cargo_test] +fn publish() { + // Publish behavior with /? syntax. + Package::new("bar", "1.0.0").feature("feat", &[]).publish(); + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + description = "foo" + license = "MIT" + homepage = "https://example.com/" + + [dependencies] + bar = { version = "1.0", optional = true } + + [features] + feat1 = [] + feat2 = ["bar?/feat"] + "#, + ) + .file("src/lib.rs", "") + .build(); + + p.cargo("publish --token sekrit -Z weak-dep-features") + .masquerade_as_nightly_cargo() + .with_stderr( + "\ +[UPDATING] [..] +[PACKAGING] foo v0.1.0 [..] +[VERIFYING] foo v0.1.0 [..] +[COMPILING] foo v0.1.0 [..] +[FINISHED] [..] +[UPLOADING] foo v0.1.0 [..] +", + ) + .run(); + + publish::validate_upload_with_contents( + r#" + { + "authors": [], + "badges": {}, + "categories": [], + "deps": [ + { + "default_features": true, + "features": [], + "kind": "normal", + "name": "bar", + "optional": true, + "registry": "https://github.com/rust-lang/crates.io-index", + "target": null, + "version_req": "^1.0" + } + ], + "description": "foo", + "documentation": null, + "features": { + "feat1": [], + "feat2": ["bar?/feat"] + }, + "homepage": "https://example.com/", + "keywords": [], + "license": "MIT", + "license_file": null, + "links": null, + "name": "foo", + "readme": null, + "readme_file": null, + "repository": null, + "vers": "0.1.0" + } + "#, + "foo-0.1.0.crate", + &["Cargo.toml", "Cargo.toml.orig", "src/lib.rs"], + &[( + "Cargo.toml", + r#"[..] +[package] +name = "foo" +version = "0.1.0" +description = "foo" +homepage = "https://example.com/" +license = "MIT" +[dependencies.bar] +version = "1.0" +optional = true + +[features] +feat1 = [] +feat2 = ["bar?/feat"] +"#, + )], + ); +} From 0d4137f4f50d19eef583753c32806217a30cc031 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Fri, 19 Feb 2021 10:51:38 -0800 Subject: [PATCH 3/4] 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. --- src/cargo/core/summary.rs | 3 + src/cargo/sources/registry/index.rs | 47 ++++++++++++++-- src/cargo/sources/registry/mod.rs | 4 ++ tests/testsuite/old_cargos.rs | 87 ++++++++++++++++++++++++++++- 4 files changed, 134 insertions(+), 7 deletions(-) diff --git a/src/cargo/core/summary.rs b/src/cargo/core/summary.rs index 7bd5f99dc41..8f77c75fb4c 100644 --- a/src/cargo/core/summary.rs +++ b/src/cargo/core/summary.rs @@ -37,6 +37,9 @@ impl Summary { features: &BTreeMap>, links: Option>, ) -> CargoResult { + // ****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(); diff --git a/src/cargo/sources/registry/index.rs b/src/cargo/sources/registry/index.rs index 83850c59223..c3af00e6f37 100644 --- a/src/cargo/sources/registry/index.rs +++ b/src/cargo/sources/registry/index.rs @@ -68,7 +68,7 @@ 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}; @@ -76,6 +76,7 @@ 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> { @@ -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 { + // ****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, diff --git a/src/cargo/sources/registry/mod.rs b/src/cargo/sources/registry/mod.rs index c6e50bf53b4..d43a3d26da2 100644 --- a/src/cargo/sources/registry/mod.rs +++ b/src/cargo/sources/registry/mod.rs @@ -250,6 +250,10 @@ pub struct RegistryConfig { pub api: Option, } +/// 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> { diff --git a/tests/testsuite/old_cargos.rs b/tests/testsuite/old_cargos.rs index 870c5f0b9c7..5feed15fd77 100644 --- a/tests/testsuite/old_cargos.rs +++ b/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(); +} From f25856922d69f6a9e193604c5ecb7ccc88cefd2f Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Sat, 20 Feb 2021 06:28:51 -0800 Subject: [PATCH 4/4] Fix index_v version check. The whole point is that when loading a cache from an earlier version, that cache may be missing new entries. So don't load older caches. --- src/cargo/sources/registry/index.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/cargo/sources/registry/index.rs b/src/cargo/sources/registry/index.rs index c3af00e6f37..729b502cf1d 100644 --- a/src/cargo/sources/registry/index.rs +++ b/src/cargo/sources/registry/index.rs @@ -708,9 +708,9 @@ impl<'a> SummariesCache<'a> { .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 { + if index_v != INDEX_V_MAX { bail!( - "index format version {} is greater than the newest version I know ({})", + "index format version {} doesn't match the version I know ({})", index_v, INDEX_V_MAX );