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

Add Coursier as a library management implementation #270

Merged
merged 17 commits into from Oct 29, 2018

Conversation

Projects
None yet
5 participants
@andreaTP
Copy link
Contributor

andreaTP commented Oct 16, 2018

This is a revamp of the great effort done first by @leonardehrenfried in #190

Now we can reproduce the test cases using the scripted tests implementation.
Current status is that normal project dependencies are properly downloaded and resolved, while sbt plugins gets downloaded but not discovered by Sbt itself.

I'm not sure if the resolution of sbt-plugins is mandatory to start integrating this PR.
I've noticed that now coursier is able to publish artifacts, but I think this can be added in a second stage.

I will ask for help and review by @alexarchambault and @eed3si9n

@eed3si9n eed3si9n added the ready label Oct 16, 2018

@andreaTP

This comment has been minimized.

Copy link
Contributor

andreaTP commented Oct 16, 2018

Essentially, this is a port of the current sbt-coursier sbt plugin, I just revamped the previous implementation and added integration tests with sbt/sbt.

The only current problem is that using coursier to fetch sbtplugins (with addSbtPlugin) result in having the artifacts fetched but sbt do not load the plugin itself.

@leonardehrenfried

This comment has been minimized.

Copy link
Contributor

leonardehrenfried commented Oct 16, 2018

This is a fantastic step in the right direction. I'm not sure I can do a meaningful review but I'm going to try anyway.

However, I also got stuck on the sbt plugins and that was one of the reasons I couldn't complete the PR, as I ran out of ideas.

One question: when you set the resolver to coursier does the sbt launcher also fetch the sbt jars themselves using coursier or ivy? On a CI environment this is often the slowest part of the build.

@andreaTP

This comment has been minimized.

Copy link
Contributor

andreaTP commented Oct 16, 2018

One question: when you set the resolver to coursier does the sbt launcher also fetch the sbt jars themselves using coursier or ivy? On a CI environment this is often the slowest part of the build.

I'm pretty sure this is not happening, nor I have a clue on how this can be done, maybe @eed3si9n can help?
As you can see on the hack for testing the sbt-plugins integration also going in that direction starts to become tricky https://github.com/sbt/librarymanagement/pull/270/files#diff-3a3697897c85dadd0627ea7d643e5b31R3

@eed3si9n

This comment has been minimized.

Copy link
Member

eed3si9n commented Oct 16, 2018

One question: when you set the resolver to coursier does the sbt launcher also fetch the sbt jars themselves using coursier or ivy? On a CI environment this is often the slowest part of the build.

sbt launcher is its own small program that has its own Ivy embedded. It might be good to at least parallelize download of that like sbt 1's Ivy in lm.

@eed3si9n eed3si9n added review and removed ready labels Oct 16, 2018

@leonardehrenfried

This comment has been minimized.

Copy link
Contributor

leonardehrenfried commented Oct 17, 2018

sbt launcher is its own small program that has its own Ivy embedded. It might be good to at least parallelize download of that like sbt 1's Ivy in lm.

I experimented with using coursier for this too: leonardehrenfried/sbt@4d41125#diff-2d60fc47edf4d9f1c9de681e52e5cd47R95

I downloaded the dependencies but then the JVM couldn't find or load them. In the end I gave up.

IMO, we can however solve one problem at a time and this one can be tackled later. In any case, the change would need to happen to the sbt/sbt repo.

@alexarchambault

This comment has been minimized.

Copy link
Contributor

alexarchambault commented Oct 17, 2018

Thanks for your efforts @andreaTP!

Have you thought about adding this to the sbt-coursier repository instead? It already has plenty of scripted tests. These currently ensure the sbt-coursier plugin is fine, but I guess they could be tweaked to run against a custom version of sbt, depending on lm-coursier, like you do here. These tests should be rather thorough, so this could help catch bugs in lm-coursier (and debug the issue with sbt plugins I guess).

I can help move that there.

@@ -259,6 +260,22 @@ lazy val lmIvy = (project in file("ivy"))
),
)

lazy val lmCoursier = (project in file("coursier"))
.enablePlugins(ContrabandPlugin, JsonCodecPlugin)

This comment has been minimized.

@alexarchambault

alexarchambault Oct 18, 2018

Contributor

Are these plugins necessary?

Suggested change Beta
.enablePlugins(ContrabandPlugin, JsonCodecPlugin)

This comment has been minimized.

@andreaTP

andreaTP Oct 19, 2018

Contributor

kept Contraband that is now used and removed the unused JsonCodec

build.sbt Outdated
commonSettings,
crossScalaVersions := Seq(scala212, scala211),
name := "librarymanagement-coursier",
libraryDependencies ++= Seq(coursier, coursierCache, scalaTest, scalaCheck),

This comment has been minimized.

@alexarchambault

alexarchambault Oct 18, 2018

Contributor

Only depend on scalaTest and scalaCheck for the tests?

Suggested change Beta
libraryDependencies ++= Seq(coursier, coursierCache, scalaTest, scalaCheck),
libraryDependencies ++= Seq(coursier, coursierCache, scalaTest % Test, scalaCheck % Test),

This comment has been minimized.

@andreaTP

andreaTP Oct 19, 2018

Contributor

correct thanks

name := "librarymanagement-coursier",
libraryDependencies ++= Seq(coursier, coursierCache, scalaTest, scalaCheck),
managedSourceDirectories in Compile +=
baseDirectory.value / "src" / "main" / "contraband-scala",

This comment has been minimized.

@alexarchambault

alexarchambault Oct 18, 2018

Contributor

