From 7dd9872c13e31b5782ec37dee05f35abab6b6f92 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Thu, 4 Feb 2021 08:33:33 -0800 Subject: [PATCH 1/4] Change git dependencies to use `HEAD` by default This commit follows through with work started in #8522 to change the default behavior of `git` dependencies where if not branch/tag/etc is listed then `HEAD` is used instead of the `master` branch. This involves also changing the default lock file format, now including a `version` marker at the top of the file notably as well as changing the encoding of `branch=master` directives in `Cargo.toml`. If we did all our work correctly then this will be a seamless change. First released on stable in 1.47.0 (2020-10-08) Cargo has been emitting warnings about situations which may break in the future. This means that if you don't specify `branch = 'master'` but your HEAD branch isn't `master`, you've been getting a warning. Similarly if your dependency graph used both `branch = 'master'` as well as specifying nothing, you were receiving warnings as well. These two situations are broken by this commit, but it's hoped that by giving enough times with warnings we don't actually break anyone in practice. --- src/cargo/core/resolver/dep_cache.rs | 48 -------- src/cargo/core/resolver/mod.rs | 3 +- src/cargo/core/resolver/resolve.rs | 2 +- src/cargo/core/source/source_id.rs | 78 +------------ src/cargo/sources/git/source.rs | 8 +- src/cargo/sources/git/utils.rs | 167 ++------------------------- src/cargo/sources/registry/remote.rs | 2 +- tests/testsuite/git.rs | 122 ------------------- tests/testsuite/lockfile_compat.rs | 8 ++ tests/testsuite/update.rs | 2 +- 10 files changed, 30 insertions(+), 410 deletions(-) diff --git a/src/cargo/core/resolver/dep_cache.rs b/src/cargo/core/resolver/dep_cache.rs index 1f6c49ca0fb..7e3f4fb5938 100644 --- a/src/cargo/core/resolver/dep_cache.rs +++ b/src/cargo/core/resolver/dep_cache.rs @@ -14,10 +14,8 @@ use crate::core::resolver::errors::describe_path; use crate::core::resolver::types::{ConflictReason, DepInfo, FeaturesSet}; use crate::core::resolver::{ActivateError, ActivateResult, ResolveOpts}; use crate::core::{Dependency, FeatureValue, PackageId, PackageIdSpec, Registry, Summary}; -use crate::core::{GitReference, SourceId}; use crate::util::errors::{CargoResult, CargoResultExt}; use crate::util::interning::InternedString; -use crate::util::Config; use log::debug; use std::cmp::Ordering; use std::collections::{BTreeSet, HashMap, HashSet}; @@ -40,10 +38,6 @@ pub struct RegistryQueryer<'a> { >, /// all the cases we ended up using a supplied replacement used_replacements: HashMap, - /// Where to print warnings, if configured. - config: Option<&'a Config>, - /// Sources that we've already wared about possibly colliding in the future. - warned_git_collisions: HashSet, } impl<'a> RegistryQueryer<'a> { @@ -52,7 +46,6 @@ impl<'a> RegistryQueryer<'a> { replacements: &'a [(PackageIdSpec, Dependency)], try_to_use: &'a HashSet, minimal_versions: bool, - config: Option<&'a Config>, ) -> Self { RegistryQueryer { registry, @@ -62,8 +55,6 @@ impl<'a> RegistryQueryer<'a> { registry_cache: HashMap::new(), summary_cache: HashMap::new(), used_replacements: HashMap::new(), - config, - warned_git_collisions: HashSet::new(), } } @@ -75,44 +66,6 @@ impl<'a> RegistryQueryer<'a> { self.used_replacements.get(&p) } - /// Issues a future-compatible warning targeted at removing reliance on - /// unifying behavior between these two dependency directives: - /// - /// ```toml - /// [dependencies] - /// a = { git = 'https://example.org/foo' } - /// a = { git = 'https://example.org/foo', branch = 'master } - /// ``` - /// - /// Historical versions of Cargo considered these equivalent but going - /// forward we'd like to fix this. For more details see the comments in - /// src/cargo/sources/git/utils.rs - fn warn_colliding_git_sources(&mut self, id: SourceId) -> CargoResult<()> { - let config = match self.config { - Some(config) => config, - None => return Ok(()), - }; - let prev = match self.warned_git_collisions.replace(id) { - Some(prev) => prev, - None => return Ok(()), - }; - match (id.git_reference(), prev.git_reference()) { - (Some(GitReference::DefaultBranch), Some(GitReference::Branch(b))) - | (Some(GitReference::Branch(b)), Some(GitReference::DefaultBranch)) - if b == "master" => {} - _ => return Ok(()), - } - - config.shell().warn(&format!( - "two git dependencies found for `{}` \ - where one uses `branch = \"master\"` and the other doesn't; \ - this will break in a future version of Cargo, so please \ - ensure the dependency forms are consistent", - id.url(), - ))?; - Ok(()) - } - /// Queries the `registry` to return a list of candidates for `dep`. /// /// This method is the location where overrides are taken into account. If @@ -120,7 +73,6 @@ impl<'a> RegistryQueryer<'a> { /// applied by performing a second query for what the override should /// return. pub fn query(&mut self, dep: &Dependency) -> CargoResult>> { - self.warn_colliding_git_sources(dep.source_id())?; if let Some(out) = self.registry_cache.get(dep).cloned() { return Ok(out); } diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index 0531f3dba89..6cec24702f4 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/src/cargo/core/resolver/mod.rs @@ -133,8 +133,7 @@ pub fn resolve( Some(config) => config.cli_unstable().minimal_versions, None => false, }; - let mut registry = - RegistryQueryer::new(registry, replacements, try_to_use, minimal_versions, config); + let mut registry = RegistryQueryer::new(registry, replacements, try_to_use, minimal_versions); let cx = activate_deps_loop(cx, &mut registry, summaries, config)?; let mut cksums = HashMap::new(); diff --git a/src/cargo/core/resolver/resolve.rs b/src/cargo/core/resolver/resolve.rs index 9345aaf9e55..2d99fa7b2aa 100644 --- a/src/cargo/core/resolver/resolve.rs +++ b/src/cargo/core/resolver/resolve.rs @@ -409,6 +409,6 @@ impl Default for ResolveVersion { /// file anyway so it takes the opportunity to bump the lock file version /// forward. fn default() -> ResolveVersion { - ResolveVersion::V2 + ResolveVersion::V3 } } diff --git a/src/cargo/core/source/source_id.rs b/src/cargo/core/source/source_id.rs index a1183f39670..6061a9cf7db 100644 --- a/src/cargo/core/source/source_id.rs +++ b/src/cargo/core/source/source_id.rs @@ -394,45 +394,9 @@ impl Ord for SourceId { // Sort first based on `kind`, deferring to the URL comparison below if // the kinds are equal. - match (&self.inner.kind, &other.inner.kind) { - (SourceKind::Path, SourceKind::Path) => {} - (SourceKind::Path, _) => return Ordering::Less, - (_, SourceKind::Path) => return Ordering::Greater, - - (SourceKind::Registry, SourceKind::Registry) => {} - (SourceKind::Registry, _) => return Ordering::Less, - (_, SourceKind::Registry) => return Ordering::Greater, - - (SourceKind::LocalRegistry, SourceKind::LocalRegistry) => {} - (SourceKind::LocalRegistry, _) => return Ordering::Less, - (_, SourceKind::LocalRegistry) => return Ordering::Greater, - - (SourceKind::Directory, SourceKind::Directory) => {} - (SourceKind::Directory, _) => return Ordering::Less, - (_, SourceKind::Directory) => return Ordering::Greater, - - (SourceKind::Git(a), SourceKind::Git(b)) => { - use GitReference::*; - let ord = match (a, b) { - (Tag(a), Tag(b)) => a.cmp(b), - (Tag(_), _) => Ordering::Less, - (_, Tag(_)) => Ordering::Greater, - - (Rev(a), Rev(b)) => a.cmp(b), - (Rev(_), _) => Ordering::Less, - (_, Rev(_)) => Ordering::Greater, - - // See module comments in src/cargo/sources/git/utils.rs - // for why `DefaultBranch` is treated specially here. - (Branch(a), DefaultBranch) => a.as_str().cmp("master"), - (DefaultBranch, Branch(b)) => "master".cmp(b), - (Branch(a), Branch(b)) => a.cmp(b), - (DefaultBranch, DefaultBranch) => Ordering::Equal, - }; - if ord != Ordering::Equal { - return ord; - } - } + match self.inner.kind.cmp(&other.inner.kind) { + Ordering::Equal => {} + other => return other, } // If the `kind` and the `url` are equal, then for git sources we also @@ -509,43 +473,9 @@ impl fmt::Display for SourceId { // The hash of SourceId is used in the name of some Cargo folders, so shouldn't // vary. `as_str` gives the serialisation of a url (which has a spec) and so // insulates against possible changes in how the url crate does hashing. -// -// Note that the semi-funky hashing here is done to handle `DefaultBranch` -// hashing the same as `"master"`, and also to hash the same as previous -// versions of Cargo while it's somewhat convenient to do so (that way all -// versions of Cargo use the same checkout). impl Hash for SourceId { fn hash(&self, into: &mut S) { - match &self.inner.kind { - SourceKind::Git(GitReference::Tag(a)) => { - 0usize.hash(into); - 0usize.hash(into); - a.hash(into); - } - SourceKind::Git(GitReference::Branch(a)) => { - 0usize.hash(into); - 1usize.hash(into); - a.hash(into); - } - // For now hash `DefaultBranch` the same way as `Branch("master")`, - // and for more details see module comments in - // src/cargo/sources/git/utils.rs for why `DefaultBranch` - SourceKind::Git(GitReference::DefaultBranch) => { - 0usize.hash(into); - 1usize.hash(into); - "master".hash(into); - } - SourceKind::Git(GitReference::Rev(a)) => { - 0usize.hash(into); - 2usize.hash(into); - a.hash(into); - } - - SourceKind::Path => 1usize.hash(into), - SourceKind::Registry => 2usize.hash(into), - SourceKind::LocalRegistry => 3usize.hash(into), - SourceKind::Directory => 4usize.hash(into), - } + self.inner.kind.hash(into); match self.inner.kind { SourceKind::Git(_) => self.inner.canonical_url.hash(into), _ => self.inner.url.as_str().hash(into), diff --git a/src/cargo/sources/git/source.rs b/src/cargo/sources/git/source.rs index 0723e360628..9d7c42b829f 100644 --- a/src/cargo/sources/git/source.rs +++ b/src/cargo/sources/git/source.rs @@ -126,12 +126,10 @@ impl<'cfg> Source for GitSource<'cfg> { // database, then try to resolve our reference with the preexisting // repository. (None, Some(db)) if self.config.offline() => { - let rev = db - .resolve(&self.manifest_reference, None) - .with_context(|| { - "failed to lookup reference in preexisting repository, and \ + let rev = db.resolve(&self.manifest_reference).with_context(|| { + "failed to lookup reference in preexisting repository, and \ can't check for updates in offline mode (--offline)" - })?; + })?; (db, rev) } diff --git a/src/cargo/sources/git/utils.rs b/src/cargo/sources/git/utils.rs index bdb3638b61c..1998f27a23e 100644 --- a/src/cargo/sources/git/utils.rs +++ b/src/cargo/sources/git/utils.rs @@ -1,110 +1,5 @@ //! Utilities for handling git repositories, mainly around //! authentication/cloning. -//! -//! # `DefaultBranch` vs `Branch("master")` -//! -//! Long ago in a repository not so far away, an author (*cough* me *cough*) -//! didn't understand how branches work in Git. This led the author to -//! interpret these two dependency declarations the exact same way with the -//! former literally internally desugaring to the latter: -//! -//! ```toml -//! [dependencies] -//! foo = { git = "https://example.org/foo" } -//! foo = { git = "https://example.org/foo", branch = "master" } -//! ``` -//! -//! It turns out there's this things called `HEAD` in git remotes which points -//! to the "main branch" of a repository, and the main branch is not always -//! literally called master. What Cargo would like to do is to differentiate -//! these two dependency directives, with the first meaning "depend on `HEAD`". -//! -//! Unfortunately implementing this is a breaking change. This was first -//! attempted in #8364 but resulted in #8468 which has two independent bugs -//! listed on that issue. Despite this breakage we would still like to roll out -//! this change in Cargo, but we're now going to take it very slow and try to -//! break as few people as possible along the way. These comments are intended -//! to log the current progress and what wonkiness you might see within Cargo -//! when handling `DefaultBranch` vs `Branch("master")` -//! -//! ### Repositories with `master` and a default branch -//! -//! This is one of the most obvious sources of breakage. If our `foo` example -//! in above had two branches, one called `master` and another which was -//! actually the main branch, then Cargo's change will always be a breaking -//! change. This is because what's downloaded is an entirely different branch -//! if we change the meaning of the dependency directive. -//! -//! It's expected this is quite rare, but to handle this case nonetheless when -//! Cargo fetches from a git remote and the dependency specification is -//! `DefaultBranch` then it will issue a warning if the `HEAD` reference -//! doesn't match `master`. It's expected in this situation that authors will -//! fix builds locally by specifying `branch = 'master'`. -//! -//! ### Differences in `cargo vendor` configuration -//! -//! When executing `cargo vendor` it will print out configuration which can -//! then be used to configure Cargo to use the `vendor` directory. Historically -//! this configuration looked like: -//! -//! ```toml -//! [source."https://example.org/foo"] -//! git = "https://example.org/foo" -//! branch = "master" -//! replace-with = "vendored-sources" -//! ``` -//! -//! We would like to, however, transition this to not include the `branch = -//! "master"` unless the dependency directive actually mentions a branch. -//! Conveniently older Cargo implementations all interpret a missing `branch` -//! as `branch = "master"` so it's a backwards-compatible change to remove the -//! `branch = "master"` directive. As a result, `cargo vendor` will no longer -//! emit a `branch` if the git reference is `DefaultBranch` -//! -//! ### Differences in lock file formats -//! -//! Another issue pointed out in #8364 was that `Cargo.lock` files were no -//! longer compatible on stable and nightly with each other. The underlying -//! issue is that Cargo was serializing `branch = "master"` *differently* on -//! nightly than it was on stable. Historical implementations of Cargo would -//! encode `DefaultBranch` and `Branch("master")` the same way in `Cargo.lock`, -//! so when reading a lock file we have no way of differentiating between the -//! two. -//! -//! To handle this difference in encoding of `Cargo.lock` we'll be employing -//! the standard scheme to change `Cargo.lock`: -//! -//! * Add support in Cargo for a future format, don't turn it on. -//! * Wait a long time -//! * Turn on the future format -//! -//! Here the "future format" is `branch=master` shows up if you have a `branch` -//! in `Cargo.toml`, and otherwise nothing shows up in URLs. Due to the effect -//! on crate graph resolution, however, this flows into the next point.. -//! -//! ### Unification in the Cargo dependency graph -//! -//! Today dependencies with `branch = "master"` will unify with dependencies -//! that say nothing. (that's because the latter simply desugars). This means -//! the two `foo` directives above will resolve to the same dependency. -//! -//! The best idea I've got to fix this is to basically get everyone (if anyone) -//! to stop doing this today. The crate graph resolver will start to warn if it -//! detects that multiple `Cargo.toml` directives are detected and mixed. The -//! thinking is that when we turn on the new lock file format it'll also be -//! hard breaking change for any project which still has dependencies to -//! both the `master` branch and not. -//! -//! ### What we're doing today -//! -//! The general goal of Cargo today is to internally distinguish -//! `DefaultBranch` and `Branch("master")`, but for the time being they should -//! be functionally equivalent in terms of builds. The hope is that we'll let -//! all these warnings and such bake for a good long time, and eventually we'll -//! flip some switches if your build has no warnings it'll work before and -//! after. -//! -//! That's the dream at least, we'll see how this plays out. use crate::core::GitReference; use crate::util::errors::{CargoResult, CargoResultExt}; @@ -182,7 +77,7 @@ impl GitRemote { } pub fn rev_for(&self, path: &Path, reference: &GitReference) -> CargoResult { - reference.resolve(&self.db_at(path)?.repo, None) + reference.resolve(&self.db_at(path)?.repo) } pub fn checkout( @@ -207,7 +102,7 @@ impl GitRemote { } } None => { - if let Ok(rev) = reference.resolve(&db.repo, Some((&self.url, cargo_config))) { + if let Ok(rev) = reference.resolve(&db.repo) { return Ok((db, rev)); } } @@ -226,7 +121,7 @@ impl GitRemote { .context(format!("failed to clone into: {}", into.display()))?; let rev = match locked_rev { Some(rev) => rev, - None => reference.resolve(&repo, Some((&self.url, cargo_config)))?, + None => reference.resolve(&repo)?, }; Ok(( @@ -295,21 +190,13 @@ impl GitDatabase { self.repo.revparse_single(&oid.to_string()).is_ok() } - pub fn resolve( - &self, - r: &GitReference, - remote_and_config: Option<(&Url, &Config)>, - ) -> CargoResult { - r.resolve(&self.repo, remote_and_config) + pub fn resolve(&self, r: &GitReference) -> CargoResult { + r.resolve(&self.repo) } } impl GitReference { - pub fn resolve( - &self, - repo: &git2::Repository, - remote_and_config: Option<(&Url, &Config)>, - ) -> CargoResult { + pub fn resolve(&self, repo: &git2::Repository) -> CargoResult { let id = match self { // Note that we resolve the named tag here in sync with where it's // fetched into via `fetch` below. @@ -334,38 +221,11 @@ impl GitReference { .ok_or_else(|| anyhow::format_err!("branch `{}` did not have a target", s))? } - // See the module docs for why we're using `master` here. + // We'll be using the HEAD commit GitReference::DefaultBranch => { - let master = repo - .find_branch("origin/master", git2::BranchType::Remote) - .chain_err(|| "failed to find branch `master`")?; - let master = master - .get() - .target() - .ok_or_else(|| anyhow::format_err!("branch `master` did not have a target"))?; - - if let Some((remote, config)) = remote_and_config { - let head_id = repo.refname_to_id("refs/remotes/origin/HEAD")?; - let head = repo.find_object(head_id, None)?; - let head = head.peel(ObjectType::Commit)?.id(); - - if head != master { - config.shell().warn(&format!( - "\ - fetching `master` branch from `{}` but the `HEAD` \ - reference for this repository is not the \ - `master` branch. This behavior will change \ - in Cargo in the future and your build may \ - break, so it's recommended to place \ - `branch = \"master\"` in Cargo.toml when \ - depending on this git repository to ensure \ - that your build will continue to work.\ - ", - remote, - ))?; - } - } - master + let head_id = repo.refname_to_id("refs/remotes/origin/HEAD")?; + let head = repo.find_object(head_id, None)?; + head.peel(ObjectType::Commit)?.id() } GitReference::Rev(s) => { @@ -899,8 +759,6 @@ pub fn fetch( } GitReference::DefaultBranch => { - // See the module docs for why we're fetching `master` here. - refspecs.push(String::from("refs/heads/master:refs/remotes/origin/master")); refspecs.push(String::from("HEAD:refs/remotes/origin/HEAD")); } @@ -1166,10 +1024,7 @@ fn github_up_to_date( handle.useragent("cargo")?; let mut headers = List::new(); headers.append("Accept: application/vnd.github.3.sha")?; - headers.append(&format!( - "If-None-Match: \"{}\"", - reference.resolve(repo, None)? - ))?; + headers.append(&format!("If-None-Match: \"{}\"", reference.resolve(repo)?))?; handle.http_headers(headers)?; handle.perform()?; Ok(handle.response_code()? == 304) diff --git a/src/cargo/sources/registry/remote.rs b/src/cargo/sources/registry/remote.rs index d3f9eb9c03c..91bf3301b21 100644 --- a/src/cargo/sources/registry/remote.rs +++ b/src/cargo/sources/registry/remote.rs @@ -106,7 +106,7 @@ impl<'cfg> RemoteRegistry<'cfg> { fn head(&self) -> CargoResult { if self.head.get().is_none() { let repo = self.repo()?; - let oid = self.index_git_ref.resolve(repo, None)?; + let oid = self.index_git_ref.resolve(repo)?; self.head.set(Some(oid)); } Ok(self.head.get().unwrap()) diff --git a/tests/testsuite/git.rs b/tests/testsuite/git.rs index 548d7264c2a..2d22e5e1edd 100644 --- a/tests/testsuite/git.rs +++ b/tests/testsuite/git.rs @@ -2787,66 +2787,6 @@ to proceed despite [..] git_project.cargo("package --no-verify").run(); } -#[cargo_test] -fn default_not_master() { - let project = project(); - - // Create a repository with a `master` branch, but switch the head to a - // branch called `main` at the same time. - let (git_project, repo) = git::new_repo("dep1", |project| { - project - .file("Cargo.toml", &basic_lib_manifest("dep1")) - .file("src/lib.rs", "pub fn foo() {}") - }); - let head_id = repo.head().unwrap().target().unwrap(); - let head = repo.find_commit(head_id).unwrap(); - repo.branch("main", &head, false).unwrap(); - repo.set_head("refs/heads/main").unwrap(); - - // Then create a commit on the new `main` branch so `master` and `main` - // differ. - git_project.change_file("src/lib.rs", ""); - git::add(&repo); - git::commit(&repo); - - let project = project - .file( - "Cargo.toml", - &format!( - r#" - [project] - name = "foo" - version = "0.5.0" - - [dependencies] - dep1 = {{ git = '{}' }} - "#, - git_project.url() - ), - ) - .file("src/lib.rs", "pub fn foo() { dep1::foo() }") - .build(); - - project - .cargo("build") - .with_stderr( - "\ -[UPDATING] git repository `[..]` -warning: fetching `master` branch from `[..]` but the `HEAD` \ - reference for this repository is not the \ - `master` branch. This behavior will change \ - in Cargo in the future and your build may \ - break, so it's recommended to place \ - `branch = \"master\"` in Cargo.toml when \ - depending on this git repository to ensure \ - that your build will continue to work. -[COMPILING] dep1 v0.5.0 ([..]) -[COMPILING] foo v0.5.0 ([..]) -[FINISHED] dev [unoptimized + debuginfo] target(s) in [..]", - ) - .run(); -} - #[cargo_test] fn historical_lockfile_works() { let project = project(); @@ -2959,68 +2899,6 @@ dependencies = [ project.cargo("build").run(); } -#[cargo_test] -fn two_dep_forms() { - let project = project(); - - let (git_project, _repo) = git::new_repo("dep1", |project| { - project - .file("Cargo.toml", &basic_lib_manifest("dep1")) - .file("src/lib.rs", "") - }); - - let project = project - .file( - "Cargo.toml", - &format!( - r#" - [project] - name = "foo" - version = "0.5.0" - - [dependencies] - dep1 = {{ git = '{}', branch = 'master' }} - a = {{ path = 'a' }} - "#, - git_project.url() - ), - ) - .file("src/lib.rs", "") - .file( - "a/Cargo.toml", - &format!( - r#" - [project] - name = "a" - version = "0.5.0" - - [dependencies] - dep1 = {{ git = '{}' }} - "#, - git_project.url() - ), - ) - .file("a/src/lib.rs", "") - .build(); - - project - .cargo("build") - .with_stderr( - "\ -[UPDATING] [..] -warning: two git dependencies found for `[..]` where one uses `branch = \"master\"` \ -and the other doesn't; this will break in a future version of Cargo, so please \ -ensure the dependency forms are consistent -warning: [..] -[COMPILING] [..] -[COMPILING] [..] -[COMPILING] [..] -[FINISHED] [..] -", - ) - .run(); -} - #[cargo_test] fn metadata_master_consistency() { // SourceId consistency in the `cargo metadata` output when `master` is diff --git a/tests/testsuite/lockfile_compat.rs b/tests/testsuite/lockfile_compat.rs index 265bc43ef7a..2f7972344db 100644 --- a/tests/testsuite/lockfile_compat.rs +++ b/tests/testsuite/lockfile_compat.rs @@ -25,6 +25,8 @@ fn oldest_lockfile_still_works_with_command(cargo_command: &str) { let expected_lockfile = r#"# This file is automatically @generated by Cargo. # It is not intended for manual editing. +version = 3 + [[package]] name = "bar" version = "0.1.0" @@ -169,6 +171,8 @@ source = "registry+https://github.com/rust-lang/crates.io-index" assert_lockfiles_eq( r#"# This file is automatically @generated by Cargo. # It is not intended for manual editing. +version = 3 + [[package]] name = "bar" version = "0.1.0" @@ -407,6 +411,8 @@ fn current_lockfile_format() { let expected = "\ # This file is automatically @generated by Cargo.\n# It is not intended for manual editing. +version = 3 + [[package]] name = \"bar\" version = \"0.1.0\" @@ -467,6 +473,8 @@ dependencies = [ assert_lockfiles_eq( r#"# [..] # [..] +version = 3 + [[package]] name = "bar" version = "0.1.0" diff --git a/tests/testsuite/update.rs b/tests/testsuite/update.rs index 93f6eb6846c..eed45439358 100644 --- a/tests/testsuite/update.rs +++ b/tests/testsuite/update.rs @@ -584,7 +584,7 @@ fn preserve_top_comment() { let mut lines = lockfile.lines().collect::>(); lines.insert(2, "# some other comment"); let mut lockfile = lines.join("\n"); - lockfile.push_str("\n\n"); // .lines/.join loses the last newline + lockfile.push_str("\n"); // .lines/.join loses the last newline println!("saving Cargo.lock contents:\n{}", lockfile); p.change_file("Cargo.lock", &lockfile); From c4b53c956f56a2be48a42e4893a08b26bc4b6f87 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Thu, 4 Feb 2021 10:37:26 -0800 Subject: [PATCH 2/4] Fix a bug loading v2 lockfiles This commit fixes a bug in Cargo where after `DefaultBranch` and `Branch("master")` are considered distinct no v2 lockfile would by default match a dependency specification with the `branch = 'master'` annotation. A feature of the v2 lockfile, however, is that no mention of a branch is equivalent to the `master` branch. The bug here is fixed by updating the code which registers a previous lock file with our `PackageRegistry`. This code registers nodes, only with v2 lock files, for both default and `master` branch git dependencies. This way requests for either one will get matched now that they're considered distinct. --- src/cargo/ops/resolve.rs | 32 +++++++++++++++++++++++++++++--- 1 file changed, 29 insertions(+), 3 deletions(-) diff --git a/src/cargo/ops/resolve.rs b/src/cargo/ops/resolve.rs index c54a2bc42df..9cab4434f8b 100644 --- a/src/cargo/ops/resolve.rs +++ b/src/cargo/ops/resolve.rs @@ -13,10 +13,12 @@ use crate::core::compiler::{CompileKind, RustcTargetData}; use crate::core::registry::PackageRegistry; use crate::core::resolver::features::{FeatureResolver, ForceAllTargets, ResolvedFeatures}; -use crate::core::resolver::{self, HasDevUnits, Resolve, ResolveOpts}; +use crate::core::resolver::{self, HasDevUnits, Resolve, ResolveOpts, ResolveVersion}; use crate::core::summary::Summary; use crate::core::Feature; -use crate::core::{PackageId, PackageIdSpec, PackageSet, Source, SourceId, Workspace}; +use crate::core::{ + GitReference, PackageId, PackageIdSpec, PackageSet, Source, SourceId, Workspace, +}; use crate::ops; use crate::sources::PathSource; use crate::util::errors::{CargoResult, CargoResultExt}; @@ -597,7 +599,31 @@ fn register_previous_locks( .deps_not_replaced(node) .map(|p| p.0) .filter(keep) - .collect(); + .collect::>(); + + // In the v2 lockfile format and prior the `branch=master` dependency + // directive was serialized the same way as the no-branch-listed + // directive. Nowadays in Cargo, however, these two directives are + // considered distinct and are no longer represented the same way. To + // maintain compatibility with older lock files we register locked nodes + // for *both* the master branch and the default branch. + // + // Note that this is only applicable for loading older resolves now at + // this point. All new lock files are encoded as v3-or-later, so this is + // just compat for loading an old lock file successfully. + if resolve.version() <= ResolveVersion::V2 { + let source = node.source_id(); + if let Some(GitReference::DefaultBranch) = source.git_reference() { + let new_source = + SourceId::for_git(source.url(), GitReference::Branch("master".to_string())) + .unwrap() + .with_precise(source.precise().map(|s| s.to_string())); + + let node = node.with_source_id(new_source); + registry.register_lock(node, deps.clone()); + } + } + registry.register_lock(node, deps); } From 1fefa5de262359cf0fa64ed65544391c6c4ee35c Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Tue, 9 Feb 2021 07:29:01 -0800 Subject: [PATCH 3/4] Add back in deleted tests --- tests/testsuite/git.rs | 112 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 112 insertions(+) diff --git a/tests/testsuite/git.rs b/tests/testsuite/git.rs index 2d22e5e1edd..74a5d62997a 100644 --- a/tests/testsuite/git.rs +++ b/tests/testsuite/git.rs @@ -2787,6 +2787,57 @@ to proceed despite [..] git_project.cargo("package --no-verify").run(); } +#[cargo_test] +fn default_not_master() { + let project = project(); + + // Create a repository with a `master` branch, but switch the head to a + // branch called `main` at the same time. + let (git_project, repo) = git::new_repo("dep1", |project| { + project + .file("Cargo.toml", &basic_lib_manifest("dep1")) + .file("src/lib.rs", "pub fn foo() {}") + }); + let head_id = repo.head().unwrap().target().unwrap(); + let head = repo.find_commit(head_id).unwrap(); + repo.branch("main", &head, false).unwrap(); + repo.set_head("refs/heads/main").unwrap(); + + // Then create a commit on the new `main` branch so `master` and `main` + // differ. + git_project.change_file("src/lib.rs", "pub fn bar() {}"); + git::add(&repo); + git::commit(&repo); + + let project = project + .file( + "Cargo.toml", + &format!( + r#" + [project] + name = "foo" + version = "0.5.0" + [dependencies] + dep1 = {{ git = '{}' }} + "#, + git_project.url() + ), + ) + .file("src/lib.rs", "pub fn foo() { dep1::bar() }") + .build(); + + project + .cargo("build") + .with_stderr( + "\ +[UPDATING] git repository `[..]` +[COMPILING] dep1 v0.5.0 ([..]) +[COMPILING] foo v0.5.0 ([..]) +[FINISHED] dev [unoptimized + debuginfo] target(s) in [..]", + ) + .run(); +} + #[cargo_test] fn historical_lockfile_works() { let project = project(); @@ -2899,6 +2950,67 @@ dependencies = [ project.cargo("build").run(); } +#[cargo_test] +fn two_dep_forms() { + let project = project(); + + let (git_project, _repo) = git::new_repo("dep1", |project| { + project + .file("Cargo.toml", &basic_lib_manifest("dep1")) + .file("src/lib.rs", "") + }); + + let project = project + .file( + "Cargo.toml", + &format!( + r#" + [project] + name = "foo" + version = "0.5.0" + [dependencies] + dep1 = {{ git = '{}', branch = 'master' }} + a = {{ path = 'a' }} + "#, + git_project.url() + ), + ) + .file("src/lib.rs", "") + .file( + "a/Cargo.toml", + &format!( + r#" + [project] + name = "a" + version = "0.5.0" + [dependencies] + dep1 = {{ git = '{}' }} + "#, + git_project.url() + ), + ) + .file("a/src/lib.rs", "") + .build(); + + // This'll download the git repository twice, one with HEAD and once with + // the master branch. Then it'll compile 4 crates, the 2 git deps, then + // the two local deps. + project + .cargo("build") + .with_stderr( + "\ +[UPDATING] [..] +[UPDATING] [..] +[COMPILING] [..] +[COMPILING] [..] +[COMPILING] [..] +[COMPILING] [..] +[FINISHED] [..] +", + ) + .run(); +} + #[cargo_test] fn metadata_master_consistency() { // SourceId consistency in the `cargo metadata` output when `master` is From 9f2ce1ffefc6ae4631312b5fda27006cab39f38b Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Tue, 9 Feb 2021 07:30:08 -0800 Subject: [PATCH 4/4] Squash warnings on the nightly channel --- crates/cargo-test-support/src/cross_compile.rs | 2 +- tests/testsuite/cargo_command.rs | 2 +- tests/testsuite/new.rs | 14 +++++++++++--- 3 files changed, 13 insertions(+), 5 deletions(-) diff --git a/crates/cargo-test-support/src/cross_compile.rs b/crates/cargo-test-support/src/cross_compile.rs index 7d3ec335301..ff272d482f3 100644 --- a/crates/cargo-test-support/src/cross_compile.rs +++ b/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. diff --git a/tests/testsuite/cargo_command.rs b/tests/testsuite/cargo_command.rs index 75eec676dfb..04e3051c785 100644 --- a/tests/testsuite/cargo_command.rs +++ b/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); } diff --git a/tests/testsuite/new.rs b/tests/testsuite/new.rs index 548810bce26..9a7f71cf125 100644 --- a/tests/testsuite/new.rs +++ b/tests/testsuite/new.rs @@ -370,7 +370,11 @@ fn finds_git_author() { let toml = paths::root().join("foo/Cargo.toml"); let contents = fs::read_to_string(&toml).unwrap(); - assert!(contents.contains(r#"authors = ["foo "]"#), contents); + assert!( + contents.contains(r#"authors = ["foo "]"#), + "{}", + contents + ); } #[cargo_test] @@ -411,7 +415,11 @@ fn finds_git_author_in_included_config() { cargo_process("new foo/bar").run(); let toml = paths::root().join("foo/bar/Cargo.toml"); let contents = fs::read_to_string(&toml).unwrap(); - assert!(contents.contains(r#"authors = ["foo "]"#), contents,); + assert!( + contents.contains(r#"authors = ["foo "]"#), + "{}", + contents + ); } #[cargo_test] @@ -632,7 +640,7 @@ fn new_with_blank_email() { .run(); let contents = fs::read_to_string(paths::root().join("foo/Cargo.toml")).unwrap(); - assert!(contents.contains(r#"authors = ["Sen"]"#), contents); + assert!(contents.contains(r#"authors = ["Sen"]"#), "{}", contents); } #[cargo_test]