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

Avoid early error for transistive constraints #1621

Closed
wants to merge 1 commit into from

Conversation

charliermarsh
Copy link
Member

Summary

If a constraint causes some requirement in a dependency to yield the empty set, we should avoid early termination. (I believe we do this right now because the error messages are better.) This is different than if you have (e.g.) two direct requirements with conflicting versions, which should error immediately.

Closes #1522.

@charliermarsh charliermarsh requested a review from zanieb February 18, 2024 02:24

----- stderr -----
× No solution found when resolving dependencies:
╰─▶ you require ∅
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 need to improve this case before merging.

@charliermarsh
Copy link
Member Author

@zanieb - I think this is the thing that would've been solved by that PR you had at one point to allow returning multiple versions (i.e., to avoid this eager merging).

@charliermarsh charliermarsh added the bug Something isn't working label Feb 18, 2024
@zanieb
Copy link
Member

zanieb commented Feb 18, 2024

I'm not sure which one you're referring to anymore :)

@charliermarsh
Copy link
Member Author

@zanieb - Found it! #383

@zanieb
Copy link
Member

zanieb commented Feb 18, 2024

Oh gosh we probably still want that don't we... 😭

@charliermarsh
Copy link
Member Author

Yeah I think it would indirectly solve this problem

@charliermarsh
Copy link
Member Author

(Like in theory it would make this PR redundant and avoid the bad error message I commented on in the PR.)

@zanieb
Copy link
Member

zanieb commented Feb 18, 2024

I think the main blocker there was the URL dependency stuff and I think you had a better solution for that than me? Maybe you should take a look at it?

@charliermarsh
Copy link
Member Author

@zanieb - Yeah I can take this on. I have to prioritize some other things over it first. Do you think this is worth merging as-is even with the bad error message? Defer to you on that.

Copy link
Member

@zanieb zanieb left a comment

Choose a reason for hiding this comment

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

I'll take the incremental improvement.

Is there a test case covering a successful resolution for the bug? It looks like the new test case does not succeed?

@charliermarsh
Copy link
Member Author

Closing in favor of #1796.

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.

handle constraints on indirect dependencies
2 participants