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

cargo can silently fix some bad lockfiles (use --locked to disable) #5831

Merged
merged 5 commits into from Aug 1, 2018

Conversation

Projects
None yet
6 participants
@Eh2406
Contributor

Eh2406 commented Jul 30, 2018

Lock files often get corrupted by git merge. This makes all cargo commands silently fix that kind of corruption.

If you want to be sure that your CI does not change the lock file you have commited

Then make sure to use --locked in your CI

Edit: original description below


This is a continuation of @dwijnand work in #5809, and closes #5684

This adds a ignore_errors arg to reading a lock file which ignores sections it doesn't understand. Specifically things that depend on versions that don't exist in the lock file. Then all users pass false except for the two that relate to update command.

I think the open questions for this pr relate to testing.

  • Now that we are passing false in all other commands, do they each need a test for a bad lockfile?
  • Do we need a test with a more subtly corrupted lock file, or is this always sufficient for update to clean up?

dwijnand and others added some commits Jul 26, 2018

@rust-highfive

This comment has been minimized.

Show comment
Hide comment
@rust-highfive

rust-highfive Jul 30, 2018

r? @matklad

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

rust-highfive commented Jul 30, 2018

r? @matklad

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

@dwijnand

An amateur's LGTM, from me.

The only other suggestion I could give is you have such wonderful terse sum type syntax in Rust (https://twitter.com/dwijnand/status/859338631982567424), I'd suggest considering creating and using one instead of using opaque bool. But it might not feel right in the codebase.

[edit: oh and thank you for last mile-ing this!)

@Eh2406

This comment has been minimized.

Show comment
Hide comment
@Eh2406

Eh2406 Jul 30, 2018

Contributor

Good thought! House that look?

Contributor

Eh2406 commented Jul 30, 2018

Good thought! House that look?

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Jul 30, 2018

Member

Thanks! I think actually we may want to unconditionally ignore errors for these sorts of lockfiles. Cargo 99% of the time is about to regenerate a lockfile anyway, so the purpose of the previous lockfile is basically just there to influence the next build as much as possible. If it ends up being corrupt in some subtle ways then it should be fine to always discard as we'd be about to make a new one anyway

Member

alexcrichton commented Jul 30, 2018

Thanks! I think actually we may want to unconditionally ignore errors for these sorts of lockfiles. Cargo 99% of the time is about to regenerate a lockfile anyway, so the purpose of the previous lockfile is basically just there to influence the next build as much as possible. If it ends up being corrupt in some subtle ways then it should be fine to always discard as we'd be about to make a new one anyway

@Eh2406

This comment has been minimized.

Show comment
Hide comment
@Eh2406

Eh2406 Jul 31, 2018

Contributor

I'd like to be careful about updating dependencies without telling the user. Hence update will, but it was going to change the deps anyway, but build won't. If you think I am being too careful, then we can remove the error branch entirely.

Contributor

Eh2406 commented Jul 31, 2018

I'd like to be careful about updating dependencies without telling the user. Hence update will, but it was going to change the deps anyway, but build won't. If you think I am being too careful, then we can remove the error branch entirely.

@dwijnand

This comment has been minimized.

Show comment
Hide comment
@dwijnand

dwijnand Jul 31, 2018

Member

Good thought! House that look?

So great.

Member

dwijnand commented Jul 31, 2018

Good thought! House that look?

So great.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Jul 31, 2018

Member

@Eh2406 oh cargo build will actually update dependencies if you change Cargo.toml (silently currently). That's perhaps a bug and we should fix it, but I do agree that in general we probably shouldn't update much without telling the user!

Member

alexcrichton commented Jul 31, 2018

@Eh2406 oh cargo build will actually update dependencies if you change Cargo.toml (silently currently). That's perhaps a bug and we should fix it, but I do agree that in general we probably shouldn't update much without telling the user!

@Eh2406

This comment has been minimized.

Show comment
Hide comment
@Eh2406

Eh2406 Jul 31, 2018

Contributor

So the error message is removed. I thinking this may lead to odd behavior. Take the motivating example link, If a atomatick merge makes a bad lockfile then CI will pass, by making a new file, but the next PR will see an unexpland change to the lock file.

Contributor

Eh2406 commented Jul 31, 2018

So the error message is removed. I thinking this may lead to odd behavior. Take the motivating example link, If a atomatick merge makes a bad lockfile then CI will pass, by making a new file, but the next PR will see an unexpland change to the lock file.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Aug 1, 2018

Member

@Eh2406 hm do you think this strategy isn't the right one?

If a repository has a lock file checked in it's recommended to build with --locked on CI to ensure that the lock file always reflects reality (to prevent against the problem I think you're mentioning), but do you think that's not enough per se?

