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.

Sign up for GitHub

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

Jump to bottom

fix(resolver): Prefer MSRV, rather than ignore incompatible #12950

Merged
merged 2 commits into from
Nov 14, 2023
Merged

fix(resolver): Prefer MSRV, rather than ignore incompatible #12950

merged 2 commits into from
Nov 14, 2023

Conversation

epage
Copy link
Contributor

@epage epage commented Nov 10, 2023

What does this PR try to resolve?

This is another experiment for #9930.

Comparing preferring over exclusively using MSRV compatible:

Benefits

  • Better error messages
  • --ignore-rust-version is implicitly sticky

Downsides

  • Can't backtrack for MSRV compatible version
  • Still requires workspace-wide MSRV (compared to our desired end state of declaring MSRV as yet another dependency)

How should we test and review this PR?

Additional information

Note: --ignore-rust-version is not yet implemented for the resolver.

This builds on #12930

@rustbot
Copy link
Collaborator

rustbot commented Nov 10, 2023

r? @ehuss

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added A-dependency-resolution Area: dependency resolution and the resolver S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 10, 2023
@epage epage mentioned this pull request Nov 11, 2023
Open
@epage epage force-pushed the msrv branch 3 times, most recently from be530f6 to bb34609 Compare November 13, 2023 16:44
@Eh2406
Copy link
Contributor

Eh2406 commented Nov 13, 2023

At a code level this looks fine, although it would be nice to have a test that correctly backtracked before and does not do so now.

How are we going to evaluate if this provides a better user experience for the people attempting to maintain MSRV?

@epage
Copy link
Contributor Author

epage commented Nov 13, 2023

At a code level this looks fine, although it would be nice to have a test that correctly backtracked before and does not do so now.

I feel fairly weak in creating resolver test cases...

How are we going to evaluate if this provides a better user experience for the people attempting to maintain MSRV?

I had considered trying to find ways to evaluate the two options until I realized

  • We might be able to stabilize this approach relatively easily as the default approach
  • We can always continue on the hard-error path later, even if this is stabilized

So I instead see the questions as

  • Is my base assumption correct that this can be an on-by-default approach?
  • Does this provide a sufficient quality of an experience?

@Eh2406
Copy link
Contributor

Eh2406 commented Nov 14, 2023

I feel fairly weak in creating resolver test cases...

Root depends on a. a comes in two versions, the newer version depends on b and the older version has no dependencies. The only version of b has an incompatible MSRV requirement. If I've constructed this correctly... Before this PR you will get the older version of a. After this PR you will get a and b and a MSRV error.

Is my base assumption correct that this can be an on-by-default approach?

I'm wondering if this should be an always on approach. We sort lock file versions first with no way to opt out of it. To be blunt, I don't want to have to write the description for the flag to turn off this functionality. :-P But then how does a library that has an MSRV policy test with the latest versions of everything. 🤔

Does this provide a sufficient quality of an experience?

I have never maintained a library with an MSRV policy, so I don't know that I'm the person to ask. My sense is that the hard part is dealing with a transitive dependency adding a new dependency on something that does not work with your MSRV. Which this PR does not help with. On the other hand, many real-world dependency trees are smaller and simpler than I imagine. And this seems pretty painless for the easy case.

@epage
Copy link
Contributor Author

epage commented Nov 14, 2023

I'm wondering if this should be an always on approach. We sort lock file versions first with no way to opt out of it.

I agree, I'm hoping we can just make this the default with little fanfare / ceremony.

To be blunt, I don't want to have to write the description for the flag to turn off this functionality. :-P But then how does a library that has an MSRV policy test with the latest versions of everything.

I was going to leverage --ignore-rust-version; I just haven't plumbed it through yet. It was a lower priority to me since the behavior is opt-in at the moment

I have never maintained a library with an MSRV policy, so I don't know that I'm the person to ask. My sense is that the hard part is dealing with a transitive dependency adding a new dependency on something that does not work with your MSRV. Which this PR does not help with. On the other hand, many real-world dependency trees are smaller and simpler than I imagine. And this seems pretty painless for the easy case.

