sbt / sbt-pom-reader

Translates xml -> awesome. Maven-ish support for sbt.
Other
76 stars 27 forks source link

Bumping to mvn 3.5.2 and wagon 2.12 #47

Closed blast-hardcheese closed 6 years ago

blast-hardcheese commented 6 years ago

This seems to work, although I'm relying heavily on the scripted tests being comprehensive, as well as the local tests I've done with a few repos so far.

Despite the warnings from the initial sbt 1.x upgrade, sbt-pom-reader seems pretty happy in the handful of repos I'm using it in. The motivation behind this PR is to bring the dependencies in line with https://github.com/arktekk/sbt-aether-deploy, which now seems to be accomplished (🎉)

Unfortunately, I was unable to eradicate all of the warnings:

[warn] Found version conflict(s) in library dependencies; some are suspected to be binary incompatible:
[warn]  * com.google.guava:guava:20.0 is selected over 16.0.1
[warn]      +- io.swagger:swagger-core:1.5.16                     (depends on 20.0)
[warn]      +- org.apache.maven:maven-model-builder:3.5.2         (depends on 20.0)
[warn]      +- org.apache.maven:maven-core:3.5.2                  (depends on 20.0)
[warn]      +- org.apache.maven:maven-embedder:3.5.2              (depends on 20.0)
[warn]      +- org.apache.maven:maven-resolver-provider:3.5.2     (depends on 20.0)
[warn]      +- com.google.inject:guice:4.0                        (depends on 16.0.1)
[warn]  * org.codehaus.plexus:plexus-utils:3.1.0 is selected over {3.0.24, 3.0.17, 1.5.5}
[warn]      +- org.apache.maven:maven-repository-metadata:3.5.2   (depends on 3.1.0)
[warn]      +- org.apache.maven:maven-model:3.5.2                 (depends on 3.1.0)
[warn]      +- org.apache.maven:maven-model-builder:3.5.2         (depends on 3.1.0)
[warn]      +- org.apache.maven:maven-core:3.5.2                  (depends on 3.1.0)
[warn]      +- org.apache.maven:maven-resolver-provider:3.5.2     (depends on 3.1.0)
[warn]      +- org.apache.maven:maven-settings:3.5.2              (depends on 3.1.0)
[warn]      +- org.apache.maven:maven-artifact:3.5.2              (depends on 3.1.0)
[warn]      +- org.apache.maven:maven-embedder:3.5.2              (depends on 3.1.0)
[warn]      +- org.apache.maven:maven-plugin-api:3.5.2            (depends on 3.1.0)
[warn]      +- org.apache.maven:maven-settings-builder:3.5.2      (depends on 3.1.0)
[warn]      +- org.sonatype.plexus:plexus-sec-dispatcher:1.4      (depends on 1.5.5)
[warn]      +- org.eclipse.sisu:org.eclipse.sisu.plexus:0.3.3     (depends on 3.0.17)
[warn]      +- org.apache.maven.wagon:wagon-provider-api:2.12     (depends on 3.0.24)
[warn]      +- org.apache.maven.wagon:wagon-http-lightweight:2.12 (depends on 3.0.24)
[warn]      +- org.apache.maven.wagon:wagon-file:2.12             (depends on 3.0.24)
[warn] Run 'evicted' to see detailed eviction warnings
[info] Loading settings from build.sbt ...

This seems to be outside our control, however, as sbt-dependency-graph shows:

[info]     +-org.apache.maven:maven-core:3.5.2
[info]     | +-com.google.guava:guava:20.0
[info]     | +-com.google.inject:guice:4.0
[info]     | | +-aopalliance:aopalliance:1.0
[info]     | | +-com.google.guava:guava:16.0.1 (evicted by: 20.0)
[info]     | | +-com.google.guava:guava:20.0
[info]     | | +-javax.inject:javax.inject:1

Short of attempting to manually resolve these direct conflicts to reduce noise for the user, I'm comfortable with where things stand now.

Thanks again for managing the publishing of this plugin, hopefully this gets us all out of the woods until the next major change.

blast-hardcheese commented 6 years ago

Also, http://maven.40175.n5.nabble.com/Formerly-Aether-now-Maven-Artifact-Resolver-documentation-td5900927.html is what got me on the right path to finding the new homes for these libraries -- all the previous homes 302 or 404.

jvican commented 6 years ago

This is a valuable contributions, thank you. You've done some of the stuff I've been doing over the past few weeks. Could you please compare this PR with it? https://github.com/scalacenter/sbt-pom-reader/commits/master

I notice that there are a few things that should be removed. The Wagon transport hack is one of them (MyModelResolver). I also think you need to add the following https://github.com/sbt/sbt-pom-reader/compare/master...scalacenter:master#diff-e6b5dc2306de10d20247d876cf506bf2R18, which is required by default IIRC.

blast-hardcheese commented 6 years ago

@jvican I can pull those changes in, and thanks for verifying the wagon stuff is irrelevant -- I took it out of the resolver and all the tests still ran, but I still figured it was useful for existing workflows. I'll update the PR later tonight. Thanks again for the review!

blast-hardcheese commented 6 years ago

@jvican Our branches look nearly identical, save one change in https://github.com/scalacenter/sbt-pom-reader/blob/master/src/main/scala/ch/epfl/scala/sbt/pom/MavenModelResolver.scala#L54, where you append the new repository -- I took replace to mean https://github.com/sbt/sbt-pom-reader/pull/47/files#diff-7b12b421c41385c28a91c420c9c26ce0R68.

This was really only a few hours of investigation and work, so I'm fine if you want to take over from here.

blast-hardcheese commented 6 years ago

Also, as for

I notice that there are a few things that should be removed. The Wagon transport hack is one of them (MyModelResolver). I also think you need to add the following master...scalacenter:masterdiff-e6b5dc2306de10d20247d876cf506bf2R18, which is required by default IIRC.

that diff link seems broken, so I'm missing some context there

blast-hardcheese commented 6 years ago

Great! With that, we're only down to two direct deps!

blast-hardcheese commented 6 years ago

One more stab in (hopefully) the right direction. Manually resolving dependency conflicts from upstream, as well as adding the sbt-dependency-graph plugin so we can actually see the conflicts.

blast-hardcheese commented 6 years ago

@jvican Any further thoughts here, or would you like to merge what's here and rebase your organizational changes going forward? I'm using a locally published version of the plugin, but it would be nice to use one provided by bintray

jvican commented 6 years ago

Sorry for my delay @blast-hardcheese 😞 Let's merge!

jvican commented 6 years ago

And thank you for this awesome work.

blast-hardcheese commented 6 years ago

@jvican Thank you! I'm glad to see this plugin having some interest from EPFL 😄

One other thought, I'm currently doing something similar to how Spark uses the pom reader, by applying a regex to the pom file between builds to support multiple scala versions.

Based on my understanding of the design of maven, this is a design limitation that would only be able to be worked around by convention, instead of using infrastructure provided in the POM itself. I'd be interested in any work that has been done initially to attempt to rein this challenge.

This is by no means a blocker, just interested in moving towards a better integration.

Thank you for your effort!

jvican commented 6 years ago

Based on my understanding of the design of maven, this is a design limitation that would only be able to be worked around by convention, instead of using infrastructure provided in the POM itself. I'd be interested in any work that has been done initially to attempt to rein this challenge.

Unfortunately there's no effort in this area. We envision a better Maven integration though, and our solution to it is bloop; it already has a Maven plugin that reads information from the conventional Scala plugin to allow for a faster development workflow. Our goal is to support all Scala compilers and backends (Scalajs, Scala Native, etc).

blast-hardcheese commented 6 years ago

Understood, thank you for the insight!