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

Respect rust-version when selecting a version for cargo install #10903

Open
epage opened this issue Jul 26, 2022 · 8 comments
Open

Respect rust-version when selecting a version for cargo install #10903

epage opened this issue Jul 26, 2022 · 8 comments
Labels
C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` Command-install

Comments

@epage
Copy link
Contributor

epage commented Jul 26, 2022

When running cargo install foo, if my rust version is too old, I'll get compilation errors. If the failing crate sets rust-version, then I'll get a nice error message about it but no path forward for fixing it. #10891 adds a path forward for cargo install when the failure is in a dependency but not when its in the crate itself that is being installed.

Ideally, cargo install will select an older version (with a warning) that is compatible with my toolchain. As a step back from the ideal, I'd get an error with corrective steps (ideally, telling me what version to use).

@epage epage added Command-install C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` labels Jul 26, 2022
@epage
Copy link
Contributor Author

epage commented Jul 26, 2022

This is blocked on rust-version being in the registry

@8573
Copy link

8573 commented Jul 26, 2022

While I generally very much support supporting older Rust versions, I worry about how this would (not silently but) quietly give users insecure versions of programs, in the absence of either integration with RustSec or a culture of yanking insecure versions (which I would hope would be blocked on having a way to bypass yankedness for e.g. git bisect).

@epage
Copy link
Contributor Author

epage commented Jul 26, 2022

Our choices are

  • Error and let the user figure it out (current behavior)
  • Error and tell the user how to resolve it
    • This is included as an alternative in this issue
    • This puts it in the users hands for deciding what is safe for them
    • We are still leaving a road hump along the users path to accomplishing their goal, souring their opinion of the ecosystem and possibly being the part that broke their will to continue experimenting with the solution (which will include the common refrain "why is it erroring instead of just doing it")
  • Auto-select a compatible version
    • This is included as the primary proposed solution to this issue
    • The user might be surprised at the version used and lack of features
    • The crate version might have vulnerabilities that affect the user

Maybe I'm missing some use cases but I personally have little sympathy for the security side of this. If the error just leads them to selecting an old version anyways, did we really help them with security? How do we know the newest version is any more secure than the older version? This also isn't too different than if someone had cargo installed things previously.

@weihanglo weihanglo added S-blocked-external Status: ❌ blocked on something out of the direct control of the Cargo project, e.g., upstream fix and removed S-blocked labels Apr 18, 2023
bors added a commit that referenced this issue Apr 28, 2023
feat: Add `-Zmsrv-policy` feature flag

### What does this PR try to resolve?

Nothing noticeable....

The intent is to unblock experiments with different compatible MSRV policies like
- #9930
- #10653
- #10903

While I normally don't like PRs that do nothing on their own, this at least allows any one of those efforts to move forward with different people without juggling these base commits for whoever is first to include in their PR

While there isn't an RFC for this yet, this is intended to allow us to experiment to get a better idea of what we should put in an RFC.  In some cases, we first do an eRFC for this but I assumed this wouldn't be needed in this case as this builds on rust-lang/rfcs#2495 and, I'm assuming, will be more surgical in nature

### How should we test and review this PR?

The `Summary` changes are largely untested as they will be mostly tested through the future work that builds on this PR.  However, I wasn't too concerned about that because the code is relatively trivial.

### Additional information

I chose the name `msrv-policy` to distinguish this unstable feature from `rust-version`.  Though those appear in different places (`Cargo.toml` vs `-Z`), I can see them being confusing which was especially apparent when editing `unstable.md` which has an anchor for `rust-version`.
@cassaundra
Copy link
Contributor

cassaundra commented May 30, 2023

Thoughts I had from a discussion with Ed:

My inclination here is to have cargo install still error, but provide a more useful suggestion. I see a couple of problems with auto-selecting a compatible version:

  • Most users are running the latest Rust stable/nightly installed via rustup (per 2021 survey), so an out-of-date compiler is more likely to signal to the user that they should upgrade their installation, rather than install an older version of a package.
  • cargo install is pretty noisy already, so users might miss the warning.
    • The warning could be put at the end, but by that time the package has already been installed.
    • Even if made less noisy, the warning does not give the user much time to decide what to do.

Improving the error instead helps the average user (instructing them to upgrade their Rust version), but also the less common user (giving them a path forward to install with their current Rust version).

@epage
Copy link
Contributor Author

epage commented May 30, 2023

"programs you run" is a little different than "project dependencies" and can more likely be ahead.

I also understand the problem of cargo installs output. In general, I wonder if we can do things to shrink our build and test output but that is outside of the scope of this.

Telling the user what version they can install would be a good first step for improving the error message. As that is just UX, we can do that without putting it behind the -Zmsrv-policy feature flag. We can always explore auto-selecting for cargo install in the future.

@epage epage removed the S-blocked-external Status: ❌ blocked on something out of the direct control of the Cargo project, e.g., upstream fix label Aug 10, 2023
@epage
Copy link
Contributor Author

epage commented Aug 25, 2023

FYI #12553 should make further MSRV work a bit easier

epage added a commit to epage/cargo that referenced this issue Oct 10, 2023
The next step would be to also automatically install an MSRV compatible
version if compatible with the version req (rust-lang#10903).
This improved error message will still be useful if the MSRV compatible
version is outside of the version req.

I did this as the first step
- Helps people now, not needing to wait on `-Zmsrv-policy` to be stabilized
- Has fewer questions on how it should be done (or if it should be)
epage added a commit to epage/cargo that referenced this issue Oct 10, 2023
The next step would be to also automatically install an MSRV compatible
version if compatible with the version req (rust-lang#10903).
This improved error message will still be useful if the MSRV compatible
version is outside of the version req.

I did this as the first step
- Helps people now, not needing to wait on `-Zmsrv-policy` to be stabilized
- Has fewer questions on how it should be done (or if it should be)
epage added a commit to epage/cargo that referenced this issue Oct 10, 2023
The next step would be to also automatically install an MSRV compatible
version if compatible with the version req (rust-lang#10903).
This improved error message will still be useful if the MSRV compatible
version is outside of the version req.

I did this as the first step
- Helps people now, not needing to wait on `-Zmsrv-policy` to be stabilized
- Has fewer questions on how it should be done (or if it should be)
bors added a commit that referenced this issue Oct 10, 2023
fix(install): Suggest an alternative version on MSRV failure

### What does this PR try to resolve?

Moves users from a bad error message, suggesting `--locked` which won't do anything, to suggesting a version of a package to use instead.

The side benefit is errors get reported sooner
- Before downloading the `.crate`
- When installing multiple packages, before building the first

This comes at the cost of an extra `rustc` invocation.

### How should we test and review this PR?

Per-commit this builds it up, from tests to the final design.

### Additional information

This is also written in a way to align fairly well with how we'd likely implement #10903.
This improved error message will still be useful after that issue is resolved when the MSRV compatible version is outside of the version req.
@charles-r-earp
Copy link
Contributor

Telling the user what version they can install would be a good first step for improving the error message. As that is just UX, we can do that without putting it behind the -Zmsrv-policy feature flag. We can always explore auto-selecting for cargo install in the future.

Is it possible to implement cargo +nightly install -Z msrv-policy?

Will this be the default when stable, or is it opt in, behind resolver 3 (#9930)?

@epage
Copy link
Contributor Author

epage commented Mar 25, 2024

cargo install was left out of scope for the MSRV-aware Cargo work, besides the error improvement mentioned earlier, as there were additional design concerns to be worked out and we didn't want to smaller improvement on this.

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-install
Projects
None yet
Development

No branches or pull requests

5 participants