Good point to call out that the quality of this is predicated on "if you care about MSRV, your dependencies should at least declare an MSRV". There are likely corner cases where that can still have problems from backtracking but it is unlikely to be a problem.

@Eh2406
Copy link
Contributor

Eh2406 commented Nov 14, 2023

Good point to call out that the quality of this is predicated on "if you care about MSRV, your dependencies should at least declare an MSRV". There are likely corner cases where that can still have problems from backtracking but it is unlikely to be a problem.

If that's the assumption of this mechanism, should it consider a version that does not state and MSRV as being sorted with the noncompatible versions?

@epage
test(resolver): Verify backtracking for MSRV resolver
0bc5d22
@epage
fix(resolver): Prefer MSRV, rather than ignore incompatible
0d29d3f 
This is another experiment for rust-lang#9930.

Comparing preferring over exclusively using MSRV compatible:

Benefits
- Better error messages
- `--ignore-rust-version` is implicitly sticky

Downsides
- Can't backtrack for MSRV compatible version
- Still requires workspace-wide MSRV (compared to our desired end state of declaring MSRV as yet another dependency)

This builds on rust-lang#12930
@epage
Copy link
Contributor Author

epage commented Nov 14, 2023

Alright, a backtracking test is now in

@Eh2406
Copy link
Contributor

Eh2406 commented Nov 14, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Nov 14, 2023

📌 Commit 0d29d3f has been approved by Eh2406

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 14, 2023
@bors
Copy link
Contributor

bors commented Nov 14, 2023

⌛ Testing commit 0d29d3f with merge 0fcc90d...

@bors
Copy link
Contributor

bors commented Nov 14, 2023

☀️ Test successful - checks-actions
Approved by: Eh2406
Pushing 0fcc90d to master...

@bors bors merged commit 0fcc90d into rust-lang:master Nov 14, 2023
20 checks passed
@epage epage deleted the msrv branch November 14, 2023 22:35
@weihanglo weihanglo mentioned this pull request Nov 16, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 16, 2023
@bors
Auto merge of rust-lang#117916 - weihanglo:update-cargo, r=weihanglo
525c91d 
Update cargo

