rust-lang / cargo

The Rust package manager
https://doc.rust-lang.org/cargo
Apache License 2.0
12.57k stars 2.38k forks source link

When generating version requirements, use the oldest "good" version within a major version #14372

Open ia0 opened 1 month ago

ia0 commented 1 month ago

Problem

Proposed Solution

I'm thinking that cargo add (resp. cargo update --breaking) would add (resp. update to) the oldest version satisfying both properties below:

The exact algorithm would be:

Notes

The current behavior is to use the latest published version (not sure if vulnerabilites are checked).

I'm obviously fine if I have to use a flag, e.g. cargo add --bikeshed and cargo update --breaking --bikeshed.

epage commented 1 month ago

I only want to use caret requirements (I'm open to be convinced otherwise).

This is our default stance as well, see our guidelines

I don't want to use a version that is known to be vulnerable (according to RustSec).

Integration of security checks is being tracked in #7678.

imo not all security vulnerabilities are created equal. I've had people ping me about my Cargo.lock containing "vulnerable" versions of regex when

cargo add --bikeshed

I feel like doing this by default would run counter to user expectations. If I ran cargo add serde and got serve = "1.0.0", then I will be have a negative experience.

When starting this reply, I was assuming this is something users would want as a blanket policy and config would be more appropriate but serde is a great example of where this doesn't work as a blanket policy. This actually ends up being more a function of how stable the API is for how well "pick the oldest within a major version" would actually work.

In light of the serde case, how that applies to a lesser extent to a lot of other cases (e.g. clap), and that we don't want to "punish' stable APIs, I'm starting to think that even a flag for this wouldn't be appropriate. Maybe people could play with a third-party command, like cargo msrv that finds the lowest version req that can work for their tests.

cargo update --breaking --bikeshed

I feel like this runs counter to the mental model of cargo update. You are telling it to update, so not updating would be strange, even with a flag.

ia0 commented 1 month ago
  • so raising the version requirement above that vulnerable version would run counter to "I want my users to have maximal choice" which I also try to maintain

That's a fair point. I agree this is subtle and I don't really see an obvious solution. It might be rare enough that manual intervention after the default generated version requirement is acceptable.

I feel like doing this by default would run counter to user expectations. If I ran cargo add serde and got serve = "1.0.0", then I will be have a negative experience.

Good point. I am fine manually editing the version requirement to the oldest version that contains all features I use, but I can see how it's a very bad idea for most users.

I'm starting to think that even a flag for this wouldn't be appropriate. Maybe people could play with a third-party command, like cargo msrv that finds the lowest version req that can work for their tests.

I'm going from the principle that users shouldn't use flags that don't fit their usecase, in which case, the only cost of a flag is its maintainance cost. That said, I can see how the maintainance could be too high for the benefits of the flag. In which case, as you suggest, a third-party command would be the right approach. If that's the issue conclusion, I would be curious to look into it if I get time.

I feel like this runs counter to the mental model of cargo update. You are telling it to update, so not updating would be strange, even with a flag.

It's true that using this flag without --breaking is probably meaningless. It's actually more like a strategy for --breaking: how to choose the version within the breaking ones. The thing is that in my mental model, --breaking should be the default behavior of cargo update.

I guess I'll just wait until either this issue reaches a conclusion regarding whether this should be a third-party command, or if I get time to play around with writing a third-party command (which would essentially provide cargo add --bikeshed --no-default-features and cargo update --breaking --bikeshed).

epage commented 1 month ago

I'm going from the principle that users shouldn't use flags that don't fit their usecase, in which case, the only cost of a flag is its maintainance cost.

UI / documentation bloat is another problem. This came up in the discussion of --out-dir (#6100) and is a problem I frequently see with clap users (people either don't use everything they can of clap or ask a lot of questions about what features exist because they can't find features among the large selection).

It's true that using this flag without --breaking is probably meaningless. It's actually more like a strategy for --breaking: how to choose the version within the breaking ones. The thing is that in my mental model, --breaking should be the default behavior of cargo update.

Even with --breaking. --breaking is a "force" flag, not a strategy flag. cargo update picks the latest, so a flag that doesn't feels strange (I've had other thoughts for #5657).

ia0 commented 1 month ago

Even with --breaking. --breaking is a "force" flag, not a strategy flag. cargo update picks the latest, so a flag that doesn't feels strange (I've had other thoughts for #5657).

I see. I would consider this a rather strong argument for making it a third-party command then. And thanks for the link to the minimal-version discussion. That third-party command should also:

ia0 commented 1 month ago

I feel like this runs counter to the mental model of cargo update. You are telling it to update, so not updating would be strange, even with a flag.

Actually, thinking a bit more about this, I fell like the behavior of cargo update might need to depend on whether the crate is[^1] a binary (use the latest versions) or a library (use the oldest versions matching some criteria[^2]). I wonder if this came up in another issue. I couldn't find anything. Maybe the answer is that cargo update shouldn't be used for libraries.

[^1]: Crates can have multiple build-targets, so this concept of whether a crate is a binary or a library is actually invalid. I thought I saw an issue about build-target-specific dependencies which could solve this, but couldn't find it again. [^2]: For example "major version is the latest", "no security vulnerability", or a combination of.

epage commented 1 month ago

I wonder if this came up in another issue. I couldn't find anything.

This came up quite a bit in:

https://internals.rust-lang.org/t/feedback-on-cargo-upgrade-to-prepare-it-for-merging/17101

Maybe the answer is that cargo update shouldn't be used for libraries.

The problem is that you still run into issues with Cargo choosing versions different versions for your Cargo.lock than your Cargo.toml. If cargo update could be made to work to keep the two in sync with either #10498 or #5657 then it could still work.