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

Suggest "update -p" to fix a bad lockfile #5809

Closed
wants to merge 2 commits into from

Conversation

dwijnand
Copy link
Member

Resolves #5684

Thanks for the great mentoring instructions, @Eh2406!

I added a test for the --verbose part of issue, and it looked fine? I don't see the "Caused by" message you posted, so I'm a little confused and could use a second pair of eyes, please. :)

r? @Eh2406

@Eh2406
Copy link
Contributor

Eh2406 commented Jul 26, 2018

Thank you for working on this! That looks good.

Just to clarify (#lazyweb) if you have a new project with this lockfile what do you get after this PR for cargo build and for cargo build --verbose

@dwijnand
Copy link
Member Author

dwijnand commented Jul 26, 2018

Looks good:

19:32:05 $ git co https://github.com/rust-lang-nursery/mdBook/pull/625
remote: Counting objects: 50, done.
remote: Total 50 (delta 34), reused 34 (delta 34), pack-reused 16
Unpacking objects: 100% (50/50), done.
From git://github.com/maccoda/mdBook
 * [new branch]          serve_tidy_up -> maccoda/serve_tidy_up
Branch 'serve_tidy_up' set up to track remote branch 'serve_tidy_up' from 'maccoda'.
Switched to a new branch 'serve_tidy_up'

19:32:37 $ git co 5b65ac8e37dc1259083c160defb2ed0927c83c54
Note: checking out '5b65ac8e37dc1259083c160defb2ed0927c83c54'.

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by performing another checkout.

If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -b with the checkout command again. Example:

  git checkout -b <new-branch-name>

HEAD is now at 5b65ac8e3 Merge branch 'master' into serve_tidy_up

19:35:12 $ type dcargo
dcargo is a function
dcargo ()
{
    /d/cargo/target/debug/cargo "$@"
}

19:35:14 $ dcargo build
warning: An explicit [[bin]] section is specified in Cargo.toml which currently
disables Cargo from automatically inferring other binary targets.
This inference behavior will change in the Rust 2018 edition and the following
files will be included as a binary target:

* /d/mdbook/src/bin/serve.rs
* /d/mdbook/src/bin/test.rs
* /d/mdbook/src/bin/clean.rs
* /d/mdbook/src/bin/build.rs
* /d/mdbook/src/bin/watch.rs
* /d/mdbook/src/bin/init.rs

This is likely to break cargo build or cargo test as these files may not be
ready to be compiled as a binary target today. You can future-proof yourself
and disable this warning by adding `autobins = false` to your [package]
section. You may also move the files to a location where Cargo would not
automatically infer them to be a target, such as in subfolders.

For more information on this warning you can consult
https://github.com/rust-lang/cargo/issues/5330
error: failed to parse lock file at: /d/mdbook/Cargo.lock

Caused by:
  package `proc-macro2 0.2.3 (registry+https://github.com/rust-lang/crates.io-index)` is specified as a dependency, but is missing from the package list
  consider running 'cargo update -p structopt-derive'

19:35:22 ! dcargo build --verbose
warning: An explicit [[bin]] section is specified in Cargo.toml which currently
disables Cargo from automatically inferring other binary targets.
This inference behavior will change in the Rust 2018 edition and the following
files will be included as a binary target:

* /d/mdbook/src/bin/serve.rs
* /d/mdbook/src/bin/test.rs
* /d/mdbook/src/bin/clean.rs
* /d/mdbook/src/bin/build.rs
* /d/mdbook/src/bin/watch.rs
* /d/mdbook/src/bin/init.rs

This is likely to break cargo build or cargo test as these files may not be
ready to be compiled as a binary target today. You can future-proof yourself
and disable this warning by adding `autobins = false` to your [package]
section. You may also move the files to a location where Cargo would not
automatically infer them to be a target, such as in subfolders.

For more information on this warning you can consult
https://github.com/rust-lang/cargo/issues/5330
error: failed to parse lock file at: /d/mdbook/Cargo.lock

Caused by:
  package `proc-macro2 0.2.3 (registry+https://github.com/rust-lang/crates.io-index)` is specified as a dependency, but is missing from the package list
  consider running 'cargo update -p structopt-derive'

@dwijnand
Copy link
Member Author

dwijnand commented Jul 26, 2018

That is, assuming, suggesting structopt-derive is right.

@Eh2406
Copy link
Contributor

Eh2406 commented Jul 26, 2018

That is easy to test, try running cargo update -p structopt-derive as suggested :-P

@dwijnand
Copy link
Member Author

Oh dear.

$ dcargo update -p structopt-derive
error: failed to parse lock file at: /d/mdbook/Cargo.lock

Caused by:
  package `proc-macro2 0.2.3 (registry+https://github.com/rust-lang/crates.io-index)` is specified as a dependency, but is missing from the package list
  consider running 'cargo update -p structopt-derive'

@Eh2406
Copy link
Contributor

Eh2406 commented Jul 26, 2018

Drat. So having experimented locally I think the recommendation is correct. cargo update -p structopt-derive would work if update could be run on a corrupted lockfile.

@alexcrichton Is there some way to make update use the parts of the file that make sense? Something like normally we parse the lockfile and if there is an error we bail, but for update we ignore each row that has an error?

@alexcrichton
Copy link
Member

Ah yeah I think we can't fix this by suggesting -p unfortunately. Instead I think what we'll need to do here is to basically make lockfile parsing a bit more lenient. We should in general be able to handle an erroneous lock file as much as we can, Cargo's job is to just preserve as much of it as possible (that it can)

@Eh2406
Copy link
Contributor

Eh2406 commented Jul 27, 2018

I spiked out adding an argument to allow us to ignore bad parts of lock files for update. With some more careful work and lots of tests, this could make update use the lock file, and build/publish report the problem.

@alexcrichton
Copy link
Member

Seems like a good start yeah!

@dwijnand
Copy link
Member Author

Going to drop this off my PR queue, as it looks to require more work than I was interested in committing to it.

@dwijnand dwijnand closed this Jul 30, 2018
@dwijnand dwijnand deleted the bad-lockfile-error-msg branch July 30, 2018 06:20
@Eh2406
Copy link
Contributor

Eh2406 commented Jul 30, 2018

@dwijnand Thank you for working on it. I am very sorry for the miss information in the original report. I will try to flesh out my spiked, to get your work merged.

@dwijnand
Copy link
Member Author

Not a problem, estimates are hard. 😄 (learnt some more Cargo in the process)

bors added a commit that referenced this pull request Aug 1, 2018
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?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants