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.

Sign up for GitHub

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

Jump to bottom

Improve support for non-master main branches #8364

Merged
merged 1 commit into from
Jun 24, 2020
Merged

Improve support for non-master main branches #8364

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion src/bin/cargo/commands/install.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ pub fn exec(config: &mut Config, args: &ArgMatches<'_>) -> CliResult {
} else if let Some(rev) = args.value_of("rev") {
GitReference::Rev(rev.to_string())
} else {
GitReference::Branch("master".to_string())
GitReference::DefaultBranch
};
SourceId::for_git(&url, gitref)?
} else if let Some(path) = args.value_of_path("path", config) {
Expand Down
17 changes: 10 additions & 7 deletions src/cargo/core/source/source_id.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,10 +63,12 @@ enum SourceKind {
pub enum GitReference {
/// From a tag.
Tag(String),
/// From the HEAD of a branch.
/// From a branch.
Branch(String),
/// From a specific revision.
Rev(String),
/// The default branch of the repository, the reference named `HEAD`.
DefaultBranch,
}

impl SourceId {
Expand Down Expand Up @@ -114,7 +116,7 @@ impl SourceId {
match kind {
"git" => {
let mut url = url.into_url()?;
let mut reference = GitReference::Branch("master".to_string());
let mut reference = GitReference::DefaultBranch;
for (k, v) in url.query_pairs() {
match &k[..] {
// Map older 'ref' to branch.
Expand Down Expand Up @@ -549,10 +551,10 @@ impl<'a> fmt::Display for SourceIdIntoUrl<'a> {

impl GitReference {
/// Returns a `Display`able view of this git reference, or None if using
/// the head of the "master" branch
/// the head of the default branch
pub fn pretty_ref(&self) -> Option<PrettyRef<'_>> {
match *self {
GitReference::Branch(ref s) if *s == "master" => None,
GitReference::DefaultBranch => None,
_ => Some(PrettyRef { inner: self }),
}
}
Expand All @@ -569,6 +571,7 @@ impl<'a> fmt::Display for PrettyRef<'a> {
GitReference::Branch(ref b) => write!(f, "branch={}", b),
GitReference::Tag(ref s) => write!(f, "tag={}", s),
GitReference::Rev(ref s) => write!(f, "rev={}", s),
GitReference::DefaultBranch => unreachable!(),
}
}
}
Expand All @@ -581,11 +584,11 @@ mod tests {
#[test]
fn github_sources_equal() {
let loc = "https://github.com/foo/bar".into_url().unwrap();
let master = SourceKind::Git(GitReference::Branch("master".to_string()));
let s1 = SourceId::new(master.clone(), loc).unwrap();
let default = SourceKind::Git(GitReference::DefaultBranch);
let s1 = SourceId::new(default.clone(), loc).unwrap();

let loc = "git://github.com/foo/bar".into_url().unwrap();
let s2 = SourceId::new(master, loc.clone()).unwrap();
let s2 = SourceId::new(default, loc.clone()).unwrap();

assert_eq!(s1, s2);

Expand Down
1 change: 1 addition & 0 deletions src/cargo/ops/vendor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,6 +275,7 @@ fn sync(
GitReference::Branch(ref b) => branch = Some(b.clone()),
GitReference::Tag(ref t) => tag = Some(t.clone()),
GitReference::Rev(ref r) => rev = Some(r.clone()),
GitReference::DefaultBranch => {}
}
}
VendorSource::Git {
Expand Down
2 changes: 1 addition & 1 deletion src/cargo/sources/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,7 @@ restore the source replacement configuration to continue the build
Some(b) => GitReference::Tag(b.val),
None => match def.rev {
Some(b) => GitReference::Rev(b.val),
None => GitReference::Branch("master".to_string()),
None => GitReference::DefaultBranch,
},
},
};
Expand Down
6 changes: 1 addition & 5 deletions src/cargo/sources/git/source.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,10 +261,6 @@ mod test {
}

fn src(s: &str) -> SourceId {
SourceId::for_git(
&s.into_url().unwrap(),
GitReference::Branch("master".to_string()),
)
.unwrap()
SourceId::for_git(&s.into_url().unwrap(), GitReference::DefaultBranch).unwrap()
}
}
14 changes: 14 additions & 0 deletions src/cargo/sources/git/utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,6 +250,14 @@ impl GitReference {
.target()
.ok_or_else(|| anyhow::format_err!("branch `{}` did not have a target", s))?
}
GitReference::DefaultBranch => {
let refname = "refs/remotes/origin/HEAD";
let id = repo.refname_to_id(refname)?;
let obj = repo.find_object(id, None)?;
let obj = obj.peel(ObjectType::Commit)?;
obj.id()
}

GitReference::Rev(s) => {
let obj = repo.revparse_single(s)?;
match obj.as_tag() {
Expand Down Expand Up @@ -734,11 +742,16 @@ pub fn fetch(
refspecs.push(format!("refs/tags/{0}:refs/remotes/origin/tags/{0}", t));
}

GitReference::DefaultBranch => {
refspecs.push(format!("HEAD:refs/remotes/origin/HEAD"));
}

// For `rev` dependencies we don't know what the rev will point to. To
// handle this situation we fetch all branches and tags, and then we
// pray it's somewhere in there.
GitReference::Rev(_) => {
refspecs.push(format!("refs/heads/*:refs/remotes/origin/*"));
refspecs.push(format!("HEAD:refs/remotes/origin/HEAD"));
tags = true;
}
}
Expand Down Expand Up @@ -957,6 +970,7 @@ fn github_up_to_date(
let github_branch_name = match reference {
GitReference::Branch(branch) => branch,
GitReference::Tag(tag) => tag,
GitReference::DefaultBranch => "HEAD",
GitReference::Rev(_) => {
debug!("can't use github fast path with `rev`");
return Ok(false);
Expand Down
2 changes: 1 addition & 1 deletion src/cargo/sources/registry/remote.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ impl<'cfg> RemoteRegistry<'cfg> {
source_id,
config,
// TODO: we should probably make this configurable
index_git_ref: GitReference::Branch("master".to_string()),
index_git_ref: GitReference::DefaultBranch,
tree: RefCell::new(None),
repo: LazyCell::new(),
head: Cell::new(None),
Expand Down
2 changes: 1 addition & 1 deletion src/cargo/util/toml/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1686,7 +1686,7 @@ impl DetailedTomlDependency {
.map(GitReference::Branch)
.or_else(|| self.tag.clone().map(GitReference::Tag))
.or_else(|| self.rev.clone().map(GitReference::Rev))
.unwrap_or_else(|| GitReference::Branch("master".to_string()));
.unwrap_or_else(|| GitReference::DefaultBranch);
let loc = git.into_url()?;

if let Some(fragment) = loc.fragment() {
Expand Down
2 changes: 1 addition & 1 deletion src/doc/src/reference/specifying-dependencies.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ Cargo will fetch the `git` repository at this location then look for a
of a workspace and setting `git` to the repository containing the workspace).

Since we haven’t specified any other information, Cargo assumes that
we intend to use the latest commit on the `master` branch to build our package.
we intend to use the latest commit on the main branch to build our package.
You can combine the `git` key with the `rev`, `tag`, or `branch` keys to
specify something else. Here's an example of specifying that you want to use
the latest commit on a branch named `next`:
Expand Down
1 change: 0 additions & 1 deletion tests/build-std/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ use std::path::Path;
fn enable_build_std(e: &mut Execs, arg: Option<&str>) {
e.env_remove("CARGO_HOME");
e.env_remove("HOME");
e.arg("-Zno-index-update");
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

This ended up being a bit of an odd coincidence, but what was happening is that this test relies on the index being updated by the Cargo used to drive the tests. The cargo run here is assumed to read that index and work with it correctly. If we ever change anything about how the index is checked out, then this breaks because the new cargo can't read the old index.

That's basically what happened here where the new Cargo is requiring refs/remotes/origin/HEAD to resolve the most recent commit of the index, but previous Cargo would only fill in refs/remotes/origin/master. This meant that with -Zno-index-update the new cargo binary would fail to update the index and would think there were no crates.

I can't actually recall the original purpose for this argument myself, but it didn't seem to slow down too too much when adding this back in.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, that's a bit worrisome. I think there are a fair number of users of Zno-index-update, and it looks like it just breaks with a pre-existing index. Would it be possible to handle that?

This was added from the original build-std tests where there were over 30 of them. In #7350 the build-std suite was improved where only 3 "full" tests were moved to build-std/main.rs.

Copy link
Contributor

Choose a reason for hiding this comment

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

I should have said, is there an easy way to handle it? If not, I guess since it is an unstable feature, might not be worth putting in the effort if it isn't easy.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh I think this is only localized to this test suite. If you were, for example, to do:

$ cargo +nightly test --no-run
$ rm -rf $HOME/.cargo/registry
$ CARGO_RUN_BUILD_STD_TESTS=1 rustup run nightly ./target/debug/deps/build_std-*

this breaks today. The tests rely on the index already being up-to-date, and this just happens to work because the Cargo we're testing and the outer cargo (which ran cargo +nightly test) agree on the index format, so the tests know they'll be able to use the index the outer Cargo populated.

We could perhaps handle this by fetching the HEAD reference into refs/remotes/origin/master and then reading that, but I'd just want to confirm that we're on the same page about the fallout we're dealing with.

Copy link
Contributor

Choose a reason for hiding this comment

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

The fallout I'm thinking of, is if someone has the registry populated from a current version of Cargo, and then they update to a nightly with this PR, and the first command they run is cargo build -Zno-index-update, it will fail with error: no matching package named …, which is a bit confusing.

I'm not sure what the likelyhood is that someone runs with -Zno-index-update as the very first command after updating. As I mentioned, only bother if it is easy to handle. It seems like it is unlikely that it would be the first command someone would run. I believe Crater does an initial fetch, so crater should be safe I would think.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's true that it's a confusing error, yeah. Do you think it's worth trying to return a better error?

Copy link
Contributor

Choose a reason for hiding this comment

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

Eh. I'm inclined to say "no" because it is an unstable feature, and I think it is unlikely that someone will run that as the first command.

r=me if you think this is ready to go.


// And finally actually enable `build-std` for now
let arg = match arg {
Expand Down
48 changes: 48 additions & 0 deletions tests/testsuite/git.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2784,3 +2784,51 @@ to proceed despite [..]
git::commit(&repo);
git_project.cargo("package --no-verify").run();
}

#[cargo_test]
fn default_not_master() {
let project = project();

// Create a repository with a `master` branch ...
let (git_project, repo) = git::new_repo("dep1", |project| {
project.file("Cargo.toml", &basic_lib_manifest("dep1"))
});

// Then create a `main` branch with actual code, and set the head of the
// repository (default branch) to `main`.
git_project.change_file("src/lib.rs", "pub fn foo() {}");
git::add(&repo);
let rev = git::commit(&repo);
let commit = repo.find_commit(rev).unwrap();
repo.branch("main", &commit, false).unwrap();
repo.set_head("refs/heads/main").unwrap();

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 `[..]`
[COMPILING] dep1 v0.5.0 ([..])
[COMPILING] foo v0.5.0 ([..])
[FINISHED] dev [unoptimized + debuginfo] target(s) in [..]",
)
.run();
}