fix: don't allow the root package dependencies to be overwritten in the map #47
Conversation
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
schomatis
commented
Aug 22, 2018
main.go
Outdated
| m[from] = to | ||
| } else if entryExists && m[from] != to { | ||
| VLog("trying to overwrite rewrite map entry %s poiting to %s with %s", from, m[from], to) |
There was a problem hiding this comment.
Fix typo: "poiting".
…he map The rewrite map is built recursively scanning the root package dependencies. If two packages in the tree imported the same dependency with different hash the last one to be processed would overwrite the first one. Include a flag in `buildRewriteMapping` to signal when the root package dependencies are being process and allow them to overwrite the map (adding an overwrite flag in `addRewriteForDep`) but don't allow the inverse (transitive dependencies overwriting direct ones). This change has as a side effect that in a scenario where transitive dependencies with different versions were overwriting each other that won't be allowed now, changing in fact the rewrite map, but that would happen only in the already problematic scenario of dependencies with mismatched versions.
Stebalien
previously approved these changes
Aug 27, 2018
Stebalien
dismissed
their stale review
Actually, I'm not sure if this is the right fix.
|
So, this is a fix but, after thinking about this, I think the better fix is to change the traversal order to breadth first (and ban all overwriting). This catches the case where the root package overrides a dependency version but doesn't apply the same logic recursively. How about:
|
|
Do you mind merging this anyway while we discuss this other solution in the original issue? (The other |
Stebalien
approved these changes
Aug 28, 2018
|
Fair enough. This fixes the immediate issue. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes #44.
Tested locally, it feels that it should have a test but I don't see any testing framework here (it would be nice to have it to easily create dependencies trees and test different scenarios, I'll open an issue about it.)