Skip to content
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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Coursier dependency resolution integration #4614

Merged
merged 22 commits into from Apr 27, 2019

Conversation

@eed3si9n
Copy link
Member

eed3si9n commented Apr 17, 2019

Fixes #2997

This adopts Coursier-backed LM API implementation (lm-coursier-shaded).

As it stands, this uses Coursier by default to resolve both the metabuild and the proper build.

ThisBuild / useCoursier := false or -Dsbt.coursier=false can be used to opt-out of using Coursier for the dependency resolution.

credits

The Coursier-backed LM API implementation was implemented by:

@@ -115,6 +100,9 @@ object Dependencies {
def addSbtZincCompile(p: Project): Project =
addSbtModule(p, sbtZincPath, "zincCompile", zincCompile)

val lmCousierVersion = "1.1.0-M14"

This comment has been minimized.

Copy link
@eed3si9n

eed3si9n Apr 17, 2019

Author Member

Note that I am adding dependency to lm-coursier 1.1.0-M14.

@eed3si9n

This comment has been minimized.

Copy link
Member Author

eed3si9n commented Apr 17, 2019

Here are the failing tests. Some of these are testing Ivy specific settings, we can fix them by flipping back to Ivy.

  • actions/all
  • actions/external-doc
  • dependency-management/compiler-bridge-binary
  • dependency-management/configurations-to-retrieve
  • dependency-management/artifact
  • dependency-management/cache-classifiers
  • dependency-management/cache-local
  • dependency-management/cache-resolver
  • dependency-management/cached-resolution-circular
  • dependency-management/cached-resolution-classifier
  • dependency-management/cached-resolution-conflicts
  • dependency-management/cached-resolution-exclude
  • dependency-management/circular-dependency
  • dependency-management/conflict-manager
  • dependency-management/default-artifact
  • dependency-management/dynamic-revision
  • dependency-management/ext-pom-classifier
  • dependency-management/force-update-period
  • dependency-management/deliver-artifacts
  • dependency-management/extra
  • dependency-management/info
  • dependency-management/ivy-settings-a
  • dependency-management/ivy-settings-b
  • dependency-management/ivy-settings-multi-a
  • dependency-management/pom-classpaths
  • dependency-management/ivy-settings-c
  • dependency-management/latest-local-plugin
  • dependency-management/metadata-only-resolver
  • dependency-management/retrieve-managed-sync
  • dependency-management/scala-organization
  • dependency-management/sources
  • dependency-management/pom-parent-pom
  • dependency-management/snapshot-resolution
  • dependency-management/test-artifact
  • dependency-management/update-sbt-classifiers
  • dependency-management/url
  • project/cross-plugins-defaults
  • project/default-auto-plugins
  • project/extra
  • project/update-classifiers
  • watch/dynamic-inputs
  • watch/task
  • watch/input-parser
  • watch/on-termination
  • watch/input-aggregation
  • watch/custom-config
  • classloader-cache/snapshot
  • repoOverrideTest:scripted dependency-management/default-resolvers
@eed3si9n

This comment has been minimized.

Copy link
Member Author

eed3si9n commented Apr 17, 2019

#4459 fixes actions/all

@eed3si9n

This comment has been minimized.

Copy link
Member Author

eed3si9n commented Apr 17, 2019

Reported actions/external-doc failure as coursier/coursier#1123

@eed3si9n

This comment has been minimized.

Copy link
Member Author

eed3si9n commented Apr 18, 2019

dependency-management/configurations-to-retrieve is Ivy feature.
dependency-management/artifact is border-line Ivy feature, meaning it could be called a bug.

dependency-management/cache-local is failing because it's not respecting the customized Ivy home?

[info] [error] (update) sbt.librarymanagement.ResolveException: Error downloading org.example:def_2.12:1.0
[info] [error]   Not found
[info] [error]   not found: /Users/eed3si9n/.ivy2/local/org.example/def_2.12/1.0/ivys/ivy.xml
[info] [error]   not found: https://repo1.maven.org/maven2/org/example/def_2.12/1.0/def_2.12-1.0.pom

Reported that as coursier/coursier#1124.

dependency-management/cached-resolution-classifier is failing because it's missing commons-io-1.4-sources even though the subproject is depending on "commons-io" % "commons-io" % "1.4" classifier "sources".

dependency-management/cached-resolution-exclude is Ivy feature.

dependency-management/circular-dependency is also Ivy feature, sort of. Ivy is able to throw an exception given a circular dependency.

[info] [error] org.example#b_2.10;1.0-SNAPSHOT->org.example#c_2.10;1.0-SNAPSHOT->...
[info] [error] org.apache.ivy.plugins.circular.CircularDependencyException: org.example#b_2.10;1.0-SNAPSHOT->org.example#c_2.10;1.0-SNAPSHOT->...
[info] [error] 	at org.apache.ivy.plugins.circular.ErrorCircularDependencyStrategy.handleCircularDependency(ErrorCircularDependencyStrategy.java:37)

Coursier just publishLocals without an error. Reported as coursier/coursier#1125.

dependency-management/default-artifact looks like a bug.

[info] [error] coursier.error.FetchError$DownloadingArtifacts: Error fetching artifacts:
[info] [error] file:///private/var/folders/hg/2602nfrs2958vnshglyl3srw0000gn/T/sbt_1fe50cd/repo/a/b/1.0.0/b1.jar.jar: not found: /private/var/folders/hg/2602nfrs2958vnshglyl3srw0000gn/T/sbt_1fe50cd/repo/a/b/1.0.0/b1.jar.jar
[info] [error] file:///private/var/folders/hg/2602nfrs2958vnshglyl3srw0000gn/T/sbt_1fe50cd/repo/a/b/1.0.0/b2.jar.jar: not found: /private/var/folders/hg/2602nfrs2958vnshglyl3srw0000gn/T/sbt_1fe50cd/repo/a/b/1.0.0/b2.jar.jar

Note b2.jar.jar.

dependency-management/dynamic-revision is a weird one. POM says:

        <dependency>
            <groupId>org.scala-stm</groupId>
            <artifactId>scala-stm_2.10.0</artifactId>
            <version>0.6</version>
        </dependency>

in http://dl.bintray.com/typesafe/maven-releases/play/play_2.10/2.1.2/play_2.10-2.1.2.pom, but Coursier errors out as:

[info] [error] (update) sbt.librarymanagement.ResolveException: Error downloading org.scala-stm:scala-stm_2.10.6:0.6
[info] [error]   Not found
[info] [error]   not found: /Users/eed3si9n/.ivy2/local/org.scala-stm/scala-stm_2.10.6/0.6/ivys/ivy.xml
[info] [error]   not found: https://repo1.maven.org/maven2/org/scala-stm/scala-stm_2.10.6/0.6/scala-stm_2.10.6-0.6.pom
[info] [error]   not found: https://repo.typesafe.com/typesafe/releases/org/scala-stm/scala-stm_2.10.6/0.6/scala-stm_2.10.6-0.6.pom

It seems like it's trying to overwrite the _2.10.0 with _2.10.6.

dependency-management/latest-local-plugin is about latest.integration. coursier/coursier#209

dependency-management/metadata-only-resolver just runs, so I am going to disable the test.

dependency-management/retrieve-managed-sync is Ivy feature.

dependency-management/scala-organization is on sbt part of Ivy integration. For now this would require ThisBuild / useCoursier := false.

dependency-management/snapshot-resolution failure results from 24h TTL.

dependency-management/test-artifact failure, I'm not sure.

dependency-management/url seems to be some download bug. Reported coursier/coursier#1127.

@eed3si9n

This comment has been minimized.

Copy link
Member Author

eed3si9n commented Apr 19, 2019

project/cross-plugins-defaults couldn't find sbt 0.13 due to missing Typesafe Ivy Releases repo. Adding resolvers += Resolver.typesafeIvyRepo("releases") lets the test pass.

watch/* failures is related to Coursier not excluding sbt artifacts. Reported as coursier/coursier#1128.

repoOverrideTest:scripted dependency-management/default-resolvers failure seems legitimate.

[info] [error] (update) sbt.librarymanagement.ResolveException: Error downloading org.apache.logging.log4j:log4j-slf4j-impl:2.11.2
[info] [error]   Not found
[info] [error]   not found: /Users/eed3si9n/.ivy2/local/org.apache.logging.log4j/log4j-slf4j-impl/2.11.2/ivys/ivy.xml
[info] [error]   not found: https://oss.sonatype.org/content/repositories/public/org/apache/logging/log4j/log4j-slf4j-impl/2.11.2/log4j-slf4j-impl-2.11.2.pom
[info] [error] Error downloading org.apache.logging.log4j:log4j-api:2.11.2
[info] [error]   Not found
[info] [error]   not found: /Users/eed3si9n/.ivy2/local/org.apache.logging.log4j/log4j-api/2.11.2/ivys/ivy.xml
[info] [error]   not found: https://oss.sonatype.org/content/repositories/public/org/apache/logging/log4j/log4j-api/2.11.2/log4j-api-2.11.2.pom
[info] [error] Error downloading org.sonatype.oss:oss-parent:7
[info] [error]   Not found
[info] [error]   not found: /Users/eed3si9n/.ivy2/local/org.sonatype.oss/oss-parent/7/ivys/ivy.xml
[info] [error]   not found: https://oss.sonatype.org/content/repositories/public/org/sonatype/oss/oss-parent/7/oss-parent-7.pom
[info] [error] Error downloading com.google.protobuf:protobuf-java:3.3.1
[info] [error]   Not found
[info] [error]   not found: /Users/eed3si9n/.ivy2/local/com.google.protobuf/protobuf-java/3.3.1/ivys/ivy.xml
[info] [error]   not found: https://oss.sonatype.org/content/repositories/public/com/google/protobuf/protobuf-java/3.3.1/protobuf-java-3.3.1.pom
[info] [error] Error downloading org.sonatype.oss:oss-parent:6
[info] [error]   Not found
[info] [error]   not found: /Users/eed3si9n/.ivy2/local/org.sonatype.oss/oss-parent/6/ivys/ivy.xml
[info] [error]   not found: https://oss.sonatype.org/content/repositories/public/org/sonatype/oss/oss-parent/6/oss-parent-6.pom
[info] [error] Error downloading org.apache.logging.log4j:log4j-core:2.11.2
[info] [error]   Not found
[info] [error]   not found: /Users/eed3si9n/.ivy2/local/org.apache.logging.log4j/log4j-core/2.11.2/ivys/ivy.xml
[info] [error]   not found: https://oss.sonatype.org/content/repositories/public/org/apache/logging/log4j/log4j-core/2.11.2/log4j-core-2.11.2.pom
[info] Project loading failed: (r)etry, (q)uit, (l)ast, or (i)gnore?

This is failing on the resolution of metabuild via Sonatype OSS.

@eed3si9n eed3si9n force-pushed the eed3si9n:wip/coursier branch 2 times, most recently from 76c6622 to 89f2854 Apr 19, 2019
@eed3si9n eed3si9n added this to the 1.3.0 milestone Apr 20, 2019
@eed3si9n eed3si9n mentioned this pull request Apr 21, 2019
1 of 1 task complete
@eed3si9n eed3si9n force-pushed the eed3si9n:wip/coursier branch from 56d30e7 to 6c780d6 Apr 21, 2019
@leonardehrenfried

This comment has been minimized.

Copy link

leonardehrenfried commented Apr 21, 2019

While I don't think I'm up to date anymore with the technical details to add anything substantial, I still would like to say that I'm very happy that this feature is going ahead.

Thank you, Eugene!

@eed3si9n eed3si9n force-pushed the eed3si9n:wip/coursier branch 2 times, most recently from d0d4a36 to 890b73b Apr 21, 2019
@alexarchambault

This comment has been minimized.

Copy link
Contributor

alexarchambault commented Apr 21, 2019

I'll be able to discuss more on Tuesday, but I just don't think it's the right approach.

There's fair amount of logic needed to correctly wire sbt and coursier together, even when relying on the dependencyResolution key. It's mostly done in the sbt-coursier-shared module. The sbt-lm-coursier plugin, that relies on the dependencyResolution key, uses it in particular. Most of the changes done in it have non regression tests, that I don't think this PR would pass.

Overall, this would be quite a regression for sbt-coursier users (to the point that I would probably loudly disassociate myself from this initiative, if this were to be merged as is…).

@eed3si9n

This comment has been minimized.

@eed3si9n eed3si9n force-pushed the eed3si9n:wip/coursier branch from 890b73b to 7bf74bb Apr 22, 2019
Copy link
Contributor

andreaTP left a comment

@eed3si9n again, thanks a lot for taking over! I left a few minor comments but overall great work, can't wait to see it merged!

main/src/main/scala/sbt/Defaults.scala Show resolved Hide resolved
}
)
val notIvyOpt = ivyOpt map { !_ }
coursierOpt.orElse(notIvyOpt).getOrElse(true)

This comment has been minimized.

Copy link
@andreaTP

andreaTP Apr 22, 2019

Contributor

this default to true, are we confident enough to have it as the default choice?

This comment has been minimized.

Copy link
@eed3si9n

eed3si9n Apr 22, 2019

Author Member

I think it's ready to at least try for 1.3.0-RC-1, and run it against Scala Community Build.

coursierOpt.orElse(notIvyOpt).getOrElse(true)
}

def dependencyResolutionTask: Def.Initialize[Task[DependencyResolution]] = Def.taskDyn {

This comment has been minimized.

Copy link
@andreaTP

andreaTP Apr 22, 2019

Contributor

Does this means that changing the from Ivy to Coursier in the shell does work already?

This comment has been minimized.

Copy link
@eed3si9n

eed3si9n Apr 22, 2019

Author Member

It should.

@@ -29,22 +29,7 @@ object Dependencies {
private val utilScripted = "org.scala-sbt" %% "util-scripted" % utilVersion

private val libraryManagementCore = "org.scala-sbt" %% "librarymanagement-core" % lmVersion

This comment has been minimized.

Copy link
@andreaTP

andreaTP Apr 22, 2019

Contributor

why this configuration values got removed?
if this is the case remember that the integration tests in lm will stop to work(e.g. they should be removed as well).

This comment has been minimized.

Copy link
@eed3si9n

eed3si9n Apr 22, 2019

Author Member

I figured that if we adopted lm-coursier from coursier repo, we can remove one from sbt/librarymanagement.

@andreaTP

This comment has been minimized.

Copy link
Contributor

andreaTP commented Apr 22, 2019

@alexarchambault I understand your point on regression tests, but you have also to consider the gain by having coursier as a first class citizen, such as the community build that ensures some good level of confidence.
sbt-coursier will stay as an alternative if any bug got spotted in this PR (and following development).
If you are really afraid on the quality level it will be a nice contribution to add the regression tests themselves on top of this PR 👍

@eed3si9n eed3si9n force-pushed the eed3si9n:wip/coursier branch from 349623e to cc92392 Apr 22, 2019
@leonardehrenfried

This comment has been minimized.

Copy link

leonardehrenfried commented Apr 23, 2019

By the way, is the sbt meta build (plugins) also resolved using coursier or is that still using ivy?

@eed3si9n

This comment has been minimized.

Copy link
Member Author

eed3si9n commented Apr 23, 2019

@leonardehrenfried This PR enables Coursier LM for both metabuild and proper build by default atm. Due to the difference in the way exclusions are interpreted, it might need further tweaking.

@alexarchambault

This comment has been minimized.

Copy link
Contributor

alexarchambault commented Apr 23, 2019

To detail a bit my position on this approach:

  • this doesn't allow to fetch sbt itself with coursier, so the coursier-based sbt launcher is still be needed,
  • this doesn't bring much compared to what the coursier-based launcher already does. The launcher just puts the sbt-lm-coursier plugin along sbt's classpath, which has the same effect as this PR - using coursier in the meta and proper builds. (I don't know much about the internals of the community build… is it easier to test in the community build this way? couldn't the coursier-based sbt launcher be used from there?)
  • this is much less flexible than keeping coursier in a separate plugin. This ties specific versions of sbt with particular versions of coursier. So a hot fix in coursier will now have to wait for a sbt release to be used from sbt, whereas a separate plugin only needs a new release of the plugin (with scala steward automatically bumping the coursier version in the sbt-coursier project, and releases handled from the CI with sbt-ci-release, these should happen almost overnight now). Also, I'm not really looking forward testing the coursier master from sbt by building sbt from sources… Easier to just build and locally publish a plugin.
  • for users already setting some coursier-specific keys in ~/.sbt/1.0/*.sbt, this will probably require a compatibility plugin, to create keys with the same name and under the same namespace. (As the new keys have different names that the original ones in coursier, but also live in an object under the sbt namespace now)
  • the current coursier sbt plugins will probably still be maintained a bit, for the former sbt versions at least. That makes two virtually identical code bases to maintain at the same time, which is just a loss of time.
@eed3si9n

This comment has been minimized.

Copy link
Member Author

eed3si9n commented Apr 23, 2019

@alexarchambault Thanks for the comment.
Here are my interests:

  • overall Scala community weaning off of Ivy, and migrate to Coursier, regardless of sbt (script + launcher) implementation
  • repeatability of the build (what author of the library get is the same as someone else who git clone gets)
  • speedup of Scala Community Build

First I want to thank you for all the effort around Coursier work, and also want to clarify that I don't intend to prevent or compete with csbt (Coursier-based sbt launcher) or sbt-coursier to provide richer experience to many users. Coursier has improved over time, so I think it's time to discuss drawback of having 2 major dependency resolutions. I want to tip the scale so Coursier becomes the outright majority.

To achieve that in a safe manner, I believe shipping Coursier as the out-of-box LM implementation of sbt 1.3.0 is the way to go. This will not require reinstallation of sbt (Bash script + launcher), the library authors just write sbt.version=1.3.0 in build.properties. If for some reason the graph is not satisfactory, they can opt-out as useCoursier := false.

this doesn't bring much compared to what the coursier-based launcher already does.

As noted above, what this PR aims to bring is wider distribution of Coursier and improved repeatability. If I am a library author of scopt etc, and if someone else using csbt got different dependency graph when they git cloned, that's a bug as far as sbt is concerned.

this is much less flexible than keeping coursier in a separate plugin.

I think we should continue to allow Coursier to be bumped using the plugin. In 7091f0e I had to flip the classpath ordering from mjars ++ sbtCp (plugin wins) to sbtCp ++ mjars (sbt classpath wins) due to the way exclusion works, but hopefully we can found our way.

for users already setting some coursier-specific keys in ~/.sbt/1.0/*.sbt, this will probably require a compatibility plugin

Yes, I tried to avoid key collision with sbt-coursier. I am open to suggestion for how we handle this.

the current coursier sbt plugins will probably still be maintained a bit, for the former sbt versions at least. That makes two virtually identical code bases to maintain at the same time, which is just a loss of time.

I agree it's not ideal, but I think the payoff is worth it.
If you have any more concerns, I would be happy to schedule a call with you to discuss more.

@alexarchambault

This comment has been minimized.

Copy link
Contributor

alexarchambault commented Apr 24, 2019

@eed3si9n Thanks for your answer (and for pushing this effort further, overall).

If it's still possible to use sbt-coursier, that's a big showstopper less for me. I don't know a lot about the way classloaders are handled for plugins… Will it still be fine to load newer coursier versions from sbt-coursier (newer versions possibly breaking binary compatibility with the one sbt was built with)?

About the details here, wouldn't it be possible to keep all things related to coursier in a separate auto plugin, itself in a separate module say, rather than putting everything in Defaults.scala? The sbt module could depend on it, but not main say. sbt would depend on it by default, so that would make no difference at first, but that would allow to just exclude it from csbt for example.

@alexarchambault

This comment has been minimized.

Copy link
Contributor

alexarchambault commented Apr 24, 2019

Also, ideally, sbt plugins could depend on main rather than sbt, so that they don't unnecessarily depend on coursier itself (and don't rely on changing things in it).

@alexarchambault

This comment has been minimized.

Copy link
Contributor

alexarchambault commented Apr 24, 2019

To temper the issue with having sbt depend on coursier (but also on shapeless, argonaut, argonaut-shapeless, that the coursier module, with its high level API, now depends on), I'm moderately confident I can have lm-coursier not depend on coursier itself, via some shading. I'm working on it right now here: coursier/sbt-coursier#58.

@alexarchambault

This comment has been minimized.

Copy link
Contributor

alexarchambault commented Apr 24, 2019

There would be two lm-coursier modules: one shading coursier (lm-coursier-shaded), and one with no shading. sbt-coursier would still depend on the latter, to do more advanced stuff with more refined coursier APIs.

If the plugin classpath really has a higher priority than the one of sbt, then the latter module (brought by sbt-coursier) would take over the one with shading, if ever sbt-coursier is added to a build. (Edit: it seems I can have the sbt-lm-coursier plugin rely only on lm-coursier, so it ought to work fine with both the shaded and non-shaded versions of lm-coursier. So we wouldn't even need this classpath take over.)

I could also picture csbt substituting the module with shading for the one with no shading from the sbt classpath, if needed.

@eed3si9n eed3si9n force-pushed the eed3si9n:wip/coursier branch from cc92392 to 96ad731 Apr 26, 2019
@eed3si9n

This comment has been minimized.

Copy link
Member Author

eed3si9n commented Apr 27, 2019

Looks like the tests passed using lm-coursier-shaded.

allExcludeDependencies didn't change the graph, but I think it's because I have sbt as a direct dependency. I'll remove that and see if that would work around the situation.

@eed3si9n eed3si9n force-pushed the eed3si9n:wip/coursier branch from df6f154 to a311cd0 Apr 27, 2019
Ref #4589

This requires sbt server tests to resolve sbt off of local.
@eed3si9n eed3si9n force-pushed the eed3si9n:wip/coursier branch from a311cd0 to f999f6a Apr 27, 2019
@eed3si9n eed3si9n merged commit a3cebd3 into sbt:develop Apr 27, 2019
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
typesafe-cla-validator All users have signed the CLA
Details
@eed3si9n eed3si9n deleted the eed3si9n:wip/coursier branch Apr 27, 2019
@eed3si9n eed3si9n removed the in progress label Apr 27, 2019
@fractal

This comment has been minimized.

Copy link

fractal commented Apr 29, 2019

Excellent news. I'll try this latest setup on my builds. Thanks for the great work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.