Improve support for non-`master` main branches #8364

alexcrichton · 2020-06-15T21:22:03Z

This commit improves Cargo's support for git repositories whose "main
branch" is not called master. Cargo currently pretty liberally assumes
that if nothing else about a git repository is specified then master
is the branch name to use. Instead now Cargo has a fourth option as the
desired reference of a repository named DefaultBranch. Cargo doesn't
know anything about the actual name of the default branch, it just
updates how git references are fetched internally.

This commit is motivated by news that GitHub is likely to switch away
from the default branch being named master in the near future. It
would be a bit of a bummer if from now on everyone had to type
branch = '...', so this tries to improve that!

Closes #3517

rust-highfive · 2020-06-15T21:22:10Z

r? @ehuss

(rust_highfive has picked a reviewer for you, use r? to override)

alexcrichton · 2020-06-15T21:24:08Z

Note that this builds on the internal refactorings of #8363, so the first commit of that is included here.

This is also something we'll likely want to carefully approach, it's by default a breaking change to Cargo. (if a git repo has a non-default branch named master a git dep in Cargo to that repo without any other indicator will switch now from master to the default branch). I do think that practically the breakage is likely to be small because it's super rare I've seen a repo with a non-default master branch and it seems pretty unlikely that if that were the case that it'd also have a master branch and Cargo would depend on it, but explicitly the master branch. As usual I don't really have data to back up this hunch, it's just, well, a hunch.

Erk- · 2020-06-17T14:01:01Z

Related to #3517 I assume

bors · 2020-06-18T17:45:15Z

