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

Use to Cargo's experimental lockfile format #63579

Merged
merged 1 commit into from
Aug 19, 2019

Conversation

alexcrichton
Copy link
Member

This commit changes the lock file format of this repository to an
experimental format that isn't rolled out by default in Cargo but is
intended to eventually become the default. The new format moves
information around and compresses the lock file a bit. The intention of
the new format is to reduce the amount of git merge conflicts that
happen in a repository, with rust-lang/rust being a prime candidate for
testing this.

The new format wille ventually become the default but for now it is
off-by-default in Cargo, but Cargo will preserve the format if it sees
it. Since we always build with a beta version of Cargo for the
rust-lang/rust repository it should be safe to go ahead and change the
lock file format here and everyone building this repository will
automatically pick it up.

It's intended that we'll evaluate this lock file format in the
rust-lang/rust repository to see if it reduces the number of perceived
merge conflicts for changes that touch the lock file. This will in turn
help inform the development of the feature in Cargo and whether we
choose to stabilize this and turn it on by default.

Note that this commit does not actually change the contents of the lock
file in terms of a resolution graph, it simply reencodes the lock file
with a new format.

@rust-highfive
Copy link
Collaborator

r? @nikomatsakis

(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 Aug 15, 2019
@alexcrichton
Copy link
Member Author

cc @ehuss

r? @Mark-Simulacrum

@ehuss
Copy link
Contributor

ehuss commented Aug 15, 2019

to see if it reduces the number of perceived merge conflicts for changes that touch the lock file

Do you have any ideas on how to measure this? I'm concerned most people, when they hit a conflict or build error, will just silently fix it and we won't know about it. Like when there are spurious test errors, and people just bors retry without leaving a note or telling the relevant parties.

@Mark-Simulacrum
Copy link
Member

This looks good from an implementation standpoint to me. Cc @Centril for roll-up awareness.

I personally am not too worried about gathering statistics, if necessary I think we can get at least some data from bors merge conflict comments when they're in the expanded from (usually but not always the case? I feel like I've seen both recently). It's also something that'll just naturally pervolate, historically I think I've hit a Cargo.lock conflict quite a few times and presumably that'll go down with this change.

@ehuss
Copy link
Contributor

ehuss commented Aug 15, 2019

historically I think I've hit a Cargo.lock conflict quite a few times and presumably that'll go down with this change

Hopefully it will almost never happen. I did tests with historical conflicts and I was only able to cause problems simulating when two PRs try to update the same dependencies, which should be extremely rare. The concern is when it doesn't cause a conflict, but then either the build fails or --locked fails. Perhaps after a few months we can scan RLA logs (or get logs from azure) to see how often it happens? I expect it to be very rare, but I think it would good to confirm that assumption.

Is anything archiving azure logs (particularly for PR builds)? The retention isn't clear.

@Centril
Copy link
Contributor

Centril commented Aug 15, 2019

Cc @Centril for roll-up awareness.

👋

@alexcrichton
Copy link
Member Author

It's true yeah that I don't really know how we can rigorously and quantitatively measure the impact of this change. I was personally hoping moreso for a "gut feeling" from developers and folks who frob Cargo.lock a lot. (for example rollup managers, @ehuss for updating Cargo, etc)

@Mark-Simulacrum
Copy link
Member

I personally think that's the way to go and we can probably go ahead and merge (r=me).

@alexcrichton
Copy link
Member Author

@bors: r=Mark-Simulacrum

@bors
Copy link
Contributor

bors commented Aug 16, 2019

📌 Commit e4a12206e801bf60f370f367478b03986062b6fe has been approved by Mark-Simulacrum

@bors 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 Aug 16, 2019
@Centril
Copy link
Contributor

Centril commented Aug 16, 2019

@bors p=3

@bors
Copy link
Contributor

bors commented Aug 16, 2019

🔒 Merge conflict

This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again.

How do I rebase?

Assuming self is your fork and upstream is this repository, you can resolve the conflict following these steps:

  1. git checkout new-lockfile (switch to your branch)
  2. git fetch upstream master (retrieve the latest master)
  3. git rebase upstream/master -p (rebase on top of it)
  4. Follow the on-screen instruction to resolve conflicts (check git status if you got lost).
  5. git push self new-lockfile --force-with-lease (update this PR)

