-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Upgrade version of semver used #3124
Comments
Yes, I have planned to, I just have not had the time. It's going to be a big pain, but it's manageable. |
I think I'm going to try to tackle this thursday or friday, depending on how the release goes. |
Ok, thanks @steveklabnik! If it's too onerous we could also just vendor something locally in Cargo like an old regex or something like that. |
That's precisely the issue: the version of semver we're using in Cargo currently has I have a plan (shove the older parser under an |
I think it's probably ok to not literally use the same parser, we can probably just scrape all Cargo.toml files on crates.io and parse them to learn about all "invalid" specs. Would that be helpful to generate such a list? |
Oh yeah, is there an easy way to get all Cargo.toml files? I would love that for my own regression testing. |
My plan was to just download all |
hehe. well, yeah, if we can get that data set, I can maybe make it work more easily. If you can do it, that'd be good. Primarily, it would mean not bringing back the |
Ok, here's the list: https://gist.github.com/alexcrichton/97d3fa76b90e232eb61af37a59da91ad I downloaded every crate file from crates.io, parse all files that looked like The panic there is presumably an older bug in semver, so can be ignored, and word-like ones like "x-smtpapi" I think are false positives (but are odd to be accepted by the old parser!). In short it looks like the complete list of versions we need to keep accepting are:
Those are spread across a whole mess of crates though |
So, on Friday I tried to run the script to reproduce the list, but got bad results. I'm guessing something is wrong. Regardless, looking at those examples:
Given that these are such a small number of cases, I think we can just handle them individually. I've implemented a fix here: https://github.com/steveklabnik/semver/tree/deprecation One commit: dtolnay/semver@aae8a45 This will stop us from having to bring back in the entire old parser. What do you think? @alexcrichton could you try to run your script again against that branch? If so, I will prepare a real PR against semver and Cargo. We can't print out the warning in semver, since you want to respect cargo's settings; the idea is that i'll add a new error case, |
@steveklabnik sounds reasonable to me, but perhaps this behavior could be disabled by default? I doubt anyone actually wants to publish something with |
What would "disabled by default" mean? You mean in the Cargo side of the patch? I'm certainly open to it. |
Oh imagine that Cargo would actively go out of its way to parse these version requirements. General users of the I'd be fine adding this specific logic to Cargo itself as opposed to |
Ah yes. let me prepare the real patch, I think this will work as you want it to. |
Due to issues like rust-lang/cargo#3124, we've found that the old parser was lenient in ways that the new parser isn't. We've analyzed all the crates on crates.io, and found the patterns that weren't working. This commit adds support back for those versions, but marks them as deprecated. Each of them had a different way in which they were incorrect, but resolved to some kind of correct constraint. So the deprecation strategy is this: 1. If a version fails to parse, check if it matches the deprecated pattern. 2. If it does, convert it to the real pattern. 3. Return an error that it's deprecated, and the error will contain the fixed-up version constraint.
upgrade semver This is a spike at fixing #3124. @alexcrichton I'm not sure how to actually emit the error message here. In the TOML example you linked me: https://github.com/rust-lang/cargo/blob/5593045ddef2744c1042dee0c0037c2ebcc1834e/src/cargo/util/toml.rs#L166 That takes a Config as an argument. This function does not. What's the right approach here? Also, this code is messy. I am 99% sure I can write it nicer with combinators. This is just to get the discussion going.
#3154 was just merged. Closing! |
The most recent version of the
semver
repository has fixed a few bugs in version requirements that it rejects. If we update to it, however, it causes breakage, so we had to revert the recent update.Looks like the
semver
crate has fixed bugs recently to support new version requirements with build metadata.I think what we'll need to do is to add a strategy like this where we continue to parse old version requirements, but warn about them.
@steveklabnik, could you add a feature to the
semver
crate to support old version requirements like0.2*
? Once we do that we can update Cargo to using that, warn about old broken requirements, and start picking up bug fixes.The text was updated successfully, but these errors were encountered: