fix(resolver): Report unmatched versions, rather than saying no package #14897

epage · 2024-12-05T18:00:58Z

What does this PR try to resolve?

Instead of saying no package found when there are hidden Summarys, we'll instead say why the summary was hidden in the cases of

Yanked packages
Schema mismatch
Offline packages?

The schema mismatch covers part of #10623. Whats remaining is when we can't parse the Summary but can parse a subset (name, version, schema version, optionally rust-version). That will be handled in a follow up.

How should we test and review this PR?

This has a couple of risky areas

Moving the filtering of IndexSummary variants from the Index to the Registry Source. On inspection, there were other code paths but they seemed innocuous to not filter, like asking for a hash of a summary
Switching PackageRegistry to preserve the IndexSummary variant for regular sources and overrides
- I did not switch patches to preserve IndexSummary as that was more invasive and the benefits seemed more minor (normally people patch over registry dependencies and not to them)

Additional information

rustbot · 2024-12-05T18:01:03Z

r? @ehuss

rustbot has assigned @ehuss.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

…ySource

Eh2406 · 2024-12-07T03:48:08Z

I dropped this in a half done state at some point. Thank you for pushing this forward.

Rustin170506

Just asked some questions that I didn't understand.

Thank you!

Rustin170506 · 2024-12-08T13:41:05Z

src/cargo/core/resolver/errors.rs

@@ -362,12 +362,23 @@ pub(super) fn activation_error(
                        let _ = writeln!(&mut msg, "  version {} is not cached", summary.version());
                    }
                    IndexSummary::Unsupported(summary, schema_version) => {
-                        let _ = writeln!(
+                        if let Some(rust_version) = summary.rust_version() {
+                            // HACK: technically its unsupported and we shouldn't make assumptions


What do you mean by technically its unsupported?

For now, the Unsupported branch means that we can parse the entry but the schema version was bumped. How we interpret the fields could have changed.

It'll be even worse in a follow up where I'll add "failed to parse but could get basic data".

Got it. Thank you!

Rustin170506 · 2024-12-08T13:43:35Z

src/cargo/core/resolver/errors.rs

+            for candidate in version_candidates {
+                match candidate {
+                    IndexSummary::Candidate(summary) => {
+                        // HACK: If this was a real candidate, we wouldn't hit this case.


Not sure how this happens. It it possible to add a unit test to cover this?

I don't know how to hit this. This was originally an assert but I hit it because we switched everything to Candidate. That should be fixed but I figured a vague error is better for users than a crash in case I missed something or things change in the future.

Rustin170506 · 2024-12-10T14:22:52Z

@epage There is a typo in your last commit message. ; -> :

Muscraft

Thanks for this! The error messages are so much better!

weihanglo

Did a late review. Brilliant! I love the new AlternativeVersions query kind.

### What does this PR try to resolve? When I copy/pasted the alternative-names code in #14897, I didn't notice that the wildcard dependency was used. This includes a rename of the variable to make it stand out more. I also renamed `AlternativeVersions` because there is another check we do that better matches that name, so I renamed it to `RejectedVersions`. ### How should we test and review this PR? This does not have a test yet because overly-constrictive dep specs have higher precedence atm (which is what I was working on when I found this). ### Additional information

…14923) ### What does this PR try to resolve? The user likely intended what they said and we should tell them why it didn't work. Rejected versions also imply alt versions below them. Fixes #4260 In changing the errors from alt-versions to rejected-versions, I found part of the old error message was better and changed the message introduced in #14897 from matching alt-names to alt-versions. This includes the test for #14921. The "fix" was done before this because as it was part of the refactors in prep for this. ### How should we test and review this PR? ### Additional information Next steps after this are to still produce an `IndexSummary::Unsupported` even if there are deserialization errors, so long as we know the name and version which should take care of #10623 and #14894.

While rust-lang#14897 reported packages with an unsupported index schema version, that only worked if the changes in the schema version did not cause errors in deserializing `IndexPackage` or in generating a `Summary`. This extends that change by recoverying on error with a more lax, incomplete parse of `IndexPackage` which should always generate a valid `Summary`. To help with a buggy Index, we also will report as many as we can. This does not provide a way to report to users or log on cache reads if the index entry is not at least `{"name": "<string>", "vers": "<semver>"}`. As a side effect, the index cache will include more "invalid" index entries. That should be ok as we will ignore the invalid entry in the cache when loading it. Ignoring of invalid entries dates back to rust-lang#6880 when the index cache was introduced. Fixes rust-lang#10623 Fixes rust-lang#14894

### What does this PR try to resolve? While #14897 reported packages with an unsupported index schema version, that only worked if the changes in the schema version did not cause errors in deserializing `IndexPackage` or in generating a `Summary`. This extends that change by recoverying on error with a more lax, incomplete parse of `IndexPackage` which should always generate a valid `Summary`. To help with a buggy Index, we also will report as many as we can. This does not provide a way to report to users or log on cache reads if the index entry is not at least `{"name": "<string>", "vers": "<semver>"}`. Fixes #10623 Fixes #14894 ### How should we test and review this PR? My biggest paranoia is some bad interaction with the index cache including more "invalid" index entries. That should be ok as we will ignore the invalid entry in the cache when loading it. Ignoring of invalid entries dates back to #6880 when the index cache was introduced. ### Additional information

Update cargo 18 commits in 20a443231846b81c7b909691ec3f15eb173f2b18..7847c03965260b5dcc8d93218d6af295a717abb6 2024-12-06 21:56:56 +0000 to 2024-12-13 18:06:39 +0000 - fix(base): Support bases in patches in virtual manifests (rust-lang/cargo#14931) - fix(resolver): Report invalid index entries (rust-lang/cargo#14927) - feat: Implement `--depth workspace` for `cargo tree` command (rust-lang/cargo#14928) - fix(resolver): In errors, show rejected versions over alt versions (rust-lang/cargo#14923) - fix: emit_serialized_unit_graph uses the configured shell (rust-lang/cargo#14926) - fix(script): Don't override the release profile (rust-lang/cargo#14925) - feature(SourceId): use stable hash from rustc-stable-hash (rust-lang/cargo#14917) - fix(resolver): Don't report all versions as rejected (rust-lang/cargo#14921) - fix(resolver): Report unmatched versions, rather than saying no package (rust-lang/cargo#14897) - fix(build-rs): Implicitly report rerun-if-env-changed for input (rust-lang/cargo#14911) - a faster hash for ActivationsKey (rust-lang/cargo#14915) - feat(build-script): Pass CARGO_CFG_FEATURE (rust-lang/cargo#14902) - fix(build-rs): Correctly refer to the item in assert (rust-lang/cargo#14913) - chore: update auto-label to include build-rs crate (rust-lang/cargo#14912) - refactor: use Path::push to construct remap-path-prefix (rust-lang/cargo#14908) - feat(build-rs): Add the 'error' directive (rust-lang/cargo#14910) - fix(build-std): determine root crates by target spec `std:bool` (rust-lang/cargo#14899) - SemVer: Add section on RPIT capturing (rust-lang/cargo#14849)

Update cargo 19 commits in 20a443231846b81c7b909691ec3f15eb173f2b18..769f622e12db0001431d8ae36d1093fb8727c5d9 2024-12-06 21:56:56 +0000 to 2024-12-14 04:27:35 +0000 - test(build-std): dont require rustup (rust-lang/cargo#14933) - fix(base): Support bases in patches in virtual manifests (rust-lang/cargo#14931) - fix(resolver): Report invalid index entries (rust-lang/cargo#14927) - feat: Implement `--depth workspace` for `cargo tree` command (rust-lang/cargo#14928) - fix(resolver): In errors, show rejected versions over alt versions (rust-lang/cargo#14923) - fix: emit_serialized_unit_graph uses the configured shell (rust-lang/cargo#14926) - fix(script): Don't override the release profile (rust-lang/cargo#14925) - feature(SourceId): use stable hash from rustc-stable-hash (rust-lang/cargo#14917) - fix(resolver): Don't report all versions as rejected (rust-lang/cargo#14921) - fix(resolver): Report unmatched versions, rather than saying no package (rust-lang/cargo#14897) - fix(build-rs): Implicitly report rerun-if-env-changed for input (rust-lang/cargo#14911) - a faster hash for ActivationsKey (rust-lang/cargo#14915) - feat(build-script): Pass CARGO_CFG_FEATURE (rust-lang/cargo#14902) - fix(build-rs): Correctly refer to the item in assert (rust-lang/cargo#14913) - chore: update auto-label to include build-rs crate (rust-lang/cargo#14912) - refactor: use Path::push to construct remap-path-prefix (rust-lang/cargo#14908) - feat(build-rs): Add the 'error' directive (rust-lang/cargo#14910) - fix(build-std): determine root crates by target spec `std:bool` (rust-lang/cargo#14899) - SemVer: Add section on RPIT capturing (rust-lang/cargo#14849)

rustbot assigned ehuss Dec 5, 2024

rustbot added A-dependency-resolution Area: dependency resolution and the resolver A-directory-source Area: directory sources (vendoring) A-registries Area: registries S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 5, 2024

epage added 11 commits December 5, 2024 12:55

refactor(source): Qualify what alternatives we look for

4d7913b

refactor(resolver): Qualify what kind of alt candidates we're discussing

31a884e

refactor(resolver): Group related alternatives logic

eb0daec

refactor(resolver): Make no alternatives the fall-through

efe17de

refactor(resolver): Switch some code from string construction to writing

271df34

refactor(registry): Move IndexSummary filtering from Index to Registr…

0160cb7

…ySource

refactor(registry): Pass along original IndexSummary state for deps

e48a008

refactor(registry): Pass along original IndexSummary state for overrides

015e980

fix(resolver): Report unmatched versions, rather than saying no package

d2fcf6b

test(resolver): Show error with MSRV set

06b5ec5

fix(resolver): Prefer rust-version over schema version

0ed5c21

epage force-pushed the alt branch 5 times, most recently from bfec08d to dd9abed Compare December 6, 2024 19:16

Rustin170506 reviewed Dec 8, 2024

View reviewed changes

epage force-pushed the alt branch 2 times, most recently from 1cbf2b0 to 910f17f Compare December 9, 2024 18:39

epage force-pushed the alt branch 2 times, most recently from 7627738 to 0ed5c21 Compare December 10, 2024 22:00

Muscraft approved these changes Dec 11, 2024

View reviewed changes

Muscraft added this pull request to the merge queue Dec 11, 2024

Merged via the queue into rust-lang:master with commit ad5b493 Dec 11, 2024
20 checks passed

epage deleted the alt branch December 11, 2024 16:35

weihanglo reviewed Dec 11, 2024

View reviewed changes

epage mentioned this pull request Dec 11, 2024

fix(resolver): Don't report all versions as rejected #14921

Merged

epage mentioned this pull request Dec 11, 2024

fix(resolver): In errors, show rejected versions over alt versions #14923

Merged

epage mentioned this pull request Dec 12, 2024

fix(resolver): Report invalid index entries #14927

Merged

weihanglo mentioned this pull request Dec 13, 2024

Update cargo rust-lang/rust#134278

Merged

rustbot added this to the 1.85.0 milestone Dec 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(resolver): Report unmatched versions, rather than saying no package #14897

fix(resolver): Report unmatched versions, rather than saying no package #14897

epage commented Dec 5, 2024

rustbot commented Dec 5, 2024

Eh2406 commented Dec 7, 2024

Rustin170506 left a comment

Rustin170506 Dec 8, 2024

epage Dec 9, 2024

Rustin170506 Dec 10, 2024

Rustin170506 Dec 8, 2024

epage Dec 9, 2024

Rustin170506 commented Dec 10, 2024

Muscraft left a comment

weihanglo left a comment

fix(resolver): Report unmatched versions, rather than saying no package #14897

fix(resolver): Report unmatched versions, rather than saying no package #14897

Conversation

epage commented Dec 5, 2024

What does this PR try to resolve?

How should we test and review this PR?

Additional information

rustbot commented Dec 5, 2024

Eh2406 commented Dec 7, 2024

Rustin170506 left a comment

Choose a reason for hiding this comment

Rustin170506 Dec 8, 2024

Choose a reason for hiding this comment

epage Dec 9, 2024

Choose a reason for hiding this comment

Rustin170506 Dec 10, 2024

Choose a reason for hiding this comment

Rustin170506 Dec 8, 2024

Choose a reason for hiding this comment

epage Dec 9, 2024

Choose a reason for hiding this comment

Rustin170506 commented Dec 10, 2024

Muscraft left a comment

Choose a reason for hiding this comment

weihanglo left a comment

Choose a reason for hiding this comment