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

Panic due to "0.*.*" wildcard version number in Cargo.toml #3202

Closed
CensoredUsername opened this issue Oct 16, 2016 · 18 comments · Fixed by #3207
Closed

Panic due to "0.*.*" wildcard version number in Cargo.toml #3202

CensoredUsername opened this issue Oct 16, 2016 · 18 comments · Fixed by #3207

Comments

@CensoredUsername
Copy link

CensoredUsername commented Oct 16, 2016

On recent nightlies, my cargo started panicing when trying to do any operation that involved the registry. The panic message was as following:

thread 'main' panicked at 'called `Option::unwrap()` on a `None` value', ../src/libcore\option.rs:317

The panic message disappeared after changing a version number in Cargo.toml from "0.." to "0.0.". It was confirmed by another person that changing a version number to "0..*" caused his cargo to panic as well.

@steveklabnik
Copy link
Member

These are definitely invalid versions, but cargo should certainly do better than panic!

On Oct 16, 2016, 16:43 -0400, CensoredUsername notifications@github.com, wrote:

On recent nightlies, my cargo started panicing when trying to do any operation that involved the registry. The panic message was as following:

thread 'main' panicked at 'called Option::unwrap() on a None value', ../src/libcore\option.rs:317

The panic message disappeared after changing a version number in Cargo.toml from "0.." to "0.0.". It was confirmed by another person that changing a version number to "0..*" caused his cargo to panic as well.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub (#3202), or mute the thread (https://github.com/notifications/unsubscribe-auth/AABsilx9aJaNSNimDzP4m54sD-A3SBBQks5q0ox8gaJpZM4KYFAm).

@CensoredUsername
Copy link
Author

Whoops, looks like markdown accidentally ate my version wildcards. The problematic version number was "0..".

@alexcrichton
Copy link
Member

@steveklabnik it looks like this may be a semver bug? The stack trace looks like:

   1:        0x10898181a - std::sys::backtrace::tracing::imp::write::h46f28e67d38b4637
   2:        0x108988f1f - std::panicking::default_hook::{{closure}}::h1d3243f546573ff4
   3:        0x108988325 - std::panicking::default_hook::h96c288d728df3ebf
   4:        0x108988936 - std::panicking::rust_panic_with_hook::hb1322e5f2588b4db
   5:        0x1089887d4 - std::panicking::begin_panic::hfbeda5aad583dc32
   6:        0x1089886f2 - std::panicking::begin_panic_fmt::h4fe9fb9d5109c4bf
   7:        0x108988657 - rust_begin_unwind
   8:        0x1089b5b20 - core::panicking::panic_fmt::h4395919ece15c671
   9:        0x1089b5a24 - core::panicking::panic::hc74ff52ed78364e1
  10:        0x1087fe1b9 - <semver::version_req::Predicate as core::fmt::Display>::fmt::hf7732dff8fc4e82a

That is, this looks like it's in Display for Predicate?

@steveklabnik
Copy link
Member

Ah interesting... it does look like that.

I will investigate tomorrow.

@steveklabnik
Copy link
Member

steveklabnik commented Oct 17, 2016

Can confirm, something is amiss in Display for Predicate.

Wildcard(Patch) => try!(write!(fmt, "{}.{}.*", self.major, self.minor.unwrap())),

Here, if we have a wildcard on the patch version, we assume that we have a number for the minor version. But because this requirement is parsed incorrectly, it does not.

@alexcrichton , should I fold these kinds of versions back into our new "parse but indicate that they're deprecated" list? I think that's the approach I'd prefer here.

@steveklabnik
Copy link
Member

steveklabnik commented Oct 17, 2016

Actually, it looks like these versions should be okay. Nevermind. npm does parse them correctly. (I was recalling the weirdness around 2*.

steveklabnik added a commit to dtolnay/semver that referenced this issue Oct 17, 2016
We improperly assumed that if we had a wildcard patch version, we must
have a non-wildcard minor version. This is actually incorrect.
@steveklabnik
Copy link
Member

dtolnay/semver#96

@alexcrichton
Copy link
Member

Thanks for the fix @steveklabnik!

@steveklabnik
Copy link
Member

@CensoredUsername for my own knowledge... is this an older project that just started to panic? Or something new where you added it.

Given that, while the version parser has changed a lot, this code didn't change at all, I'm interested in if this is a regression rather than something new.

@steveklabnik
Copy link
Member

(Doing some of my own testing, it seems like this was in v0.2.3, the previous version Cargo supported, and v0.2.0, which may be the oldest semver that works on today's Rust. (semver is much older than Rust 1.0)

So this leads me to believe that this isn't a regression, was just always a bug in semver.

@steveklabnik
Copy link
Member

Okay, this is very strange now. While that semver version doesn't work, using cargo build with current stable Rust's Cargo (cargo 0.13.0-nightly (109cb7c 2016-08-19)) does turn semver = "0.*.*" into 0.5.0.

@alexcrichton , does/did Cargo do any pre-processing to semver versions like this? I have a hunch that this was never passed through to semver in the first place, and it'd be nice to confirm why this bug was happening before we put a band-aid on it, only to find out later it wasn't the right one.

@alexcrichton
Copy link
Member

That may be the case, although I thought we parsed everything...

Perhaps this was added "accidentally" in the update semver PR?

@steveklabnik
Copy link
Member

Some gdb logs for https://github.com/steveklabnik/cargo/blob/464e6a96fb599049761a8842c89118bde1683edd/src/cargo/core/dependency.rs#L111 when trying to parse a dependency of 0.*.*:

'stable' cargo:

3: version_req = VersionReq = {predicates = Vec<semver::version_req::Predicate>(len: 1, cap: 1) = {Predicate = {op = Wildcard = {
        Patch}, major = 0, minor = None, patch = None, pre = Vec<semver::version::Identifier>(len: 0, cap: 0)}}}

nightly cargo:

2: version_req = VersionReq = {predicates = Vec<semver::version_req::Predicate>(len: 1, cap: 1) = {Predicate = {op = Wildcard = {
        Patch}, major = 0, minor = None, patch = None, pre = Vec<semver::version::Identifier>(len: 0, cap: 0)}}}

It looks like they produce the exact same VersionReq structure, so I am very confused.

@steveklabnik
Copy link
Member

And from within semver, using panic in a test to print

old

    thread 'version_req::test::test_cargo3202' panicked at 'Predicate { op: Wildcard(Patch), major: 0, minor: None, patch: None, pre: [] }', src/version_req.rs:785

new

    thread 'version_req::test::test_cargo3202' panicked at 'Predicate { op: Wildcard(Patch), major: 0, minor: None, patch: None, pre: [] }', src/version_req.rs:815

@steveklabnik
Copy link
Member

I figured it out!

Display on Predicate was never called when doing a cargo build on v0.2.3. It is on v0.5.0. I verified this by putting a breakpoint on the call, and running old cargo with it, it ran to completion. With new cargo, it hit the breakpoint.

I'll merge that fix in and cut a new release, sorry about the spam here!

steveklabnik added a commit to dtolnay/semver that referenced this issue Oct 17, 2016
We improperly assumed that if we had a wildcard patch version, we must
have a non-wildcard minor version. This is actually incorrect.

This bug has been present in semver since at least the 0.2 series, but
`cargo build` never happend to invoke Display for Predicate. The bug
manifested in newer Cargos because in 0.5, we do call Display for
Predicate.
@CensoredUsername
Copy link
Author

@steveklabnik Just for the record, it was on a project I was actively working on, and the last time I ran rustup update should only have been a few weeks ago. I didn't change the version numbers in between.

@steveklabnik
Copy link
Member

@CensoredUsername thanks!

Unfortunately fixing this bug has exposed another. I am trying to track it down, but will stop spamming this issue, as the root cause has been found, at least from Cargo's perspective.

@CensoredUsername
Copy link
Author

@steveklabnik no problem, and good luck chasing down the bugs.

steveklabnik added a commit to dtolnay/semver that referenced this issue Oct 17, 2016
steveklabnik added a commit to steveklabnik/cargo that referenced this issue Oct 17, 2016
bors added a commit that referenced this issue Oct 17, 2016
bump semver to 0.5.1

Fixes #3202

I've verified locally that this works, and have a test for it within semver. Two, actually.

@CensoredUsername if you could give this a shot to double check that it fixes your exact bug, that'd be cool.
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 a pull request may close this issue.

3 participants