killercup / cargo-edit

A utility for managing cargo dependencies from the command line.
http://killercup.github.io/cargo-edit/
MIT License
3.07k stars 148 forks source link

`cargo add`: include only MAJOR.MINOR #126

Closed ordian closed 2 years ago

ordian commented 7 years ago

Add an option (or enable by default) to add a dependency with version = MAJOR.MINOR, not MAJOR.MINOR.PATCH.

killercup commented 7 years ago

Why?

ordian commented 7 years ago

Why?

Why e.g. semver and regex are specified in this form in cargo-edit's Cargo.toml? :trollface:

Usually, you want to specify a minimal required feature level, so e.g. docopt uses this model.

I would rather ask why do you want to specify the full version?

killercup commented 7 years ago

Good points!

If adding dependencies by hand, I tend to omit the patch version, except if it's an 0.x and I know this feature was added recently, like regex = "0.1.66".

From a practical stand point, this is aesthetics, though: 1.2.3 and 1.2 and 1 will all load 1.4.9 (if that is the latest version), as cargo interprets this as ^1.2.3 etc.

From a pessimistic stand point: 1.2.0 to 1.2.2 are probably buggy versionsβ€”why else would they publish a 1.2.3? So, let's make sure we (and hopefully als other dependencies) are using the absolutely latest version!

As you can see, there seem to be good arguments for both sidesβ€”and I'd love to hear some more before deciding!

Andronik Ordian notifications@github.com schrieb am Do. 29. Juni 2017 um 01:18:

Why?

Why e.g. semver and regex are specified in this form in cargo-edit's Cargo.toml? [image: :trollface:]

Usually, you want to specify a minimal required feature level, so e.g. docopt https://github.com/docopt/docopt.rs/blob/master/Cargo.toml uses this model.

I would rather ask why do you want to specify the full version?

β€” You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/killercup/cargo-edit/issues/126#issuecomment-311818430, or mute the thread https://github.com/notifications/unsubscribe-auth/AABOX0xJ-m_uioA7lhD29mlL6mlAvgvEks5sIt9DgaJpZM4OIifr .

ordian commented 7 years ago

From a practical stand point, this is aesthetics, though: 1.2.3 and 1.2 and 1 will all load 1.4.9 (if that is the latest version), as cargo interprets this as ^1.2.3 etc.

I would argue that 1.2 is better than 1 because the latter doesn't specify the minimal feature set, while the former does.

From a pessimistic stand point: 1.2.0 to 1.2.2 are probably buggy versionsβ€”why else would they publish a 1.2.3? So, let's make sure we (and hopefully als other dependencies) are using the absolutely latest version!

Consider the following example. At some point, you add a dependency X via cargo add, let's say 1.2.1. Later a bug in X was found. But you forgot to update the dependency. In this scenario, 1.2.1 is worse than 1.2, because 1.2 will only compile to latest version.

killercup commented 7 years ago

I would argue that 1.2 is better than 1 because the latter doesn't specify the minimal feature set, while the former does.

True. But you can easily argue that 1.2.3 is better than 1.2 because the latter doesn't specify the minimal level of patches that make the needed feature work while the former does :)

In this scenario, 1.2.1 is worse than 1.2, because 1.2 will only compile to latest version.

If I understand you correctly, this is false, actually. Assuming

But you forgot to update the dependency.

means you already have the dependency version pinned in your Cargo.lock, they both act the same. And cargo update will update both to the latest version that is <2.0.0. Cargo assumes ^ default, see this for details.

ordian commented 7 years ago

True. But you can easily argue that 1.2.3 is better than 1.2 because the latter doesn't specify the minimal level of patches that make the needed feature work while the former does :)

True, but only if you rely on the bug being fixed.

If I understand you correctly, this is false, actually.

Yeah, my brain wasn't functioning late at night. Sorry about that.

So, what you was saying in the pessimistic point is that if we have a transitive dependency X = "=1.2.0" and if we specify direct dependency X = "1.2" - it will compile, while with X = "1.2.3" it won't, forcing us to update the transitive dependency? I think, this is the rather contrived example.

In summary, I think adding an option to remove patch version won't hurt. As well as, adding an option not to mess the whole Cargo.toml :rofl: (but that's the different issue).

killercup commented 7 years ago

In summary, I think adding an option to remove patch version won't hurt.

Hurt? No. But it's one more thing to maintain, and I've not yet seen a reason to bother. Are there maybe other people who also want to have this?

an option not to mess the whole Cargo.toml

No! Not an option! The default! And no --mess flag πŸ˜„ (cf. #15)

spease commented 6 years ago

I found this ticket because I've generally been only recording major.minor in my manual Cargo.toml files because I thought that was the recommended thing to do. The only case that I can see it changing the functional behavior is if somebody were to publish and then yank a version of a crate (eg if an optimization patch was applied, but then was found to introduce a security hole or regression) - in this case specifying the patch version would prevent the library from building if somebody just checked it out until a newer version of the library was published.

However it does change how it's read, since having three instead of two numbers suggests that the author of the library very specifically chose that patch number and newer, rather than simply choosing the latest patch of the major-minor numbers.

However, alternatively, you could look at the Cargo.toml as more of a log rather than a spec for what the author last built the library with. This seems more of a Cargo.lock duty though, and the rest of the information in Cargo.toml seems to generally be a as-minimal-as-possible defintion of what the library is and how it should be built.

EDIT: NB that I found this after running the cargo upgrade command, not cargo add, but I'd expect that the two commands would have the same convention (two or three version points).

spease commented 6 years ago

I ran into another issue today - using cargo upgrade more tightly constrains the dependency graph compared to using "major.minor" only. I had to step back serde and tokio_core by a version (eg 1.0.38 to 1.0.37) because of dependent libraries that this conflicted with.

daboross commented 6 years ago

I would definitely appreciate an option in cargo add and cargo upgrade to stick to minimal-information specification.

It's a very small wart, but I prefer to keep all of my dependency versions as 0.4 or 1 rather than specifying more detail and thus end up manually editing Cargo.toml after every use of a cargo-edit command to remove the minor and patch versions.

YaLTeR commented 6 years ago

Tried cargo upgrade for the first time today and I'm also one of these people who would prefer it to stick to 1 and 0.4 rather than major.minor.patch. πŸ˜ƒ

shepmaster commented 6 years ago

I'll chime in as being in favor of cargo-edit maintaining the most specific version number possible. I'm happy that it uses the current patch level...

In fact, I'd actually love it if there was a way to walk backwards in versions to find the earliest usable / compatible version of a dependency so that my requirements are the most flexible for the people using my crate.

YaLTeR commented 6 years ago

Finding the earliest version is most certainly a bad idea because:

shepmaster commented 6 years ago

@YaLTeR I disagree with your assessment because it seems to not take into account Cargo's resolution logic.

If my crate A relies on libc 0.0.1 and another crate B relies on 0.0.2, a third crate C depending on both A and B will use 0.0.2 just fine. The user gets the maximal bug fixes possible.

Putting 0.0.1 in the Cargo.toml does not require that specific version will be picked, just one semver-compatible with it. Refer to the Cargo docs for the full list of operators you can use to express your crate's version constraints. For example, twox-hash is compatible with rand 0.3.10, 0.4.x, and 0.5.x because the API surface it needs hasn't changed in those versions. There's no reason for me to be incompatible with another crate that requires rand 0.4 and force the user to upgrade to 0.5.

and another of my crate's dependencies had even its earliest version depending on a newer openssl-sys

That's the problem that I'm advocating for avoiding. If that crate was compatible with both the older and newer versions and specified it in the Cargo.toml, you wouldn't have had the problem in the first place.

YaLTeR commented 6 years ago

Hmm, yeah, good point. Is there a way to force a crate with a requirement like twox-hashes to use a specific, not latest version of the depentent crate?

shepmaster commented 6 years ago

a crate with a requirement like twox-hashes

In a project that uses twox-hash, and a side dependency, Cargo will pick multiple versions (for whatever reason), but you can choose to unify them:


$ cat Cargo.toml
[dependencies]
twox-hash = "1.1.1"
rand = "0.4"

$ cargo tree
ex v0.1.0 (file:///private/tmp/ex)
β”œβ”€β”€ rand v0.4.3
β”‚   └── libc v0.2.43
└── twox-hash v1.1.1
    └── rand v0.5.5
        β”œβ”€β”€ libc v0.2.43 (*)
        └── rand_core v0.2.1

$ cargo update -p rand:0.5.5 --precise 0.4.3

$ cargo tree
ex v0.1.0 (file:///private/tmp/ex)
β”œβ”€β”€ rand v0.4.3
β”‚   └── libc v0.2.43
└── twox-hash v1.1.1
    └── rand v0.4.3 (*)
daboross commented 6 years ago

In fact, I'd actually love it if there was a way to walk backwards in versions to find the earliest usable / compatible version of a dependency so that my requirements are the most flexible for the people using my crate.

If this behavior is provided, I would be happy to not have support MAJOR.MINOR w/o patch.


Until we have that, though, I'm going to keep pushing for the option to exclude patch and possibly minor versions.

The main reason I remove the patch is that it's fairly arbitrary. 99% of the time it's just whatever patch version was the latest when I added the dependency. It usually doesn't even represent the features used, since I could easily end up depending on newer features without updating the patch version - I'd only update the actually used version in Cargo.lock. I'd never update the patch versions in any case as that creates commits and churn for no reason! Someone using cargo update on the end project will get the latest compatible version of all of my dependencies anyways.

I prefer to list the least-specific which still has semver information because further information (MINOR version if MAJOR > 0, or PATCH if MAJOR.MINOR > 0) is at best useless, and at worst outright incorrect.

shepmaster commented 6 years ago

I could easily end up depending on newer features without updating the patch version

I recommend adding a line to your CI to prevent this:

cargo +nightly -Z minimal-versions test
shepmaster commented 6 years ago

version in Cargo.lock

I don't think I've stated this explicitly, but I'm mostly concerned with libraries. For binaries it doesn't seem like like it really matters what you put in your Cargo.toml because there is a Cargo.lock.

daboross commented 6 years ago

@shepmaster

I really should get into the habit of using minimal-versions. Just haven't updated CI configs since it became a thing. Thanks for mentioning that.

If we get that and "update to minimal version which still compiles & passes tests" then I'd be good fully abandoning this.

I was considering binary crates as well only because I follow the same principal of excluding patch versions for them, and this issue is also a problem for them. Yes, it doesn't matter as much what's in Cargo.toml, but that's even more reason to not include unnecessary and semi-arbitrary versions.

YaLTeR commented 6 years ago

Another concern I have with versions like in the twox-hash example: you're probably developing and testing the crate on the latest crate version (rand 0.5 in this example), so you can accidentally start using an API that wasn't covered by earlier versions (0.3 or 0.4) without noticing it. Does that mean you now essentially need to test on every combination of every version of a dependency like that? @shepmaster

EDIT: also why was this marked wontfix?..

shepmaster commented 6 years ago

you're probably developing

Yep, most likely

and testing the crate on the latest crate version

See my previous comment for one way to test this.

Does that mean you now essentially need to test on every combination of every version of a dependency like that?

Because semver is a human construct and not something mandated by the compiler, technically yes, you would always need to test with every possible version to be sure. I take a more practical approach and trust that other developers get semver right most of the time.

Note that this is already a problem you could experience. If you put itertools = "1.0" in your Cargo.toml, as suggested by this issue, then you are saying that your code works with 1.0.0, which seems more likely to be incorrect than itertools = "1.2.35". The current behaviour, of picking the current version, has the benefit that your code was tested with the version of the crate you specify at one point in time.

Most CI setups ensure that you work with the newest versions of every possible crate (since the lockfile isn't committed, you re-resolve on every run). It's possible to check for the oldest version of every possible crate via -Z minimal-versions. I don't know of any way to check every permutation πŸ˜‰

YaLTeR commented 6 years ago

I take a more practical approach and trust that other developers get semver right most of the time.

Yeah, but in case of a requirement like >= 0.3, < 0.6 it's perfectly valid for, e.g., 0.5 to add some new API which you might start using without realizing it wasn't available in 0.3. I suppose -Z minimal-versions is of help here indeed.

ghost commented 4 years ago

Wanted to follow up on this as its getting a little old to keep manually needing to remove the revision from all the dependencies in my Cargo.toml file. Are there any plans to support ignoring it and allowing just major.minor versions in this crate?

ordian commented 4 years ago

I don't have time to work on this, but can review a PR if someone wants to implement this.

ghost commented 4 years ago

I had a thought for all on this thread. In addition to the major.minor specification or limit, would it also help to have an option to do wildcards?

pwoolcoc commented 3 years ago

I've opened a possible solution to this, I'd appreciate comments from maintainers as well as anyone interested in this feature

spease commented 3 years ago

I think this would be better done on a per-project basis or via command-line flag when you add or upgrade something.

I think this would most likely be due to project policy of the project adding the dependencies; or the project which is being depended on. Some projects may worry more about backwards compatibility than others and adhere to it or track it better.

Different users opens the door to the possibility that one user will get a different result with cargo upgrade than another user or the CI unintentionally and this could cause needless churn especially for projects that take contributions from incidental maintainers. Especially if each developer works on multiple projects which have different policies as to how they want to manage their .toml.

shepmaster commented 3 years ago

via command-line flag when you add or upgrade something

I agree. A key reason is how Cargo treats 1.x.y vs 0.x.y.


Over the past few days, I've had a few unrefined thoughts about this...

kornelski commented 2 years ago

I think supporting this would be a disservice.

When crates follow semver, then the patch version is for bug fixes, and is not supposed to have compatibility implications, so leaving out the patch basically only allows more bugs.

And when crates don't follow semver, then it's even riskier to allow arbitrary crate versions that have unknown compatibility. This is not a theoretical problem, major crates like serde and cc violate semver-minor rules and only bump semver-patch when adding new features to their public interface. serde 1.0.0 is not compatible with serde 1.0.99, and therefore serde = "1.0" would allow incompatible versions.

These imprecise versions cause major breakage and prevent -Z minimal-versions from working properly. This creates a chicken-egg problem that crates that would like to be precise and conservative with minimal dependency versions can't do it, because it's difficult to test it. We first need to fix the ecosystem and get everyone to bump to working latest versions, so that minimal-versions is usable and can be used in CIs to verify that dep versions are specified correctly.

ordian commented 2 years ago

A counterargument would be if a crate releases a new version that is later yanked. Updating Cargo.toml to the latest version would leave the project and its dependants in a broken state. Patch version should be updated in Cargo.lock of the end application IMHO.

Kixunil commented 2 years ago

There's also a bunch of annoying edge cases like a new "bugfix" version actually introducing another bug or being incompatible with OS/MSRV/whatever. Forcing newer versions prevents workarounds to those bugs.

I think not following semver is a problem of the crates not following it. So far seems like it was fine but maybe this is a good reason to get people to stop doing it or get explanation why they don't use semver and figure out what to do about it.

kornelski commented 2 years ago

There's also a bunch of annoying edge cases like a new "bugfix" version actually introducing another bug or being incompatible with OS/MSRV/whatever

The user of cargo add will get the very latest patch version regardless of how the version is specified, because that's what Cargo will fetch regardless. So while the problem you're describing is real and sometimes happens, the imprecise version specification does not do anything for this case.

Don't forget we're not talking about cargo update that breaks an existing working setup, but addition of a new crate to a project. This is a substantially different circumstance - it affects only one project that is a work-in-progress and hasn't been published yet, and there is no code to be incompatible with yet.

epage commented 2 years ago

Some quick thoughts:

Kixunil commented 2 years ago

@kornelski ah, yeah, I had some things confused. So as long as it doesn't lead to encouraging people to bumping the versions in Cargo.toml aggressively, I think I'm fine with that approach.

epage commented 2 years ago

Going to go ahead and close this based on this discussion and my proposal on internals

smoelius commented 2 years ago

I arrived at this thread looking for a similar feature for cargo upgrade. Since there seem to be strong opinions on supporting this for cargo add, are those opinions any different for cargo upgrade?

IMO, it would be nice if there was a way to preserve a dependency's existing precision when upgrading.

For example, consider:

itertools = "0.8"

As of this writing, cargo upgrae turns this into:

itertools = "0.10.3"

It would be nice if, say, cargo upgrade --preserve-precision could turn this into:

itertools = "0.10"
epage commented 2 years ago

I think my points from earlier still stand for cargo upgrade.

Ideally, we would be specifying the minimum version needed to work. The version you use at time of upgrade is most likely to be it since its easy to rely on new API features.

Maybe there is something I'm missing about intentionally having less precision. For myself, its mostly been out of laziness when hand writing the files but cargo-edit takes care of the laziness aspect by auto-selecting the version.

smoelius commented 2 years ago

Ideally, we would be specifying the minimum version needed to work.

This certainly applies to major and minor versions, but for patch versions, we should always want to use the most recent, shouldn't we? (Or am I missing something?)

Maybe there is something I'm missing about intentionally having less precision.

One reason appealing to me is to reduce Cargo.toml churn.

Suppose your project .gitignores the Cargo.lock file. Then

cargo update && cargo upgrade --to-locked --preserve-precision

would not dirty your repository if only patch versions are updated.

epage commented 2 years ago

This certainly applies to major and minor versions, but for patch versions, we should always want to use the most recent, shouldn't we? (Or am I missing something?)

I'll re-phrase to "ideally, we always specify at least the last working version". Without -Z minimal-versions being adopted, this is most likely whatever is in your lock file (or latest version without a lock file) since that is all that is being tested.

However, I would say version requirements should not be "always latest". We might want to upgrade binaries on a regular cadence but ideally, we don't update version requirements more than needed. It can cause extra build churn and cause conflicts unnecessarily (most patches are not critical)

One reason appealing to me is to reduce Cargo.toml churn.

Wouldn't --skip-compatible be the flag for reducing churn? There is #552 which proposes making it the default.

smoelius commented 2 years ago

You raise good points, @epage. But I would counter with the following.

There is a spectrum of possible approaches here. cargo upgrade's default behavior seems to occupy one end of that spectrum; --skip-compatible seems to occupy the other end. What I am suggesting allows for possibilities in between, and those possibilities could vary per dependency.

Furthermore, many project's Cargo.toml files use a mix of precisions. One can argue that those projects should do something else, but the fact of the matter is, they don't.

As a case in point, last year, I submitted a PR to update some of Clippy's dependencies. The PR was merged, but I was asked to remove the patch versions from the updated Cargo.toml file. (In that case, I used cargo upgrade to perform the update, and sed to remove the patch versions. I want to eliminate the need for sed.)

I think the bottom line is, if cargo upgrade were to adopt a --preserve-precision feature like I described, it would be applicable to more projects.

epage commented 2 years ago

I'll say that my opinion is less strong for preserving existing precision in cargo-upgrade than having configurable precision in cargo-add. Even better if we have consensus one way or the other and don't need a flag to control this.

I will say that cargo-set-version already has all of the logic for this as it was forked from cargo-release which implemented it all.

I do wonder if trends towards preferring a specific precision will die down with further adoption of cargo-add once its built-in to cargo.

smoelius commented 2 years ago

I'll say that my opinion is less strong for preserving existing precision in cargo-upgrade ...

Would you consider a PR to add a --preserve-precision flag like I described?

epage commented 2 years ago

First, we're well past time to have a dedicated issue for this :)

Second, don't forget this comment

Even better if we have consensus one way or the other and don't need a flag to control this.

Please, create an issue and I'll copy some points from here to there. Then feel free to create a PR that uses the cargo-set-version code unconditionally. Let's see how it works out and see what the feedback looks like from there.

With that said, I will have limited availability to help out as I should be keeping my focus on cargo-add (and clap) since cargo-add is being finalized for upstream. cargo-upgrade isn't even the next, so its fairly low in my priorities for attention, Maybe others can help step up if you do need any help.

kornelski commented 2 years ago

@smoelius I've checked that clippy PR, and clippy does not compile with the truncated versions it has specified in Cargo.toml. I think their ask to remove patch versions was objectively incorrect: they've asked to change dependencies to versions that are too old and don't work with clippy.

--preserve-precison may be an acceptable compromise, given that the crates ecosystem has a mix of semver and non-semver crates. Notably, the in case of serde the "patch" version spans 5 years of development, over 160 releases, and numerous new features in the public API. Serde's author intentionally breaks semver rules for patch releases (in all of their crates AFAIK), is unwilling to change this, and even tried to convince other crate authors to break semver in the same way. Given that a top-10 Rust crate doesn't follow semver, it's not safe to tools to assume that "patch" releases are unimportant.

smoelius commented 2 years ago

Thanks for looking into this, @kornelski, and thanks for your feedback. I like the way you say this:

--preserve-precison may be an acceptable compromise, given that the crates ecosystem has a mix of semver and non-semver crates.

Kixunil commented 2 years ago

and even tried to convince other crate authors to break semver in the same way

Sorry for OT, @kornelski do you have a link to some discussion? I'd like to see if there are interesting arguments I could learn from.

epage commented 2 years ago

--preserve-precison may be an acceptable compromise, given that the crates ecosystem has a mix of semver and non-semver crates. Notably, the in case of serde the "patch" version spans 5 years of development, over 160 releases, and numerous new features in the public API. Serde's author intentionally breaks semver rules for patch releases (in all of their crates AFAIK), is unwilling to change this, and even tried to convince other crate authors to break semver in the same way. Given that a top-10 Rust crate doesn't follow semver, it's not safe to tools to assume that "patch" releases are unimportant.

Personally, I think framing this as "breaking semver" as a bit strong. I am not fully aware of all of the conversations and ins and outs but the line between minor and patch has always been blurred and to say something "breaks" semver or not there is too definitive. If I see someone talk about not following semver or breaking semver, to me it means they aren't doing major versions for breaking changes.

kornelski commented 2 years ago

The serde, cc, and anyhow crates are unambiguously violating semver rules laid out in both official semver docs and in Cargo docs. It's not a blurry edge case, they did exactly what the docs say they must not do: they have added new public functions in "patch" releases.

epage commented 2 years ago

I didn't realize they are that specific but the user impact of that specificity is minor and framing this in language ("violating semver") that evokes much more user-impacting behavior (breaking changes in minor or patch releases)

Note that cargo already "violates" semver by inferring meaning in 0.x.y versions.

I can give reasons why the semver spec is wrong to be so specific and what my personal philosophy is but this is getting off topic and I feel like any such conversation will be unproductive with the approach I've seen taken in this thread and the linked threads. I would ask that we go ahead and put this sub-topic on pause, leaving room for anyone wanting to discuss the original topic and closing of this issue. If we find we cannot do this, I will lock the thread.