19 commits in 6790a5127895debec95c24aefaeb18e059270df3..2c03e0e2dcd05dd064fcf10cc1050d342eaf67e3
2023-11-10 17:09:35 +0000 to 2023-11-16 04:21:44 +0000
- docs(ref): Find a place to comment on --cap-lints (rust-lang/cargo#12976)
- Switch from AtomicU64 to Mutex. (rust-lang/cargo#12981)
- If the only path is a loop then counted as the shortest path. (rust-lang/cargo#12977)
- fix(resolver): Prefer MSRV, rather than ignore incompatible (rust-lang/cargo#12950)
- fix error message for duplicate links (rust-lang/cargo#12973)
- Only filter out target if its in the package root (rust-lang/cargo#12944)
- Ignore changing_spec_relearns_crate_types on windows-gnu (rust-lang/cargo#12972)
- fix: do not panic when failed to parse rustc commit-hash (rust-lang/cargo#12965)
- query{_vec} use IndexSummary (rust-lang/cargo#12970)
- Bump to 0.77.0; update changelog (rust-lang/cargo#12966)
- Improve about information of `cargo search` (rust-lang/cargo#12962)
- Fix --quiet being used with nested subcommands. (rust-lang/cargo#12959)
- make some debug assertion failures more informative (rust-lang/cargo#12963)
- refactor(toml): Consistently lead with 'Toml' prefix (rust-lang/cargo#12960)
- refactor(toml): Remove unused method (rust-lang/cargo#12961)
- Fix non-deterministic behavior in last-use repopulation (rust-lang/cargo#12958)
- Add cache garbage collection (rust-lang/cargo#12634)
- refactor(toml): Improve consistency (rust-lang/cargo#12954)
- Fix typo (rust-lang/cargo#12956)
@epage epage mentioned this pull request Nov 28, 2023
Merged
bors added a commit that referenced this pull request Nov 28, 2023
@bors
Auto merge of #13066 - epage:msrv, r=Eh2406
5dfe9bf 
fix(resolver): De-prioritize no-rust-version in MSRV resolver

### What does this PR try to resolve?

This is a corner case without a good answer.
As such, this change leans on some happy-path entries existing and
preferring those.

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

### Additional information

This was originally discussed around the time of #12950 but was held off.

When working on this, I was considering other heuristics like
- If a future version has an MSRV, assume that it applies also to the current version
  - This can be added in the future
  - We likely would want to consider an alternative value, like inferring the rust-version from the manifest or the rust-version used from publish
- Sort no-MSRV versions of a package by minimal versions
  - The lower the version, the more likely it is to be compatible
  - This likely could apply to incompatible MSRVs (or we could reverse-sort those by rust-version) but those will error anyways without `--ignore-rust-version`, so I decided against these
  - I realized this was a backdoor to minimal versions for dependencies without a MSRV and that the community support isn't there for that yet to be a high enough quality of an experience
@epage epage mentioned this pull request Nov 30, 2023
Open
27 tasks
@ehuss ehuss added this to the 1.76.0 milestone Dec 6, 2023
epage added a commit to epage/cargo that referenced this pull request Apr 23, 2024
@epage
fix(resolver): Treat unset MSRV as compatible
9515ded 
We last tweaked this logic in rust-lang#13066.
However, we noticed this was inconsistent with `cargo add` in
automatically selecting version requirements.

It looks like this is a revert of rust-lang#13066, taking us back to the behavior
in rust-lang#12950.
In rust-lang#12950 there was a concern about the proliferation of no-MSRV and
whether we should de-prioritize those to make the chance of success more
likely.

There are no right answes here, only which wrong answer is ok enough.
- Do we treat lack of rust version as `rust-version = "*"` as some
  people expect or do we try to be smart?
- If a user adds or removes `rust-version`, how should that affect the
  priority?

One piece of new information is that the RFC for this has us trying to
fill the no-MSRV gap with
`rust-version = some-value-representing-the-current-toolchain>`.
@epage epage mentioned this pull request Apr 23, 2024
Merged
epage added a commit to epage/cargo that referenced this pull request Apr 29, 2024
@epage
fix(resolver): Treat unset MSRV as compatible
89ead6f 
We last tweaked this logic in rust-lang#13066.
However, we noticed this was inconsistent with `cargo add` in
automatically selecting version requirements.

It looks like this is a revert of rust-lang#13066, taking us back to the behavior
in rust-lang#12950.
In rust-lang#12950 there was a concern about the proliferation of no-MSRV and
whether we should de-prioritize those to make the chance of success more
likely.

There are no right answes here, only which wrong answer is ok enough.
- Do we treat lack of rust version as `rust-version = "*"` as some
  people expect or do we try to be smart?
- If a user adds or removes `rust-version`, how should that affect the
  priority?

One piece of new information is that the RFC for this has us trying to
fill the no-MSRV gap with
`rust-version = some-value-representing-the-current-toolchain>`.
bors added a commit that referenced this pull request May 1, 2024
@bors
Auto merge of #13791 - epage:msrv-unset, r=Eh2406
82dca28 
fix(resolver): Treat unset MSRV as compatible

### What does this PR try to resolve?

Have the resolver treat no-MSRV as `rust-version = "*"`, like `cargo add` does for version-requirement selection

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

We last tweaked this logic in #13066.
However, we noticed this was inconsistent with `cargo add` in automatically selecting version requirements.

It looks like this is a revert of #13066, taking us back to the behavior in #12950.
In #12950 there was a concern about the proliferation of no-MSRV and whether we should de-prioritize those to make the chance of success more likely.

There are no right answes here, only which wrong answer is ok enough.
- Do we treat lack of rust version as `rust-version = "*"` as some people expect or do we try to be smart?
- If a user adds or removes `rust-version`, how should that affect the priority?

One piece of new information is that the RFC for this has us trying to fill the no-MSRV gap with
`rust-version = some-value-representing-the-current-toolchain>`.

See also #9930 (comment)

r? `@Eh2406`

### Additional information
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@ehuss ehuss

Labels
A-dependency-resolution Area: dependency resolution and the resolver S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.76.0
Development

Successfully merging this pull request may close these issues.
5 participants
@epage @rustbot @Eh2406 @bors @ehuss