New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bumping to mvn 3.5.2 and wagon 2.12 #47

Merged
merged 5 commits into from Jan 17, 2018

Conversation

Projects
None yet
2 participants
@blast-hardcheese
Collaborator

blast-hardcheese commented Jan 12, 2018

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

This comment has been minimized.

Show comment
Hide comment
@blast-hardcheese

blast-hardcheese Jan 12, 2018

Collaborator

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.

Collaborator

blast-hardcheese commented Jan 12, 2018

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

This comment has been minimized.

Show comment
Hide comment
@jvican

jvican Jan 12, 2018

Member

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 master...scalacenter:masterdiff-e6b5dc2306de10d20247d876cf506bf2R18, which is required by default IIRC.

Member

jvican commented Jan 12, 2018

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 master...scalacenter:masterdiff-e6b5dc2306de10d20247d876cf506bf2R18, which is required by default IIRC.

@blast-hardcheese

This comment has been minimized.

Show comment
Hide comment
@blast-hardcheese

blast-hardcheese Jan 12, 2018

Collaborator

@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!

Collaborator

blast-hardcheese commented Jan 12, 2018

@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

This comment has been minimized.

Show comment
Hide comment
@blast-hardcheese

blast-hardcheese Jan 12, 2018

Collaborator

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

Collaborator

blast-hardcheese commented Jan 12, 2018

@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

This comment has been minimized.

Show comment
Hide comment
@blast-hardcheese

blast-hardcheese Jan 12, 2018

Collaborator

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

Collaborator

blast-hardcheese commented Jan 12, 2018

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

This comment has been minimized.

Show comment
Hide comment
@blast-hardcheese

blast-hardcheese Jan 13, 2018

Collaborator

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

Collaborator

blast-hardcheese commented Jan 13, 2018

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

@blast-hardcheese

This comment has been minimized.

Show comment
Hide comment
@blast-hardcheese

blast-hardcheese Jan 14, 2018

Collaborator

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.

Collaborator

blast-hardcheese commented Jan 14, 2018

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

This comment has been minimized.

Show comment
Hide comment
@blast-hardcheese

blast-hardcheese Jan 17, 2018

Collaborator

@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

Collaborator

blast-hardcheese commented Jan 17, 2018

@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

This comment has been minimized.

Show comment
Hide comment
@jvican

jvican Jan 17, 2018

Member

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

Member

jvican commented Jan 17, 2018

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

@jvican

This comment has been minimized.

Show comment
Hide comment
@jvican

jvican Jan 17, 2018

Member

And thank you for this awesome work.

Member

jvican commented Jan 17, 2018

And thank you for this awesome work.

@jvican

jvican approved these changes Jan 17, 2018

@jvican jvican merged commit 77ee4e1 into sbt:master Jan 17, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@blast-hardcheese blast-hardcheese deleted the blast-hardcheese:upgrading-maven branch Jan 17, 2018

@blast-hardcheese

This comment has been minimized.

Show comment
Hide comment
@blast-hardcheese

blast-hardcheese Jan 17, 2018

Collaborator

@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!

Collaborator

blast-hardcheese commented Jan 17, 2018

@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

This comment has been minimized.

Show comment
Hide comment
@jvican

jvican Jan 22, 2018

Member

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

Member

jvican commented Jan 22, 2018

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

This comment has been minimized.

Show comment
Hide comment
@blast-hardcheese

blast-hardcheese Jan 25, 2018

Collaborator

Understood, thank you for the insight!

Collaborator

blast-hardcheese commented Jan 25, 2018

Understood, thank you for the insight!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment