whyrusleeping / gx-go

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

Handle rewrite map collisions #44

Closed schomatis closed 6 years ago

schomatis commented 6 years ago

When building the rewrite map (buildRewriteMapping) new entries for the same DvcsImport will overwrite the old ones, so if a package.json explicitly declares a dependency A with version 2.0 but another dependency B (processed later) also has a dependency to A but with an older 1.0 version, this older version will get rewritten, the root package will be using the old version even though it explicitly declared the new one.

A first approach would be to prioritize dependencies in the package.json (over indirect dependencies), but I also have the doubt of why the map needs to go through the entire dependency tree to rewrite just the root package (I'm thinking of the gx install scenario, where the rewrite -post-install hook- will be called explicitly on every package separately).

schomatis commented 6 years ago

I also have the doubt of why the map needs to go through the entire dependency tree to rewrite just the root package

Because packages like go-blockservice import dependencies (e.g., go-cid) without declaring it in their package.json. Is that standard practice?

Stebalien commented 6 years ago

See https://github.com/whyrusleeping/gx-go/pull/38#issuecomment-396455663 and the related discussion.

schomatis commented 6 years ago

Thanks for the reference. Performance issues aside, I think we should explicitly declare all the imports in package.json for pretty much all the reasons discussed there.

If that can't be fixed at the moment I still think buildRewriteMapping should give priority to the dependencies explicitly declared in package.json and don't overwrite them with transitive dependencies.

And as an extra, we could add a warning in gx-go install if we're rewriting a dependency that was not declared in package.json and was taking it from a transitive dependency (the downside is that I think gx install won't show it).