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

Error message on previously nonexistent crate is a bit unhelpful #481

Open
nmathewson opened this issue Jul 2, 2023 · 5 comments
Open
Labels
C-bug Category: doesn't meet expectations E-help-wanted Call for participation: Help is requested to fix this issue. E-mentor Call for participation: Mentorship is available for this issue.

Comments

@nmathewson
Copy link
Contributor

Steps to reproduce the bug with the above code

Run cargo-semver-checks on a workspace in which at least one crate has been created since the previous revision.

Actual Behaviour

At the end of its output, you get an error message like this:

Error: possibly due to errors: [
  failed to parse /home/nickm/src/arti/target/semver-checks/git-arti_v1_1_5_reconstructed/9e415240a0616f26cc766dabcad14f2f66b1c472/Cargo.toml: no `package` table,
]

Caused by:
    package `tor-geoip` not found in /home/nickm/src/arti/target/semver-checks/git-arti_v1_1_5_reconstructed/9e415240a0616f26cc766dabcad14f2f66b1c472

This was confusing to me for several reasons:

  1. At first, I though that this was a failure in the cargo-semver-checks process, and that I had to add --exclude tor-geoip in order to get valid output for my other crates.
  2. I thought that
  3. It looked like an internal error.

Expected Behaviour

If this message has to appear at the end of the output, I would prefer a message more along the lines of:

Warning: could not find the following crates in the baseline revision.  
    (This is not a semver problem if the crates did not previously exist.)
    *  `tor-geoip`
    * `tor-keymgr`

If the message can reasonably occur in the middle of the output, I'd prefer something like:

     Parsing tor-geoip v0.1.0 (current)
     Could not find tor-geoip in baseline!
    Cannot check tor-geoip ??? -> 0.1.0: Baseline not found.

Generated System Information

[1082]$ cargo semver-checks --bugreport
#### Software version

cargo-semver-checks 0.22.1

#### Operating system

Linux 6.3.8-200.fc38.x86_64

#### Command-line

```bash
/home/nickm/.cargo/bin/cargo-semver-checks semver-checks --bugreport 

cargo version

> cargo -V 
cargo 1.70.0 (ec8a8a0ca 2023-04-25)

Compile time information

  • Profile: release
  • Target triple: x86_64-unknown-linux-gnu
  • Family: unix
  • OS: linux
  • Architecture: x86_64
  • Pointer width: 64
  • Endian: little
  • CPU features: fxsr,sse,sse2
  • Host: x86_64-unknown-linux-gnu

### Build Configuration

_No response_

### Additional Context

I'm happy to try hacking this myself if you can point me to the right parts of the code to look at, but I might be a little slow.
@obi1kenobi
Copy link
Owner

Completely agreed that this error message leaves a lot to be desired. Thank you for flagging it and offering to look into it!

There are definitely a few similar edge cases here:

  • newly-created lib crate has no previous version on crates.io
  • existing crate newly gains a lib target (previously only a bin crate), so the prior version on crates.io has no lib target
  • all prior versions of the crate on crates.io have been yanked, so the baseline rustdoc generation fails since it can't generate a lockfile

For each of these, we need (probably in this order):

  • a test case
  • suitable error handling that detects that these edge cases happened
  • a way to propagate this error in the library API of this crate (probably in the CrateReport type)
  • a good error message when that error happens via the CLI

As a separate issue, for --workspace runs we may perhaps want to write a summary of which crates got checked (w/ outcomes) and which got skipped for which reasons (explicitly excluded, no lib target, no valid baseline for reason XYZ, etc.). This summary could perhaps be generated from the Report type.

Feel free to tackle as few or as many of these as you'd like. Just please keep PRs small for fast and easy reviews and ask early and often if you feel stuck on anything :)

Pointers to code

For the tests, I'd imagine we'd like to either add them in a file like the existing rustdoc edge cases test, or as a separate GitHub Actions job like the many we already have: https://github.com/obi1kenobi/cargo-semver-checks/blob/main/.github/workflows/ci.yml

For the "bin crate newly gained a lib target" test case, you can use cargo-semver-checks itself: it first gained a lib target in v0.18.2.

For the code that checks crates.io for prior versions to pick as a baseline, you can find it here. You may need to change the API to return something better than anyhow::Result<PathBuf> in order to fully communicate the variety of ways something might have gone wrong.

@weihanglo
Copy link

Cargo hit this issue when importing rustfix into rust-lang/cargo: rust-lang/cargo#13005 (comment).

I don't think we want a fix here, but indeed the error message could be better.
I am also glad that it failed then silent succeeded :)

@obi1kenobi
Copy link
Owner

@nmathewson any chance you're still interested in taking a look at resolving this issue? Happy to mentor if so!

@obi1kenobi obi1kenobi added E-help-wanted Call for participation: Help is requested to fix this issue. E-mentor Call for participation: Mentorship is available for this issue. labels Nov 20, 2023
@nmathewson
Copy link
Contributor Author

@nmathewson any chance you're still interested in taking a look at resolving this issue? Happy to mentor if so!

Hi! Theoretically someday, but probably not soon... if anybody else wants to pick this up, they should feel free.

@leighmcculloch
Copy link

I don't think we want a fix here

When explicitly checking a crate I agree a better error message rather than a fix might be better. But for workspaces when adding a new crate it's inconvenient that it'll cause semver-checks to fail for the first time until the crate is published. It may be more appropriate to be a warning.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: doesn't meet expectations E-help-wanted Call for participation: Help is requested to fix this issue. E-mentor Call for participation: Mentorship is available for this issue.
Projects
None yet
Development

No branches or pull requests

4 participants