-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Update to semver 1.0.0 #9508
Update to semver 1.0.0 #9508
Conversation
r? @ehuss (rust-highfive has picked a reviewer for you, use r? to override) |
src/cargo/core/dependency.rs
Outdated
".*" => "*", | ||
"0.1.0." => "0.1.0", | ||
"0.3.1.3" => "0.3.13", | ||
"0.2*" => "0.2.*", | ||
"*.0" => "*", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are copied from dtolnay/semver#93.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for keeping the compat here! That being said the warning below says that it will soon become a hard error, and that was added 5 years ago in #3154. I think it's probably fine to just delete all this at this point (and I'm glad you moved this logic to Cargo from the semver
crate itself!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call 😛 I've deleted the compatibility code in 396bdd3.
src/cargo/core/dependency.rs
Outdated
Some(v) => (true, parse_req_with_deprecated(name, v, arg)?), | ||
None => (false, VersionReq::any()), | ||
Some(v) => (true, parse_req_with_deprecated(name, v, arg)?.into()), | ||
None => (false, OptVersionReq::Any), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Any" is not considered a VersionReq, because there is no semver syntax that literally matches any version. *
is not that, because of how https://github.com/semver/semver/blob/efcff2c838c9945f79bfd21c1df0073271bcc29c/ranges.md suggests handling pre-release versions:
SemVer Versions containing PRERELEASE identifiers MUST NOT be included by a SemVer Comparator Set unless they are included by all of the Comparators in the Comparator Set, and one or more of the following conditions are met:
The implementation has provided the user with an option to explicitly include PRERELEASE versions, and the user has set this option. This SHOULD NOT be the default behavior when resolving software dependencies.
One or more of the Comparators in the Comparator Set have the same MAJOR, MINOR, and PATCH version as the SemVer Version string being considered, and has a PRERELEASE version, and would normally include the SemVer Version string by virtue of precedence comparison.
For example, the Range
>=1.0.0-alpha
would include the Version1.0.0-beta
but not the Version1.0.1-beta
.
In previous versions of the semver crate, VersionReq::any()
would literally mean any, which is different from *
, and it would match any version. But upon roundtrip through a string, it would decay to *
, because again there is no semver syntax that means "any".
Instead I'm suggesting that Cargo use something effectively like Option<VersionReq>
with an absent VersionReq meaning "any", and the equivalent of .map_or(true, |req| req.matches(version))
for matching.
Thank you for working on semver! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this @dtolnay! This all looks good to me and I would personally be ok merging. We may end up discovering one or two things that are problematic, but that's just a fear of "well things are always weird in the wild". If that comes up we can always figure out how to deal with it. It sounds like you've otherwise handled other known issues, hence me being comfortable landing this.
src/cargo/core/dependency.rs
Outdated
".*" => "*", | ||
"0.1.0." => "0.1.0", | ||
"0.3.1.3" => "0.3.13", | ||
"0.2*" => "0.2.*", | ||
"*.0" => "*", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for keeping the compat here! That being said the warning below says that it will soon become a hard error, and that was added 5 years ago in #3154. I think it's probably fine to just delete all this at this point (and I'm glad you moved this logic to Cargo from the semver
crate itself!)
Team member @alexcrichton has proposed to merge this. The next step is review by the rest of the tagged team members: No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
From the description of the situation around Also now that a lot more internals of a But all of that can be looked at in flow up PR. So I am +1. |
🔔 This is now entering its final comment period, as per the review above. 🔔 |
Thanks for being so thorough! Just to be clear, the current crates in the index that will now fail are: Bad versions:
Bad requirements:
To the best of my knowledge, these entries in the local cache will just cause the cache to be rebuilt. Otherwise, Cargo will behave as-if these releases don't exist. |
@bors r+ |
📌 Commit 7ace447 has been approved by |
⌛ Testing commit 7ace447 with merge 7c479d26ca12eccafeb15aa5ae3c5507373236e2... |
💔 Test failed - checks-actions |
That'll get fixed by #9517, will retry once that merges first. |
@bors: retry |
☀️ Test successful - checks-actions |
Update cargo 10 commits in e931e4796b61de593aa1097649445e535c9c7ee0..0cecbd67323ca14a7eb6505900d0d7307b00355b 2021-05-24 16:17:27 +0000 to 2021-06-01 20:09:13 +0000 - Configure hosts separately from targets when --target is specified. (rust-lang/cargo#9322) - Add some validation to rustc-link-arg (rust-lang/cargo#9523) - Implement suggestions for unknown features in workspace (rust-lang/cargo#9420) - Extract common `make_dep_path` to cargo_util (rust-lang/cargo#9529) - Add a note about rustflags compatibility. (rust-lang/cargo#9524) - Consolidate doc collision detection. (rust-lang/cargo#9526) - Add `--depth` option for `cargo-tree` (rust-lang/cargo#9499) - `cargo tree -e no-proc-macro` to hide procedural macro dependencies (rust-lang/cargo#9488) - Update to semver 1.0.0 (rust-lang/cargo#9508) - Update tar dependency to 0.4.35 (rust-lang/cargo#9517)
This seems to contain a breaking change where previously version strings like |
Update to semver 1.0.3 Fixes rust-lang/cargo#9508 (comment) by pulling in dtolnay/semver#247. Fixes rust-lang/cargo#9543
Since cargo v1.54, invalid syntax for version requirements is rejected cf) https://github.com/rust-lang/cargo/blob/master/CHANGELOG.md#cargo-154-2021-07-29 rust-lang/cargo#9508 Signed-off-by: Junyeong Jeong <rhdxmr@gmail.com>
Since cargo v1.54, invalid syntax for version requirements is rejected cf) https://github.com/rust-lang/cargo/blob/master/CHANGELOG.md#cargo-154-2021-07-29 rust-lang/cargo#9508 Signed-off-by: Junyeong Jeong <rhdxmr@gmail.com>
Cargo in the latest version upgraded the server dependency (rust-lang/cargo#9508). As a result, it no longer accepts "" as a version. So we now use a specific version of all such usage.
* Fix cargo failures on rust version 1.54 Cargo in the latest version upgraded the server dependency (rust-lang/cargo#9508). As a result, it no longer accepts "" as a version. So we now use a specific version of all such usage. * Fix new clippy warnings new to Rust 1.54
With this, rust-lang/cargo#9508, the dependency can no longer be used. What was picked with 1.53 cargo was ~2.0, so use that instead. Test: verify dependency can now be used
With this, rust-lang/cargo#9508, the dependency can no longer be used. What was picked with 1.53 cargo was ~2.0, so use that instead. Test: verify dependency can now be used Signed-off-by: Jean-Philippe DUFRAIGNE <j.dufraigne@gmail.com>
* Fix cargo failures on rust version 1.54 Cargo in the latest version upgraded the server dependency (rust-lang/cargo#9508). As a result, it no longer accepts "" as a version. So we now use a specific version of all such usage. * Fix new clippy warnings new to Rust 1.54
I am working on a 1.0.0 of the
semver
crate some time this week. It would be good to confirm Cargo will be able to use it, beforehand!It's a from-scratch rewrite, but dtolnay/semver#237 has code to compare against 0.10.0 (currently used by Cargo) how every possible version requirement currently published to crates.io matches against every possible crate version. The differences are all broken syntax like
^0-.11.0
previously parsing with ".11.0" as a pre-release string (which is invalid, because pre-release are not allowed to contain empty dot-separated identifiers) and~2.0-2.2
previously parsing with "2.2" as a pre-release string, when the user almost certainly meant>=2.0, <=2.2
. I'm not sure how much of those you want to add code into Cargo to preserve behavior, but I would be happy to do it.