bazeltools / bazel-deps

Generate bazel dependencies for maven artifacts
MIT License
250 stars 121 forks source link

aether vs. coursier discrepancies #161

Open roblg opened 6 years ago

roblg commented 6 years ago

I found a simple example of a pretty large discrepancy between the artifacts discovered by coursier vs. aether resolverTypes, where I think aether actually gets it right(er). I've got a complete example with outputs in a gist

The story revolves around com.sun.xml.ws:jaxws-rt:2.2.10. If you follow the trail of maven ancestor poms high enough, you'll eventually find exclusions of several dependencies. For example (full pom):

Looking at the coursier and aether outputs in the gist you can see that those artifacts aren't included in aether, but are in coursier (the exception is that istack-commons-runtime is also included in aether, which based on a comment in the pom I think was intentional -- it's allowing a different dep path to determine the version. Although istack-commons-runtime actually doesn't show up in our maven-only dep graph at all, so maybe there's a bug in aether there too, or maybe we have an exclusion somewhere else that's preventing it. I haven't dug in far enough to know for sure).

In this specific case, I think I can use bazel-deps excludes w/ coursier resolverType to achieve the correct(ish) effect, but I'm not sure if this is something that coursier is doing, or if there's code in bazel-deps' coursier usage that could be causing this discrepancy. In a scenario where we have other things that depend on the same things that jaxws-rt depends on, I can understand the dep graph not matching exactly, but it seems like for the single-dependency case, it should?

LMK if there's more information I can provide.

Thanks!

johnynek commented 6 years ago

It would be interesting to run this down. If you see an error, maybe we can find a way to show it in the coursier command line tool and report the error there.

If it is only us seeing it, that would be good to know that it is internal to our code.

We may be ignoring exclusions in our code here: https://github.com/johnynek/bazel-deps/blob/master/src/scala/com/github/johnynek/bazel_deps/CoursierResolver.scala#L197

maybe we need to consult the exclusions from Coursier: https://github.com/coursier/coursier/blob/master/core/shared/src/main/scala/coursier/core/Definitions.scala#L50

If we look at the exclusions, and not walk those edges, that might be correct.

Are you interested in trying to write a failing test that we see this exclusion, then see if we can fix the test by consulting the exclusion in the Dependency?

roblg commented 6 years ago

Are you interested in trying to write a failing test that we see this exclusion, then see if we can fix the test by consulting the exclusion in the Dependency?

Theoretically yes, but I don't know if I'm going to have time the next few weeks. If somebody else wants to take a swing at it, I'd say go for it. :)