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.

Sign up for GitHub

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

Jump to bottom

when reading a bad lock file give better error messages #5684

Closed
Eh2406 opened this issue Jul 5, 2018 · 9 comments · Fixed by #5831
Closed

when reading a bad lock file give better error messages #5684

Eh2406 opened this issue Jul 5, 2018 · 9 comments · Fixed by #5831
Labels
A-diagnostics Area: Error and warning messages generated by Cargo itself. E-easy Experience: Easy

Comments

@Eh2406
Copy link
Contributor

Eh2406 commented Jul 5, 2018

When we read a lock file we make sure it makes sense and refuse to parse the file if it doesn't. This is the correct behavior but leads to not grate error messages. For example this merge conflickt.

We should find some way to give better error messages, when the file is a valid Toml but does not make sense.
@alexcrichton
Copy link
Member

Agreed! Ideally Cargo would just deal with merge markers in Cargo.lock, and the suggestion on that thread of "start from master and then re-resolve" is basically what Cargo would do and seems like a great strategy to me!

@Eh2406
Copy link
Contributor Author

Eh2406 commented Jul 5, 2018

I was thinking something more modest. The lock file (like this one) that is in a inconsistent state (however it got that way) should lead good error message. As the existing is pretty bad.

C:\test> cargo build
error: failed to parse lock file at: C:\test\Cargo.lock

To learn more, run the command again with --verbose.

the --verbose is slightly better,

C:\test> cargo build  --verbose
error: failed to parse lock file at: C:\test\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

But it would be much better if it suggested cargo update -p tempfile

I think actually doing a merge would be a much bigger problem.

@alexcrichton
Copy link
Member

Hm true! In any case it's hard to imagine a worse situation than we're in today with our current error messages :)

@alexcrichton alexcrichton added the A-diagnostics Area: Error and warning messages generated by Cargo itself. label Jul 5, 2018
@Eh2406
Copy link
Contributor Author

Eh2406 commented Jul 5, 2018 •
edited
Loading

Looks like this error is generated at:

cargo/src/cargo/core/resolver/encode.rs

Lines 77 to 92 in f4fd347

let lookup_id = |enc_id: &EncodablePackageId| -> CargoResult<Option<PackageId>> {
match live_pkgs.get(enc_id) {
Some(&(ref id, _)) => Ok(Some(id.clone())),
None => if all_pkgs.contains(enc_id) {
// Package is found in the lockfile, but it is
// no longer a member of the workspace.
Ok(None)
} else {
bail!(
"package `{}` is specified as a dependency, \
but is missing from the package list",
enc_id
);
},
}
};

That closure is only use 3 times and one of the times we ignore the error. The other 2 are:

cargo/src/cargo/core/resolver/encode.rs

Line 108 in f4fd347

if let Some(to_depend_on) = lookup_id(edge)? {

and

cargo/src/cargo/core/resolver/encode.rs

Line 121 in f4fd347

if let Some(replace_id) = lookup_id(replace)? {

In both cases we have an id, which is a PackageId of the thing that depends on the non existent package. So we can pass a second arg to the closer of id.name and add a "\n consider running 'cargo update -p {id}'" to the bail! call.

Then update the tests at:

cargo/tests/testsuite/bad_config.rs

Line 462 in f4fd347

package `bar 0.1.0 ([..])` is specified as a dependency, but is missing from the package list

If you could describe how to make these into non--verbose errors, then it should not be to difficult. Maybe eavan E-easy?

@alexcrichton
Copy link
Member

Sure! I'll hold off on E-easy until there's instructions written up here, but if you're willing to do that I can certainly do the tagging!

@Eh2406
Copy link
Contributor Author

Eh2406 commented Jul 5, 2018

Edited to be more instruction like. If you have advice on how to make them better LMK. I don't know how to convert a --verbose error to a normal error output, so I can't write the instructions on it.

@alexcrichton alexcrichton added the E-easy Experience: Easy label Jul 5, 2018
@alexcrichton
Copy link
Member

Sounds good to me!

@Eh2406
Copy link
Contributor Author

Eh2406 commented Jul 5, 2018

Sounds good to me!

I was hoping you would write instructions for the part I don't know. ☹️

@alexcrichton
Copy link
Member

Oh sorry! Typically making something non-verbose entails tracking down the call to internal that happens along the way and remove it, but I'm not sure offhand where that's happening. If there's a PR for the first pieces I can help track it down!

@dwijnand dwijnand mentioned this issue Jul 26, 2018
Closed
@Eh2406 Eh2406 mentioned this issue Jul 30, 2018
Merged
bors added a commit that referenced this issue Aug 1, 2018
@bors
Auto merge of #5831 - Eh2406:i5684, r=alexcrichton
63a08ee 
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 bors closed this as completed in #5831 Aug 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-diagnostics Area: Error and warning messages generated by Cargo itself. E-easy Experience: Easy
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

cargo can silently fix some bad lockfiles (use --locked to disable) Eh2406/cargo
2 participants
@alexcrichton @Eh2406