Member

alexcrichton commented Aug 1, 2018

@Eh2406 hm do you think this strategy isn't the right one?

If a repository has a lock file checked in it's recommended to build with --locked on CI to ensure that the lock file always reflects reality (to prevent against the problem I think you're mentioning), but do you think that's not enough per se?

@Eh2406

This comment has been minimized.

Show comment
Hide comment
@Eh2406

Eh2406 Aug 1, 2018

Contributor

I haven't heard that before, and the crate that reported the problem isn't using that. We should make sure that CI best practice makes it into some kind of documentation. cc #5656. But that seems like a reasonable solution.

Contributor

Eh2406 commented Aug 1, 2018

I haven't heard that before, and the crate that reported the problem isn't using that. We should make sure that CI best practice makes it into some kind of documentation. cc #5656. But that seems like a reasonable solution.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Aug 1, 2018

Member

For sure yeah we'd definitely want to mention this in our CI best practices documentation!

Are you ok landing this if we do that?

Member

alexcrichton commented Aug 1, 2018

For sure yeah we'd definitely want to mention this in our CI best practices documentation!

Are you ok landing this if we do that?

@Eh2406

This comment has been minimized.

Show comment
Hide comment
@Eh2406

Eh2406 Aug 1, 2018

Contributor

Yes.

I wonder if the transition period (between it lads and when we write best practices) will be painful. But I don't know. I will add it to the OP, and update the title.

Contributor

Eh2406 commented Aug 1, 2018

Yes.

I wonder if the transition period (between it lads and when we write best practices) will be painful. But I don't know. I will add it to the OP, and update the title.

@Eh2406 Eh2406 changed the title from cargo update can deal with some bad lockfiles to cargo can silently fix some bad lockfiles (use --locked to disable) Aug 1, 2018

@Eh2406 Eh2406 added the relnotes label Aug 1, 2018

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Aug 1, 2018

Member

@bors: r+

Ok! We can of course always revert if this causes problems!

Member

alexcrichton commented Aug 1, 2018

@bors: r+

Ok! We can of course always revert if this causes problems!

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Aug 1, 2018

Contributor

📌 Commit a418364 has been approved by alexcrichton

Contributor

bors commented Aug 1, 2018

📌 Commit a418364 has been approved by alexcrichton

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Aug 1, 2018

Contributor

⌛️ Testing commit a418364 with merge 63a08ee...

Contributor

bors commented Aug 1, 2018

⌛️ Testing commit a418364 with merge 63a08ee...

bors added a commit that referenced this pull request Aug 1, 2018

Auto merge of #5831 - Eh2406:i5684, r=alexcrichton
cargo can silently fix some bad lockfiles (use --locked to disable)

Lock files often get corrupted by git merge. This makes all cargo commands silently fix that kind of corruption.

If you want to be sure that your CI does not change the lock file you have commited
---

Then make sure to use `--locked` in your CI

Edit: original description below

---------------

This is a continuation of @dwijnand work in #5809, and closes #5684

This adds a `ignore_errors` arg to reading a lock file which ignores sections it doesn't understand. Specifically things that depend on versions that don't exist in the lock file. Then all users pass false except for the two that relate to `update` command.

