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.

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

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

Closed
wants to merge 1 commit into from

Conversation

est31
Copy link
Member

@est31 est31 commented Jul 10, 2020

Fixes #8468 which was a regression of #8364 .

@rust-highfive
Copy link

r? @Eh2406

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 10, 2020
@alexcrichton
Copy link
Member

I think there's a number of issues with this approach, for example this makes it pretty uncomfortable in Cargo to figure out what to do when you have dependencies on the default branch and the master branch. More pressing, though, is that if you add a dependency:

libc = { git = 'https://github.com/rust-lang/libc', branch = 'master' }

then cargo fetch will always update the git repository even though there's a Cargo.lock. This PR doesn't implement the logic to connect the URL in the lock file with the source during resolution on the next execution of cargo.

@est31
Copy link
Member Author

est31 commented Jul 11, 2020

figure out what to do when you have dependencies on the default branch and the master branch

What happens when you have dependencies on two different crates at different paths but named the same way? This would be the same situation. The code I'm changing is the same code that's stripping the path away so that it doesn't appear in Cargo.lock. Furthermore, the URL still includes the commit hash, so it still differs when there is any difference between the branches.

cargo fetch will always update the git repository even though there's a Cargo.lock

Hmm that's bad. Will have a look.

@est31
Copy link
Member Author

est31 commented Jul 11, 2020

What happens when you have dependencies on two different crates at different paths but named the same way? This would be the same situation.

So I tested it out and apparently it's an issue. This happens:

error: package collision in the lockfile: packages foo v0.1.0 (/path/1) and foo v0.1.0 (/path/2) are different, but only one can be written to lockfile unambiguously

Same error happens when you have branch = "master" once and default branch the other time:

error: package collision in the lockfile: packages byteorder v1.3.4 (https://github.com/BurntSushi/byteorder?branch=master#c0143a7a) and byteorder v1.3.4 (https://github.com/BurntSushi/byteorder#c0143a7a) are different, but only one can be written to lockfile unambiguously

In other words, this PR is fixing a bug where Cargo.lock is silently overwritten by cargo and introduces a build failure breakage where cargo can't compile something any more that used to compile just fine... a much worse breakage.

So that's another issue the PR would have to address, and ultimately, it adds more and more complexity. In particular, it stops being a quick fix to buy time for a proper solution, e.g. version numbers. Time on that is better spent :).

@est31 est31 closed this Jul 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cargo build updates the git commit
4 participants