-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
revert #5770, provide new fix #6058
Conversation
dbfeb4c
to
722fe6a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that resolving direct origin dependencies first seems to be sensible. I just noticed a minor inconsistency (see comment below).
Co-authored-by: Randy Döring <[email protected]>
I just added some dedicated tests for |
Co-authored-by: Randy Döring <[email protected]>
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Pull Request Check List
Resolves: #6054
I think that the fix made at #5770 was not good. (I did review it at the time without seeing problems, so no blame!)
As I now understand it, the problem that it was trying to solve was:
foo
foo[extras]
foo
-related assignments find the direct-origin package#5770 fixed this by finding the
foo
assignment at the second bullet, and then the intersection of a direct-origin and not-direct-origin term always favours the direct-origin.I think this is probably unreliable: eg in the first bullet we might have registered a direct-origin assignment for
foo[other-extras]
. Or bullets 1 and 2 might happen the other way round.Apart from that, #6054 exposed a problem where we registered regular assignments for
foo
andfoo[extras]
, but with incompatible version ranges. Before #5770 that was fine and is unwound later in the search, but after #5770 we try to update thefoo
assignment and go wrong.This MR undoes the fix of #5770 and tries another. I have arranged that:
That fixes the original #5311 because now the direct-origin version of the
foo
package is the only one that we ever see; and, per the new testcase, unbreaks #6054