Like above, is this needed? (unless you plan to add contraband stuff to lmCoursier?)

This comment has been minimized.

@andreaTP

andreaTP Oct 19, 2018

Contributor

now yes


case class CoursierModuleSettings() extends ModuleSettings

private[sbt] class CoursierDependencyResolution(resolvers: Vector[Resolver])

This comment has been minimized.

@alexarchambault

alexarchambault Oct 18, 2018

Contributor

Couldn't this be like a case class, or made behave like one? That way, fields corresponding to the various sbt-coursier keys could correspond to fields here, that could be customized via a .copy(…).

Users could then easily tweak various parameters of the resolution.

And it might also be interesting to have a next version of sbt-coursier just set and / or customize dependencyResolution, updating the various fields. (That would provide a migration path sbt-coursier -> lm-coursier).

Using a simple case class might pose some binary compatibility issues though… Maybe contraband could help here? (Maybe via a separate class to keep all the parameters.)

This comment has been minimized.

@alexarchambault

alexarchambault Oct 18, 2018

Contributor

All the keys don't necessarily need to be here in a first time, just something case-class like, fine with binary compatibility if we add fields, would allow to add the various fields later.

This comment has been minimized.

@andreaTP

andreaTP Oct 19, 2018

Contributor

I'm adding a contraband CoursierSettings class, this also affect previous comments around contraband itself

val localArtifacts: Map[Artifact, Either[FileError, File]] = Gather[Task]
.gather(
resolution.artifacts.map { a =>
Cache.file[Task](a).run.map((a, _))

This comment has been minimized.

@alexarchambault

alexarchambault Oct 18, 2018

Contributor

You might want to pass a logger here too, for progress bars and all to appear.

This comment has been minimized.

@andreaTP

andreaTP Oct 19, 2018

Contributor

done


import scala.concurrent.ExecutionContext.Implicits.global

val fetch = Fetch.from(repositories, Cache.fetch[Task](logger = Some(createLogger())))

This comment has been minimized.

@alexarchambault

alexarchambault Oct 18, 2018

Contributor

Logger (TermDisplay at least) should be stopped (via TermDisplay.stop()). This might result in spurious terminal state else (some progress bar updates not being flushed in particular).

This comment has been minimized.

@andreaTP

andreaTP Oct 19, 2018

Contributor

done

andreaTP added some commits Oct 19, 2018

@andreaTP

This comment has been minimized.

Copy link
Contributor

andreaTP commented Oct 19, 2018

@alexarchambault first round done, ready for the next 😃

@andreaTP

This comment has been minimized.

Copy link
Contributor

andreaTP commented Oct 24, 2018

@alexarchambault finally solved the issue with the meta-build! 🥇

I reworked it again a few times until being able to compile sbt itself with coursier as LM.
@alexarchambault would you mind to make a new review? thanks in advance!

@typesafe-tools

This comment has been minimized.

Copy link

typesafe-tools commented Oct 25, 2018

The validator has checked the following projects, tested using dbuild, projects built on top of each other.

Project Reference Commit
sbt pull/4430/head sbt/sbt@8a8e33b
zinc develop sbt/zinc@c8e1f53
io develop sbt/io@e6529c0
librarymanagement pull/270/head abc6d69
util develop sbt/util@cd67959

The result is: SUCCESS

The binaries are at https://dl.bintray.com/sbt/maven-snapshots
The new sbt version is: 1.3.0-bin-20181025T102644
(restart)

@typesafe-tools

This comment has been minimized.

Copy link

typesafe-tools commented Oct 25, 2018

A validation involving this pull request is in progress...

@typesafe-tools

This comment has been minimized.

Copy link

typesafe-tools commented Oct 25, 2018

The validator has checked the following projects, tested using dbuild, projects built on top of each other.

Project Reference Commit
sbt pull/4430/head sbt/sbt@8a8e33b
zinc develop sbt/zinc@c8e1f53
io develop sbt/io@e6529c0
librarymanagement pull/270/head 789a4cc
util develop sbt/util@cd67959

The result is: SUCCESS

The binaries are at https://dl.bintray.com/sbt/maven-snapshots
The new sbt version is: 1.3.0-bin-20181025T151651
(restart)

@andreaTP andreaTP force-pushed the andreaTP:coursierAgain branch from 789a4cc to abc6d69 Oct 26, 2018

@typesafe-tools

This comment has been minimized.

Copy link

typesafe-tools commented Oct 26, 2018

A validation involving this pull request is in progress...

@typesafe-tools

This comment has been minimized.

Copy link

typesafe-tools commented Oct 26, 2018

The validator has checked the following projects, tested using dbuild, projects built on top of each other.

Project Reference Commit
sbt pull/4430/head sbt/sbt@1ad89a4
zinc develop sbt/zinc@c8e1f53
io develop sbt/io@e6529c0
librarymanagement pull/270/head abc6d69
util develop sbt/util@cd67959

The result is: SUCCESS

The binaries are at https://dl.bintray.com/sbt/maven-snapshots
The new sbt version is: 1.3.0-bin-20181026T084504
(restart)

@eed3si9n
Copy link
Member

eed3si9n left a comment

Thanks for all the work @leonardehrenfried, @andreaTP, and @alexarchambault

@eed3si9n eed3si9n merged commit 4b179e5 into sbt:develop Oct 29, 2018

1 check passed

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

alexarchambault added a commit to alexarchambault/sbt-coursier that referenced this pull request Nov 20, 2018

alexarchambault added a commit to alexarchambault/sbt-coursier that referenced this pull request Nov 20, 2018

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