You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial.

Please avoid the "Resolve conflicts" button on GitHub. It uses git merge instead of git rebase which makes the PR commit history more difficult to read.

Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Cargo.lock conflict is handled during merge and rebase. This is normal, and you should still perform step 5 to update this PR.

Error message
Auto-merging Cargo.lock
CONFLICT (content): Merge conflict in Cargo.lock
Automatic merge failed; fix conflicts and then commit the result.

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 16, 2019
@bors
Copy link
Contributor

bors commented Aug 16, 2019

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

@alexcrichton
Copy link
Member Author

@bors: r=Mark-Simulacrum

@bors
Copy link
Contributor

bors commented Aug 19, 2019

📌 Commit 4cadf879ab221ea78d25501c65b6c1742fb6e794 has been approved by Mark-Simulacrum

@bors 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 19, 2019
@bors
Copy link
Contributor

bors commented Aug 19, 2019

🔒 Merge conflict

This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again.

How do I rebase?

Assuming self is your fork and upstream is this repository, you can resolve the conflict following these steps:

  1. git checkout new-lockfile (switch to your branch)
  2. git fetch upstream master (retrieve the latest master)
  3. git rebase upstream/master -p (rebase on top of it)
  4. Follow the on-screen instruction to resolve conflicts (check git status if you got lost).
  5. git push self new-lockfile --force-with-lease (update this PR)

You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial.

Please avoid the "Resolve conflicts" button on GitHub. It uses git merge instead of git rebase which makes the PR commit history more difficult to read.

Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Cargo.lock conflict is handled during merge and rebase. This is normal, and you should still perform step 5 to update this PR.

Error message
warning: Cannot merge binary files: Cargo.lock (HEAD vs. heads/homu-tmp)
Auto-merging Cargo.lock
CONFLICT (content): Merge conflict in Cargo.lock
Automatic merge failed; fix conflicts and then commit the result.

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 19, 2019
This commit changes the lock file format of this repository to an
experimental format that isn't rolled out by default in Cargo but is
intended to eventually become the default. The new format moves
information around and compresses the lock file a bit. The intention of
the new format is to reduce the amount of git merge conflicts that
happen in a repository, with rust-lang/rust being a prime candidate for
testing this.

The new format wille ventually become the default but for now it is
off-by-default in Cargo, but Cargo will preserve the format if it sees
it. Since we always build with a beta version of Cargo for the
rust-lang/rust repository it should be safe to go ahead and change the
lock file format here and everyone building this repository will
automatically pick it up.

It's intended that we'll evaluate this lock file format in the
rust-lang/rust repository to see if it reduces the number of perceived
merge conflicts for changes that touch the lock file. This will in turn
help inform the development of the feature in Cargo and whether we
choose to stabilize this and turn it on by default.

Note that this commit does not actually change the contents of the lock
file in terms of a resolution graph, it simply reencodes the lock file
with a new format.
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Aug 19, 2019
@bors
Copy link
Contributor

bors commented Aug 19, 2019

⌛ Testing commit 093ede2 with merge 29a5403...

bors added a commit that referenced this pull request Aug 19, 2019
Use to Cargo's experimental lockfile format

This commit changes the lock file format of this repository to an
experimental format that isn't rolled out by default in Cargo but is
intended to eventually become the default. The new format moves
information around and compresses the lock file a bit. The intention of
the new format is to reduce the amount of git merge conflicts that
happen in a repository, with rust-lang/rust being a prime candidate for
testing this.

The new format wille ventually become the default but for now it is
off-by-default in Cargo, but Cargo will preserve the format if it sees
it. Since we always build with a beta version of Cargo for the
rust-lang/rust repository it should be safe to go ahead and change the
lock file format here and everyone building this repository will
automatically pick it up.

It's intended that we'll evaluate this lock file format in the
rust-lang/rust repository to see if it reduces the number of perceived
merge conflicts for changes that touch the lock file. This will in turn
help inform the development of the feature in Cargo and whether we
choose to stabilize this and turn it on by default.

Note that this commit does not actually change the contents of the lock
file in terms of a resolution graph, it simply reencodes the lock file
with a new format.
@bors
Copy link
Contributor

bors commented Aug 19, 2019

☀️ Test successful - checks-azure
Approved by: Mark-Simulacrum
Pushing 29a5403 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 19, 2019
@bors bors merged commit 093ede2 into rust-lang:master Aug 19, 2019
alexcrichton added a commit to alexcrichton/rust that referenced this pull request Aug 20, 2019
This commit backs out rust-lang#46539 in order to fully leverage rust-lang#63579 where
`git` should be able to merge `Cargo.lock` nowadays with only minimal
conflicts.
Centril added a commit to Centril/rust that referenced this pull request Aug 20, 2019
…rk-Simulacrum

Allow git to merge `Cargo.lock`

This commit backs out rust-lang#46539 in order to fully leverage rust-lang#63579 where
`git` should be able to merge `Cargo.lock` nowadays with only minimal
conflicts.
@alexcrichton alexcrichton deleted the new-lockfile branch August 24, 2019 22:09
@RalfJung
Copy link
Member

Looks like since this PR, stable cargo cannot parse our Cargo.toml any more, which breaks tools like cargo tree. Is there a known work-around for that?

@alexcrichton
Copy link
Member Author

Tools like cargo tree will need to update their bundled version of Cargo.

@RalfJung
Copy link
Member

The cargo on crates.io tracks stable. So, such tools can only do the update once there is a stable cargo that has that feature. Which, for the new lockfile format, there isn't.

nox added a commit to servo/servo that referenced this pull request Oct 1, 2019
The way the Cargo.lock file is encoded on stable Rust is quite unfriendly
to Git when multiple pull requests change that file at the same time,
how version numbers are everywhere and all checksums are in the same place
often produces merge conflicts for nothing.

This patch brings the new Cargo.lock format to Servo.

	rust-lang/rust#63579

The main caveat is that for now, cargo-tree and similar tools won't work
anymore.

I checked that the duplicate crate tidy check still does its job though.
nox added a commit to servo/servo that referenced this pull request Oct 4, 2019
The way the Cargo.lock file is encoded on stable Rust is quite unfriendly
to Git when multiple pull requests change that file at the same time,
how version numbers are everywhere and all checksums are in the same place
often produces merge conflicts for nothing.

This patch brings the new Cargo.lock format to Servo.

	rust-lang/rust#63579

The main caveat is that for now, cargo-tree and similar tools won't work
anymore.

I checked that the duplicate crate tidy check still does its job though.
nox added a commit to servo/servo that referenced this pull request Oct 4, 2019
The way the Cargo.lock file is encoded on stable Rust is quite unfriendly
to Git when multiple pull requests change that file at the same time,
how version numbers are everywhere and all checksums are in the same place
often produces merge conflicts for nothing.

This patch brings the new Cargo.lock format to Servo.

	rust-lang/rust#63579

The main caveat is that for now, cargo-tree and similar tools won't work
anymore.

I checked that the duplicate crate tidy check still does its job though.
@nox
Copy link
Contributor

nox commented Oct 4, 2019

I made a very small tool to go back to the previous format, just in case.

bors-servo pushed a commit to servo/servo that referenced this pull request Oct 4, 2019
Switch to the new Cargo.lock format

The way the Cargo.lock file is encoded on stable Rust is quite unfriendly
to Git when multiple pull requests change that file at the same time,
how version numbers are everywhere and all checksums are in the same place
often produces merge conflicts for nothing.

This patch brings the new Cargo.lock format to Servo.

rust-lang/rust#63579

The main caveat is that for now, cargo-tree and similar tools won't work
anymore.

I checked that the duplicate crate tidy check still does its job though.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/24334)
<!-- Reviewable:end -->
bors-servo pushed a commit to servo/servo that referenced this pull request Oct 4, 2019
Switch to the new Cargo.lock format

The way the Cargo.lock file is encoded on stable Rust is quite unfriendly
to Git when multiple pull requests change that file at the same time,
how version numbers are everywhere and all checksums are in the same place
often produces merge conflicts for nothing.

This patch brings the new Cargo.lock format to Servo.

rust-lang/rust#63579

The main caveat is that for now, cargo-tree and similar tools won't work
anymore.

I checked that the duplicate crate tidy check still does its job though.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/24334)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants