search

whyrusleeping / gx-go

gx subtool for golang
MIT License
80 stars 28 forks source link

fix: don't allow the root package dependencies to be overwritten in the map #47

Closed schomatis closed 6 years ago

schomatis commented 6 years ago 
fix: don't allow the root package dependencies to be overwritten in the 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.

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.)

Stebalien commented 6 years ago

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:

  1. Adding rewrites for all dependencies of the current package in one loop.
  2. Recursing in a second loop.
schomatis commented 6 years ago

Do you mind merging this anyway while we discuss this other solution in the original issue? (The other gx-go link PR I'm working on already depends on this one, and although not optimal it's still a valid solution that would allow me to concentrate on that before revisiting this one.)

Stebalien commented 6 years ago

Fair enough. This fixes the immediate issue.