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

Backtrack on distributions with invalid metadata #2834

Merged
merged 1 commit into from
Apr 5, 2024

Conversation

charliermarsh
Copy link
Member

Summary

Closes #2821.

Comment on lines 347 to 351
UnavailablePackage::Unparseable => {
"was found, but the metadata could not be parsed"
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 This is the kind of thing that would be improved by generic metadata in PubGrub... I started work on it but it's a lot.

@charliermarsh charliermarsh changed the base branch from main to charlie/backtrack April 5, 2024 18:12
@charliermarsh charliermarsh added the bug Something isn't working label Apr 5, 2024
@charliermarsh charliermarsh marked this pull request as ready for review April 5, 2024 18:12
Base automatically changed from charlie/backtrack to main April 5, 2024 18:16
charliermarsh added a commit that referenced this pull request Apr 5, 2024
## Summary

Demonstrates some suboptimal behavior in how we handle invalid metadata,
which are fixed in #2834.

The included wheels were modified by-hand to include invalid structures.
@charliermarsh charliermarsh force-pushed the charlie/filter-metadata branch 2 times, most recently from ca77459 to d3cb9f9 Compare April 5, 2024 18:27

× No solution found when resolving dependencies:
╰─▶ Because validation==2.0.0 is unusable because its dependencies are
unusable because the package metadata could not be parsed and you
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zanieb - Is there any way for me to improve this (the triple "because") without larger refactors?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can I make Dependencies::Unavailable accept an enum or something?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you could switch it from a string to an enum but uhh I'm not really sure. This is the idea behind a generic incompatibility metadata type in PubGrub.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I can do it, do you mind if I do it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No I don't mind it seems like a step in the right direction

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add separately.

@@ -1100,8 +1100,7 @@ fn mismatched_name() -> Result<()> {
----- stdout -----

----- stderr -----
error: Failed to read: foo @ file://[TEMP_DIR]/foo-2.0.1-py3-none-any.whl
Caused by: The .dist-info directory tomli-2.0.1 does not start with the normalized package name: foo
error: Because foo was found, but has an invalid format and you require foo, we can conclude that the requirements are unsatisfiable.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is slightly worse... I guess I could make the "reasons" more specific?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe the .dist-info thing is so broken that we shouldn't backtrack. IDK.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm it's a little weird to throw the no solution error this way (i.e. compared to how it's formatted elsewhere)

You could push the detailed reasons into some hints?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm it's a little weird to throw the no solution error this way (i.e. compared to how it's formatted elsewhere)

What do you mean by this?

Copy link
Member

@zanieb zanieb Apr 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

error: Because foo was found, ...

vs

× No solution found when resolving dependencies:
╰─▶ Because you require ...

I don't mind the lack of the arrow and such but the "No solution found" part seems relevant.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yeah. I'll add separately.

@charliermarsh charliermarsh merged commit 0093404 into main Apr 5, 2024
35 checks passed
@charliermarsh charliermarsh deleted the charlie/filter-metadata branch April 5, 2024 22:00
@charliermarsh
Copy link
Member Author

Filed issues for the follow-ups.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider dropping candidates that have parse errors for resolution instead of failing the installation
2 participants