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

cargo upgrade reports 2.0 -> 2.0.0 as an upgrade #516

Closed
Wilfred opened this issue Sep 27, 2021 · 6 comments
Closed

cargo upgrade reports 2.0 -> 2.0.0 as an upgrade #516

Wilfred opened this issue Sep 27, 2021 · 6 comments

Comments

@Wilfred
Copy link

Wilfred commented Sep 27, 2021

$ cargo upgrade
    Updating '/~https://github.com/rust-lang/crates.io-index' index
difftastic:
    Upgrading colored v2.0 -> v2.0.0

My cargo.toml previously contained:

colored = "2.0"

This seems harmless, but it's a little confusing.

@epage
Copy link
Collaborator

epage commented Sep 27, 2021

Thanks for reporting this!

@savente93
Copy link
Contributor

This might be tricky to solve. The same behaviour occurs when you try it with colored = "2" I beleive the root of the issue originates here

if str_or_1_len_table(old_dep) {
// The old dependency is just a version/git/path. We are safe to overwrite.
*old_dep = new_toml;
} else if old_dep.is_table_like() {

The semver::Version struct requires all major minor and patch fields to be filled so when that is constructed it just fills them with 0. By the time we're merging the new and old dependencies in
fn merge_dependencies(old_dep: &mut toml_edit::Item, new_toml: toml_edit::Item) {
we're not even comparing but just overwriting, and at this point we're working with just toml/string data, not Versions so checking whether that is necessary without a bunch of extra parsing is going to be hacky at best. Of course, it's completely possible that I'm misunderstanding the situation, but if the above is correct, I'd say that fixing is maybe more effort than it's worth? especially considering that the behaviour disappears when the --skip-compatibility flag is added. thoughts?

@epage
Copy link
Collaborator

epage commented Oct 14, 2021

note: this is going to meander as I explore ideas

I've run into something similar with cargo-set-version and wonder if we can define a common interface for both.

cargo-set-version was forked from cargo-release. When you modify a crate's version, we automatically update the version for all path-dependencies to that crate. cargo-release allows the following policies:

  • Upgrade to latest
  • Make compatible
  • Error on incompatible
  • Warn on incompatible
  • Ignore

For "make compatible" and "upgrade to latest", we use the exact same requirement syntax as the user and just update the relevant fields. Say we are upgrading a dep = "2.0" to 2.2.1, we'd change the string to dep = "2.2".

For now, cargo-set-version only does "Upgrade to latest" since it is the safest option in case semver wasn't broken, the top-level crate might now require new functionality from its dependency.

With cargo-upgrade, I see a couple of needs

  • Upgrade across breaking versions (cargo upgrade --skip-compatible)
  • Update my minimum required versions
    • Staying compatible only (impossible today)
    • Everywhere (cargo upgrade)

Some questions for resolving this Issue:

  • What is the appropriate version requirement for each need above?
    • Be like cargo-set-version and follow the existing pattern?
    • Fully specify, using the same requirement syntax?
      • cargo-upgrade fully specifies but doesn't reuse the same syntax
    • Fully under-specify it?
  • How do we abstract this (ie flag names and defaults)?
  • How much does this also apply to cargo-set-version?

@savente93
Copy link
Contributor

I'm going to keep thinking about the correct behaviour for a bit, but I did notice one thing during the investigating I did for this issue and that is in the case of dep = "2.0" the associated semver::VersionReq will have a patch field of None as opposed to Some(0) which is the case in dep = "2.0.0" so if you do want to stay compatible with the user that might help in figuring out what format we should use.

Continuing on that thought, I think that one common interface that might be useful is a more structured validation of the satisfactory versions. Right now it's kind of happening in-place since previously only things like yanked and prerelease needed to be checked. #528 already adds a little bit of complexity there, but if we'd like to implement the use cases you mentioned above (e.g. --skip-compatible) I think it might be worth pulling that out into it's own functions at least. And if we're doing more serious validation anyway, we could potentially calculate the (string) diffs at that point and hand that down to the updating function which could then just apply them which would keep them consistent with what the user specified. (this is just an idea)
This would also yield a bit more flexibility around which of the cargo-release policies we'd like to implement. I'm most in favour of "make compatable, error if you can't" approach, but I don't have a lot of awareness about how that is consistent or not with other behaviour, and I can see cases for any of them, so having those be levers you can pull might be worth thinking about.

@epage
Copy link
Collaborator

epage commented Oct 14, 2021

I'm going to keep thinking about the correct behaviour for a bit, but I did notice one thing during the investigating I did for this issue and that is in the case of dep = "2.0" the associated semver::VersionReq will have a patch field of None as opposed to Some(0) which is the case in dep = "2.0.0" so if you do want to stay compatible with the user that might help in figuring out what format we should use.

We do this today for cargo-set-version, see upgrade_requirement

@epage
Copy link
Collaborator

epage commented Jul 13, 2022

#613 has an unstable option for preserving precision which will make this no longer reported. Going to close this as a duplicate of #598

@epage epage closed this as completed Jul 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants