upgrade semver #3154
upgrade semver #3154
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
|
Looking good to me! It should be fine to just thread a |
|
👍 on it |
|
Whew! That was... a lot of stuff. I think I got it though. I think there might be some places where I didn't need to thread it this much, mostly around workspaces. Also, there's no tests for this in Cargo. I am testing in SemVer, though... |
|
This is still pointing at my special branch, but if the code looks good, I can cut a semver version and fix up the Cargo.toml. But I think this is ready for review. |
| @@ -27,7 +27,7 @@ pub trait Source: Registry { | |||
|
|
|||
| /// The download method fetches the full package for each name and | |||
| /// version specified. | |||
| fn download(&mut self, package: &PackageId) -> CargoResult<Package>; | |||
| fn download(&mut self, package: &PackageId, config: &Config) -> CargoResult<Package>; | |||
There was a problem hiding this comment.
Most of these Source types already store Config, so I think it may not need to get passed in here?
There was a problem hiding this comment.
(which I think may end up backing out a lot of the threading around)
There was a problem hiding this comment.
ah ha! I did not know that. That would be... great.
| ReqParseError::DeprecatedVersionRequirement(requirement) => { | ||
| let msg = format!("One of your version requirements ({}) is invalid. \ | ||
| Previous versions of Cargo accepted this malformed requirement, but it is being deprecated. Please \ | ||
| use {} instead.", req, requirement); |
There was a problem hiding this comment.
I think we'll emit this for anything cargo parses, so perhaps something like:
parsed version requirement `{}` is no longer valid
Previous versions of Cargo accepted this malformed requirement,
but it is being deprecated. This was found when parsing the manifest
of iron v1.2.3, and the correct version requirement is `0.2.0`.
This will soon become a hard error, so it's either recommended to
update to a fixed version or contact the upstream maintainer about
this warning.
That is, the requirement may not always be local, so we'll just want to help everyone find the crate at fault asap.
There was a problem hiding this comment.
Do we have the crate name at this point? I thought it was just the version string.
There was a problem hiding this comment.
Not immediately on hand, but I think it can be passed through?
There was a problem hiding this comment.
It's.... not very simple. Saying "Hey the dependency "foo" is, but not which crate has the "foo" dependency. Ugh.
There was a problem hiding this comment.
Ah yeah I'm just worried about users looking at this error and not really knowing what to do when the error shows up. The current versions says "one of you version requirements" but that's actually not true if it's coming from some upstream dependency (and then you have to hunt and track that down)
There was a problem hiding this comment.
I certainly think it has a lot of value. It's just gonna take me a while to figure it all out.
There was a problem hiding this comment.
Okay, I think I'm defeated by this. Do you have any advice, @alexcrichton ? Sorry 😦
|
☔ The latest upstream changes (presumably #3136) made this pull request unmergeable. Please resolve the merge conflicts. |
|
(oh btw @alexcrichton I have the "allow edits from maintainers" so you SHOULD be able to push directly to my branch) |
Provide some contextual information about why a dependency failed to parse.
|
@bors: r+ |
|
📌 Commit f40f8c6 has been approved by |
|
@bors: r- |
|
Thanks @alexcrichton ! Yeah, you know the source way better than I do..... I r-'d this because it's still pointing at my unreleased branch; let me cut a semver version. |
|
@bors: r+ 👍 |
|
📌 Commit 464e6a9 has been approved by |
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.
|
☀️ Test successful - cargo-cross-linux, cargo-linux-32, cargo-linux-64, cargo-mac-32, cargo-mac-64, cargo-win-gnu-32, cargo-win-gnu-64, cargo-win-msvc-32, cargo-win-msvc-64 |
|
Yesssssssssssssssssssssssssssssssssssssssssssss |
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:
cargo/src/cargo/util/toml.rs
Line 166 in 5593045
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.