☔ The latest upstream changes (presumably #8363) made this pull request unmergeable. Please resolve the merge conflicts.

This commit improves Cargo's support for git repositories whose "main branch" is not called `master`. Cargo currently pretty liberally assumes that if nothing else about a git repository is specified then `master` is the branch name to use. Instead now Cargo has a fourth option as the desired reference of a repository named `DefaultBranch`. Cargo doesn't know anything about the actual name of the default branch, it just updates how git references are fetched internally. This commit is motivated by news that GitHub is likely to switch away from the default branch being named `master` in the near future. It would be a bit of a bummer if from now on everyone had to type `branch = '...'`, so this tries to improve that!

ehuss · 2020-06-18T18:15:24Z

tests/build-std/main.rs

@@ -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");


Why was this removed?

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.

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.

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.

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.

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.

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

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.

joshtriplett · 2020-06-18T23:10:57Z

Does crater's support for building git repositories (other than those represented by crates.io) extend to building repositories with git dependencies? If so, we could run this through crater.

I think, given appropriate announcement in advance, we could make this change. (Nit: we might wish to spell it HEAD rather than DefaultBranch.)

kaimast · 2020-06-19T04:45:26Z

Thanks for fixing this! Just ran into this bug after renaming my main branch.

alexcrichton · 2020-06-23T18:35:55Z

@joshtriplett that's a good point! I do believe a crater run would be possible if we'd like.

alexcrichton · 2020-06-24T14:15:44Z

@bors: r=ehuss

@ehuss I agree that this seems unlikely to cause many issues in practice, I think we should be ready to revert this if anything comes up though.

bors · 2020-06-24T14:15:45Z

📌 Commit 4c02977 has been approved by ehuss

bors · 2020-06-24T14:15:54Z

⌛ Testing commit 4c02977 with merge 16a1b0bcb3150fda94d9e47ac05f38dc0c9bc3ed...

bors · 2020-06-24T14:47:36Z

💔 Test failed - checks-azure

alexcrichton · 2020-06-24T19:20:27Z

@bors: retry

bors · 2020-06-24T19:20:36Z

⌛ Testing commit 4c02977 with merge 160318faac7d03267c86dd40245be0e99b4accd3...

bors · 2020-06-24T20:00:27Z

💔 Test failed - checks-azure

alexcrichton · 2020-06-24T20:09:04Z

@bors: retry

bors · 2020-06-24T20:09:13Z

⌛ Testing commit 4c02977 with merge f2aba14...

bors · 2020-06-24T20:34:38Z

☀️ Test successful - checks-azure
Approved by: ehuss
Pushing f2aba14 to master...

Update cargo ## cargo 10 commits in c26576f9adddd254b3dd63aecba176434290a9f6..305eaf0dc5f5a38d6e8041319c2da95b71cf6a4a 2020-06-23 16:21:21 +0000 to 2020-06-30 14:16:08 +0000 - Update core-foundation requirement from 0.7.0 to 0.9.0 (rust-lang/cargo#8432) - Parse `# env-dep` directives in dep-info files (rust-lang/cargo#8421) - Move string interning to util (rust-lang/cargo#8419) - Expose built cdylib artifacts in the Compilation structure (rust-lang/cargo#8418) - Remove unused serde_derive dependency from the crates.io crate (rust-lang/cargo#8416) - Remove unused remove_dir_all dependency (rust-lang/cargo#8412) - Improve git error messages a bit (rust-lang/cargo#8409) - Improve the description of Config.home_path (rust-lang/cargo#8408) - Improve support for non-`master` main branches (rust-lang/cargo#8364) - Document that OUT_DIR in JSON messages is an absolute path (rust-lang/cargo#8403) ## rls 2020-06-19 15:36:00 +0200 to 2020-06-30 23:34:52 +0200 - Update cargo (rust-lang/rls#1686)

Update cargo, rls ## cargo 14 commits in c26576f9adddd254b3dd63aecba176434290a9f6..fede83ccf973457de319ba6fa0e36ead454d2e20 2020-06-23 16:21:21 +0000 to 2020-07-02 21:51:34 +0000 - Fix overflow error on 32-bit. (rust-lang/cargo#8446) - Exclude the target directory from backups using CACHEDIR.TAG (rust-lang/cargo#8378) - CONTRIBUTING.md: Link to Zulip rather than Discord (rust-lang/cargo#8436) - Update built-in help for features (rust-lang/cargo#8433) - Update core-foundation requirement from 0.7.0 to 0.9.0 (rust-lang/cargo#8432) - Parse `# env-dep` directives in dep-info files (rust-lang/cargo#8421) - Move string interning to util (rust-lang/cargo#8419) - Expose built cdylib artifacts in the Compilation structure (rust-lang/cargo#8418) - Remove unused serde_derive dependency from the crates.io crate (rust-lang/cargo#8416) - Remove unused remove_dir_all dependency (rust-lang/cargo#8412) - Improve git error messages a bit (rust-lang/cargo#8409) - Improve the description of Config.home_path (rust-lang/cargo#8408) - Improve support for non-`master` main branches (rust-lang/cargo#8364) - Document that OUT_DIR in JSON messages is an absolute path (rust-lang/cargo#8403) ## rls 2020-06-19 15:36:00 +0200 to 2020-06-30 23:34:52 +0200 - Update cargo (rust-lang/rls#1686)

Fixes rust-lang#8468 which was a regression of rust-lang#8364 .

This commit is intended to be an effective but not literal revert of rust-lang#8364. Internally Cargo will still distinguish between `DefaultBranch` and `Branch("master")` when reading `Cargo.toml` files, but for almost all purposes the two are equivalent. This will namely fix the issue we have with lock file encodings where both are encoded with no `branch` (and without a branch it's parsed from a lock file as `DefaultBranch`). This will preserve the change that `cargo vendor` will not print out `branch = "master"` annotations but that desugars to match the lock file on the other end, so it should continue to work. Tests have been added in this commit for the regressions found on rust-lang#8468.

Prepare for not defaulting to master branch for git deps This PR is the "equivalent" of #8503 for the nightly channel of Rust (the master branch of Cargo). The purpose of this change is to fix the breakage from #8364 but to still pave a path forward to enabling the feature added in #8364, namely reinterpreting dependency directives without a `branch` directive as depending on `HEAD` insead of depending on `master`. This is a series of commits which implements a few mitigation strategies, but they're all adding up into a larger plan to roll out this change with, ideally, no breaking changes for those "reasonably" keeping up with Rust. The changes here are: * Cargo still internally differentiates between `DefaultBranch` and `Branch("master")`, so it knows what you wrote in the manifest. These two, however, hash and equate together so everything in Cargo considers them equivalent. * Cargo will now issue a warning whenever it fetches a repository pinned to `DefaultBranch` and the branch's `master` branch doesn't actually match `HEAD`. This avenue of breakage didn't arise but I added it here for completionism about not having to deal with breakage from this change again. * Cargo has now implemented a new future lockfile format. Internally this is dubbed "v3". The changes in this format is that it will have a `version` marker at the top of the file as well as encoding git dependencies different. Today `DefaultBranch` and `Branch("master")` encode the same way, but in the future they will encode differently. * Cargo now has a warning where if you have a mixture of `DefaultBranch` and `Branch("master")` in your build. The intention here is to get everyone on one or the other so the transition to the new lock file format can go smoothly. With all of these pieces in place the intention is that there is no breakage from #8364 as well. Additionally projects that *would* break will receive warnings. (note that projects "broken" today, those we've got issues for, will likely not get warnings, but that's because it was accidental we broke them). The long-term plan is to let this bake for quite some time, and then as part of the same change land an update to the new lock file format as well as "fixing" `SourceId` to consider these two directives different. When this change happens we should have guaranteed, for all currently active projects, they're either only using `branch = "master"` or no directive at all. We can therefore unambiguosly read the previous `Cargo.lock` to transition it into a new `Cargo.lock` which will either have `?branch=master` or nothing. I'm not sure how long this will need to bake for because unfortunately version changes to `Cargo.lock` cannot be taken lightly. We haven't even bumped the default format for old lock files to `V2` yet, they're still on `V1`. Hopefully in getting this in early-ish, though, we can at least get the ball rolling. Closes #8468

rust-highfive assigned ehuss Jun 15, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 15, 2020

alexcrichton mentioned this pull request Jun 15, 2020

Change default branch name bytecodealliance/wasmtime#1881

Closed

4 tasks

Erk- mentioned this pull request Jun 18, 2020

Head branch renamed to "trunk" twilight-rs/twilight#222

Closed

alexcrichton force-pushed the default-no-master branch from c83eb93 to 4c02977 Compare June 18, 2020 17:56

ehuss reviewed Jun 18, 2020

View reviewed changes

Eh2406 mentioned this pull request Jun 18, 2020

cargo install --git hardcodes master, doesn't use the default remote branch (HEAD) #8380

Closed

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 24, 2020

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 24, 2020

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 24, 2020

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 24, 2020

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 24, 2020

bors merged commit f2aba14 into rust-lang:master Jun 24, 2020

asajeffrey mentioned this pull request Jun 25, 2020

Rename master branch on all repos? servo/servo#27089

Closed

ehuss mentioned this pull request Jun 30, 2020

Update cargo, rls rust-lang/rust#73897

Merged

bkchr mentioned this pull request Jul 8, 2020

cargo build updates the git commit #8468

Closed

est31 mentioned this pull request Jul 10, 2020

Serialize GitReference::Branch("master") as GitReference::DefaultBranch #8475

Closed

est31 added a commit to est31/cargo that referenced this pull request Jul 10, 2020

Serialize GitReference::Branch("master") as GitReference::DefaultBranch

9cc3064

Fixes rust-lang#8468 which was a regression of rust-lang#8364 .

meltinglava mentioned this pull request Jul 16, 2020

Clean updating the crates.io index sets 'refs/remotes/origin/' to HEAD insted of master killercup/cargo-edit#411

Closed

alexcrichton mentioned this pull request Jul 20, 2020

Prepare for not defaulting to master branch for git deps #8522

Merged

alexcrichton deleted the default-no-master branch July 21, 2020 14:03

alexcrichton added a commit to alexcrichton/cargo that referenced this pull request Jul 23, 2020

Cherry-pick build-std fix from rust-lang#8364

2c36371

8573 mentioned this pull request Aug 5, 2020

Add lint(s) for git sources EmbarkStudios/cargo-deny#235

Closed

goto-bus-stop mentioned this pull request Aug 8, 2020

Rename main branch to main? http-rs/surf#211

Closed

avrong mentioned this pull request Mar 29, 2021

CI: update nightly rust version intellij-rust/intellij-rust#6374

Merged

ehuss added this to the 1.46.0 milestone Feb 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve support for non-`master` main branches #8364

Improve support for non-`master` main branches #8364

alexcrichton commented Jun 15, 2020 •

edited

Loading

rust-highfive commented Jun 15, 2020

alexcrichton commented Jun 15, 2020

Erk- commented Jun 17, 2020

bors commented Jun 18, 2020

ehuss Jun 18, 2020

alexcrichton Jun 18, 2020

ehuss Jun 18, 2020

ehuss Jun 18, 2020

alexcrichton Jun 23, 2020

ehuss Jun 23, 2020

alexcrichton Jun 23, 2020

ehuss Jun 23, 2020

joshtriplett commented Jun 18, 2020

kaimast commented Jun 19, 2020

alexcrichton commented Jun 23, 2020

alexcrichton commented Jun 24, 2020

bors commented Jun 24, 2020

bors commented Jun 24, 2020

bors commented Jun 24, 2020

alexcrichton commented Jun 24, 2020

bors commented Jun 24, 2020

bors commented Jun 24, 2020

alexcrichton commented Jun 24, 2020

bors commented Jun 24, 2020

bors commented Jun 24, 2020

Improve support for non-master main branches #8364

Improve support for non-master main branches #8364

Conversation

alexcrichton commented Jun 15, 2020 • edited Loading

rust-highfive commented Jun 15, 2020

alexcrichton commented Jun 15, 2020

Erk- commented Jun 17, 2020

bors commented Jun 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshtriplett commented Jun 18, 2020

kaimast commented Jun 19, 2020

alexcrichton commented Jun 23, 2020

alexcrichton commented Jun 24, 2020

bors commented Jun 24, 2020

bors commented Jun 24, 2020

bors commented Jun 24, 2020

alexcrichton commented Jun 24, 2020

bors commented Jun 24, 2020

bors commented Jun 24, 2020

alexcrichton commented Jun 24, 2020

bors commented Jun 24, 2020

bors commented Jun 24, 2020

Improve support for non-`master` main branches #8364

Improve support for non-`master` main branches #8364

alexcrichton commented Jun 15, 2020 •

edited

Loading