-
Notifications
You must be signed in to change notification settings - Fork 38
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
feat: merge dependencies for better error messages #163
Merged
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
ab71a9f
bad error test
Eh2406 8b4378e
feat: merge direct dependencies
Eh2406 43be97c
separate function for clearer documentation
Eh2406 c646d3c
Update doc comments for merge_dependency
mpizenberg 4c5800e
Rename merge_dependency into merge_dependants
mpizenberg c22f498
Use american english dependent instead of dependant
mpizenberg 8a25f57
Rename dependencies into merged_dependencies
mpizenberg 5c4a6b9
Typo mergeed -> merged
mpizenberg File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 guess you already tested it but I'm gonna ask just in case. Considering we only merge dependants where the dependency is exactly the same, would it make sense to store the dependency in the key? It does increase memory so it's probably worse, but it would prevent from looping through all incompats for the packages pair.
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 suppose these package pairs are generally small anyway since you opted for
SmallVec
so yeah maybe it's a bad trade off. But still curious to hear from if you tried it.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 would absolutely love to do this. Unfortunately
VS
is neitherHash
norOrd
, so I don't know of an efficient data structure for using them as a key.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.
Ah right,
Ord
would not make sense for sets.Hash
could make sense. Maybe we can make a note of this, to explore in the future if it turns out some real-world use case end up being in the worst case scenario for this (many different ranges for a package pair, causing O(n²) complexity).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.
Realistically, this can only happen for a package pair where there are many versions of each, their versions are bumped in sync. Something like two packages from the same workspace maybe if versions are strictly specified. Or for things like features (in our "pubgrub limitations" guide section) where we pair exactly a feature version to the bare crate version. Hum, maybe not so unlikely ...
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.
Yes. "Features" is why none of the cargo benchmarks hit this code path.
I'm not too afraid of the O(n²) overhead here. Every time we merge dependencies we follow it up by doing
unit_propagation
, which does a linear scan over an even larger collection of incompatibilities. (Well...unit_propagation
has "previously_contradicted" as an optimization. So theoretically speakingunit_propagation
is not guaranteed to be slower, but so far it has been in practice.) That is to say it has the same O(n²) shaped, with a larger n. If this becomes a hotspot in a resolution that is slow we can investigate a fixed then.