I think the open questions for this pr relate to testing.
- Now that we are passing false in all other commands, do they each need a test for a bad lockfile?
- Do we need a test with a more subtly corrupted lock file, or is this always sufficient for `update` to clean up?
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Aug 1, 2018

Contributor

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 63a08ee to master...

Contributor

bors commented Aug 1, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 63a08ee to master...

@bors bors merged commit a418364 into rust-lang:master Aug 1, 2018

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build cancelled
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@Eh2406 Eh2406 deleted the Eh2406:i5684 branch Aug 2, 2018

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Sep 14, 2018

rust: Update to 1.29.0.
Version 1.29.0 (2018-09-13)
==========================

Compiler
--------
- [Bumped minimum LLVM version to 5.0.][51899]
- [Added `powerpc64le-unknown-linux-musl` target.][51619]
- [Added `aarch64-unknown-hermit` and `x86_64-unknown-hermit` targets.][52861]

Libraries
---------
- [`Once::call_once` now no longer requires `Once` to be `'static`.][52239]
- [`BuildHasherDefault` now implements `PartialEq` and `Eq`.][52402]
- [`Box<CStr>`, `Box<OsStr>`, and `Box<Path>` now implement `Clone`.][51912]
- [Implemented `PartialEq<&str>` for `OsString` and `PartialEq<OsString>`
  for `&str`.][51178]
- [`Cell<T>` now allows `T` to be unsized.][50494]
- [`SocketAddr` is now stable on Redox.][52656]

Stabilized APIs
---------------
- [`Arc::downcast`]
- [`Iterator::flatten`]
- [`Rc::downcast`]

Cargo
-----
- [Cargo can silently fix some bad lockfiles ][cargo/5831] You can use
  `--locked` to disable this behaviour.
- [`cargo-install` will now allow you to cross compile an install
  using `--target`][cargo/5614]
- [Added the `cargo-fix` subcommand to automatically move project code from
  2015 edition to 2018.][cargo/5723]

Misc
----
- [`rustdoc` now has the `--cap-lints` option which demotes all lints above
  the specified level to that level.][52354] For example `--cap-lints warn`
  will demote `deny` and `forbid` lints to `warn`.
- [`rustc` and `rustdoc` will now have the exit code of `1` if compilation
  fails, and `101` if there is a panic.][52197]
- [A preview of clippy has been made available through rustup.][51122]
  You can install the preview with `rustup component add clippy-preview`

Compatibility Notes
-------------------
- [`str::{slice_unchecked, slice_unchecked_mut}` are now deprecated.][51807]
  Use `str::get_unchecked(begin..end)` instead.
- [`std::env::home_dir` is now deprecated for its unintuitive behaviour.][51656]
  Consider using the `home_dir` function from
  https://crates.io/crates/dirs instead.
- [`rustc` will no longer silently ignore invalid data in target spec.][52330]

[52861]: rust-lang/rust#52861
[52656]: rust-lang/rust#52656
[52239]: rust-lang/rust#52239
[52330]: rust-lang/rust#52330
[52354]: rust-lang/rust#52354
[52402]: rust-lang/rust#52402
[52103]: rust-lang/rust#52103
[52197]: rust-lang/rust#52197
[51807]: rust-lang/rust#51807
[51899]: rust-lang/rust#51899
[51912]: rust-lang/rust#51912
[51511]: rust-lang/rust#51511
[51619]: rust-lang/rust#51619
[51656]: rust-lang/rust#51656
[51178]: rust-lang/rust#51178
[51122]: rust-lang/rust#51122
[50494]: rust-lang/rust#50494
[cargo/5614]: rust-lang/cargo#5614
[cargo/5723]: rust-lang/cargo#5723
[cargo/5831]: rust-lang/cargo#5831
[`Arc::downcast`]: https://doc.rust-lang.org/std/sync/struct.Arc.html#method.downcast
[`Iterator::flatten`]: https://doc.rust-lang.org/std/iter/trait.Iterator.html#method.flatten
[`Rc::downcast`]: https://doc.rust-lang.org/std/rc/struct.Rc.html#method.downcast
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment