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

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

Closed
ia0 opened this issue Aug 8, 2024 · 8 comments
Labels
C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` Command-add S-needs-team-input Status: Needs input from team on whether/how to proceed.

Comments

@ia0
Copy link

ia0 commented Aug 8, 2024

Problem

  • I'm a library writer.
  • I only want to use caret requirements (I'm open to be convinced otherwise).
  • I want my library to work with the most recent version (according to crates.io) of its dependencies.
  • I want my users to have maximal choice in the version of my dependencies.
  • I don't want to use a version that is known to be vulnerable (according to RustSec).

Proposed Solution

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

  • It is compatible with the latest version (published on crates.io), i.e. its major version is the same as the major version of the latest version.
  • It does not have a security vulnerability. (Optionally, no later version has a security vulnerability, but my personal preference would be to allow that.)

The exact algorithm would be:

  • Look at the most recent version on crates.io.
  • Go through all versions sharing the same major version in increasing order.
  • Return the first one that is not vulnerable.
  • Default to the latest version if they are all vulnerable.

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.

@ia0 ia0 added C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` S-triage Status: This issue is waiting on initial triage. labels Aug 8, 2024
@epage epage changed the title Use the oldest version compatible with the latest version and without security vulnerabilities When generating version requirements, use the oldest "good" version within a major version Aug 8, 2024
@epage
Copy link
Contributor

epage commented Aug 8, 2024

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

  • I don't actually use regex in the production code
  • If I did, the vulnerability doesn't affect most or any of my users, 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.

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.

@epage epage added Command-add S-needs-team-input Status: Needs input from team on whether/how to proceed. and removed S-triage Status: This issue is waiting on initial triage. labels Aug 8, 2024
@ia0
Copy link
Author

ia0 commented Aug 8, 2024

  • 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
Copy link
Contributor

epage commented Aug 8, 2024

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
Copy link
Author

ia0 commented Aug 8, 2024

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:

  • Make sure the lock file uses the versions of the toml file for direct dependencies.
  • Warn if a dependency depends on a direct dependency with a stricter version requirement. Because the direct dependency version requirement is thus meaningless and should be updated to the intersection of the version requirement of that dependency in the full dependency tree.

@ia0
Copy link
Author

ia0 commented Aug 9, 2024

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 is1 a binary (use the latest versions) or a library (use the oldest versions matching some criteria2). 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.

Footnotes

  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
Copy link
Contributor

epage commented Aug 9, 2024

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.

@epage
Copy link
Contributor

epage commented Oct 29, 2024

We discussed this in today's Cargo team meeting.

While we are understanding of wanting to give flexibility for users on versions, there is too much variance within a major version to pick the oldest within that and too little community convention around minor versions to pick the oldest within that. There are also likely too many caveats around this workflow to smooth this out for end users, especially considering cargo add is a UX heavy command focused on ease of use, rather than supporting every workflow.

We are inclined to close this but could revisit it if there is sufficient new information.

@epage epage closed this as not planned Won't fix, can't repro, duplicate, stale Oct 29, 2024
@ia0
Copy link
Author

ia0 commented Oct 29, 2024

Thanks for the feedback! I agree this is better addressed with a third-party command, which I hope I'll have time to implement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` Command-add S-needs-team-input Status: Needs input from team on whether/how to proceed.
Projects
None yet
Development

No branches or pull requests

2 participants