`cargo build` updates the git commit

[bkchr]

Hey, so we have the following problem:

cargo build in our repo with a Cargo.lock leads to the following output:

    Updating git repository `https://github.com/paritytech/substrate`
    Updating crates.io index

It tries to update the git commit, which leads to some other dependency problems.

Using cargo 1.46.0-nightly (fede83ccf 2020-07-02).

Doing the same with cargo 2020-06-26, it just starts building without updating the git commit.

I tracked it down to the following pr: #8364

If I revert this pr, it works.

TLDR: cargo now always updates the git commit using cargo build.

CC @alexcrichton as you have created the pr.

[bkchr]

FYI: We have everywhere branch = "master" in the Cargo.toml files.

[bkchr]

Okay, I think I know what the problem is:

source = "git+https://github.com/paritytech/substrate#59ee76a0f342ae0dc1c6a6777d24d7811d9261ef"
source = "git+https://github.com/paritytech/substrate?branch=master#d076f4705ee257b61c02c918755e63e3b34272ad"

The new syntax requires the branch=master?

[alexcrichton]

Thanks for the report! Could you expand a bit on how you're depending on the substrate git repo? Those two different source URLs looks like there's one dependency without branch and one with branch perhaps? That still shouldn't cause issues for Cargo (it's a bug that issues are happening), but I'm curious how we could reproduce locally to investigate.

[bkchr]

https://github.com/paritytech/polkadot/tree/v0.8.13

You can check this out and try to compile it.(it is enough to just compile for a second, you don't need to compile the full project) It will work with a cargo without your pr, if you add your pr it will fail to resolve a depdency (because it updated the substrate commit). I always deleted the target dir in between the runs.

What I have posted is the change that happened on master, after I updated the commit by intention. In the old format the master branch was not added to the cargo.lock and now it is added because it is not the default anymore. I think this is the reason why cargo tries to update the cargo.lock with your pr included. (but just guessing here)

[alexcrichton]

Ok so I've done some digging here and I think I see what's going on.

Before #8364 specifying branch = "master" in your Cargo.toml was 100% equivalent to specifying nothing. Specifying nothing desugared to as if you did branch = "master" internally. After #8364, however, these are now different ways to specify a dependency, where omitting a specifier means "default branch" whereas branch = "master" is now specifying you specifically want the master branch.

This had implications on Cargo.lock as well. Different branches in a git repo are different "sources" internally in Cargo, so we need to encode this information differently in Cargo.lock to differentiate (so you can pull in the same name/version from two branches if you want). The encoding of Cargo.lock includes ?branch=foo in the URL of git repositories for this purpose. In an attempt to be "pretty", however, ?branch=master was omitted. This means that your Cargo.lock does not mention ?branch=master anywhere inside of it.

With #8364, however, branch = "master" in Cargo.toml now encodes as ?branch=master in Cargo.lock. Specifying nothing is the only way to omit extra specifiers in Cargo.lock.

Anyway, putting all that together, checking out that repository and running cargo +nightly fetch fails. This is because Cargo sees the lock file but thinks it does not apply for any packages pointing to the substrate git repository. Cargo thinks nothing matches because the lock file says that source is the "default branch" whereas all the manifests say "use the master branch". Seeing this mismatch Cargo discards the lock file for those nodes, and attempts to do an incremental update. Cargo then fails the incremental update with an error message because things have changed in the meantime, and lots of other parts of the graph need to update first. All-in-all this is an encoding disagreement between Cargo versions, and due to the complexity of the dependency graph it's showing up as a dependency update error by accident.

---

Ok now what to do about this. Some possibilities I can think of:

• We did know that Improve support for non-master main branches  #8364 was technically a breaking change. We hypothesized, however, that the impact would be pretty small. This is clearly a point in the other direction! One possibility is to back out Improve support for non-master main branches  #8364 and figure out a different way to land it (although I have no idea how we'd land it without it being a breaking change and still reasonably enabled-by-default).

• You could edit your Cargo.toml files to all omit branch = "master", and then this would work across Cargo versions.

• We could try to slowly roll out Improve support for non-master main branches  #8364 with recognizing older lock files and "changing" the encoding of git repository branches.

I'd personally hope that we could continue to land the change as-is and see if updating y'all's Cargo.toml wouldn't be too bad, but I'm likely biased!

[bkchr]

Could we support loading the old cargo.lock files with no branch=master until stable writes the same Cargo.lock format?
The problem is that our build process involves stable and nightly. The nightly reuses the cargo.lock from the stable build. So we would always run into this until both versions write the same cargo.lock.

In the end all our old releases are "broken". Removing master from the Cargo.toml files could work for the future, but I will first need to see if there is a tool that changes the branch of all workspace projects (which is currently just a find&sed)

[alexcrichton]

If this is an issue for y'all the only real fix is to back everything out of Cargo for now. There's no way to roll out changes to the lock file quickly, so we'd have to slow this change down a lot.

You can work across stable/beta/nightly by omitting branch = 'master', but if you've got frozen release artifacts then it's true that historical ones will not build today with nightly.

[est31]

One could special case branch = "master" as GitReference::MasterBranch for the time being. Then make its lockfile encoding have the legacy behaviour. The commit would still be pinned in the lockfile, just now omitting the branch name can now indicate two things in Cargo.lock: either the branch name is omitted in Cargo.toml as well, or it's set to master. Fundamentally, as commits are pinned, there is no need to encode the branch name in Cargo.lock at all, except for cargo recognizing when you changed the branch name in Cargo.toml and re-downloading stuff automatically, without needing cargo update. So if the proposal is adopted, this would be the only downside, you'd have to run cargo update if you added branch = "master" (or removed such a specification), and master is not the default branch of the repo. But that would be an okay cost to pay to keep the PR inside.

In the long term, one could put version numbers into Cargo.lock and use that to create two lockfile encodings for MasterBranch and DefaultBranch. Such a transition would be what the change would buy time for.

[est31]

See #8475 .

[bkchr]

Nice :)

[eddyb]

I came across this problem before seeing this issue and the explanations, and what I ended up doing is:

sed -i 's/branch = "master"/rev = "e7457b1eb9980596301fe1afd36478a6725157ef"/' ./**/Cargo.toml

That's because we've been collecting performance data (across ~750 Rust PRs by now) for a specific version of Polkadot, and if we allow any versions to change, the performance data wouldn't be as useful (as it's not comparing rustc for the exact same code).

I'll probably leave that hack in, especially with how uniform it is, just because I can trust that going back to any Rust/Cargo version, there's no chance of version updates accidentally succeeding (although I'm pretty sure it's guaranteed to fail, at this point).