Library management API #124

eed3si9n · 2017-07-05T10:48:59Z

This is continuation of #113 and #123 on a shared branch.

The goal of Library Management API is to create an Ivy-free interface that can abstract different dependency resolution implementations such as Ivy, cached resolution, and Coursier.

Note this PR is intended to be Refactoring, in the sense that this should not introduce any change to the observable behavior.

DependencyResolution

DependencyResolution is the user-facing API that contains useful helper functions.
The intended audience is build users and plugin authors.

scala> import java.io.File
import java.io.File

scala> import sbt.librarymanagement._, syntax._
import sbt.librarymanagement._
import syntax._

scala> val log = sbt.util.LogExchange.logger("test")
log: sbt.internal.util.ManagedLogger = sbt.internal.util.ManagedLogger@c439b0f

scala> val lm = {
         import sbt.librarymanagement.ivy._
         val ivyConfig = InlineIvyConfiguration().withLog(log)
         IvyDependencyResolution(ivyConfig)
       }
lm: sbt.librarymanagement.DependencyResolution = sbt.librarymanagement.DependencyResolution@6a9b40f8

scala> val module = "commons-io" % "commons-io" % "2.5"
module: sbt.librarymanagement.ModuleID = commons-io:commons-io:2.5

scala> lm.retrieve(module, scalaModuleInfo = None, new File("target"), log)
res0: Either[sbt.librarymanagement.UnresolvedWarning,Vector[java.io.File]] = Right(Vector(target/jars/commons-io/commons-io/commons-io-2.5.jar, target/jars/commons-io/commons-io/commons-io-2.5.jar, target/jars/commons-io/commons-io/commons-io-2.5.jar))

DependencyResolutionInterface

DependencyResolutionInterface is the interface dependency resolution engine authors will implement to provide the resolution feature:

/**
 * Interface for dependency resolution intended for engine authors.
 */
trait DependencyResolutionInterface {

  /**
   * Builds a ModuleDescriptor that describes a subproject with dependencies.
   *
   * @param moduleSetting It contains the information about the module including the dependencies.
   * @return A `ModuleDescriptor` describing a subproject and its dependencies.
   */
  def moduleDescriptor(moduleSetting: ModuleDescriptorConfiguration): ModuleDescriptor

  /**
   * Resolves the given module's dependencies performing a retrieval.
   *
   * @param module The module to be resolved.
   * @param configuration The update configuration.
   * @param uwconfig The configuration to handle unresolved warnings.
   * @param log The logger.
   * @return The result, either an unresolved warning or an update report. Note that this
   *         update report will or will not be successful depending on the `missingOk` option.
   */
  def update(module: ModuleDescriptor,
             configuration: UpdateConfiguration,
             uwconfig: UnresolvedWarningConfiguration,
             log: Logger): Either[UnresolvedWarning, UpdateReport]
}

trait ModuleDescriptor {
  def directDependenciesForWarning: Vector[ModuleID]
  def scalaModuleInfo: Option[ScalaModuleInfo]
}

These methods define the most general forms of performing update.
If the implementation already integrates with sbt, it should be easy to implement them.

IvyLibraryManagement

Here's the reference implementation using Ivy.

https://github.com/eed3si9n/librarymanagement/blob/7b21353a06aab419de7892fd13cee2f4da8f26da/ivy/src/main/scala/sbt/librarymanagement/IvyLibraryManagement.scala

Credits

This PR is co-authored by Eugene Yokota (@eed3si9n, Lightbend) and Martin Duhem (@Duhemm, Scala Center). I've squash a bunch of commits by Martin from #125 and #127.

Duhemm · 2017-07-06T12:05:32Z

Note: I've already started to port sbt/zinc and sbt/sbt to the new LM API described in this PR:

[Do not merge] Adapt Zinc to use the new LibraryManagement API zinc#335

Duhemm · 2017-07-10T12:38:50Z

Duhemm · 2017-07-10T13:14:30Z

I guess we should move everything that's specific to the Ivy implementation to the sbt.librarymanagement.ivy namespace. WDYT?

eed3si9n · 2017-07-11T10:08:10Z

@Duhemm Here's my take on the builder pattern - e077272

eed3si9n · 2017-07-11T10:48:46Z

@Duhemm

IvyConfiguration uses a logger, do we really need to give another one to retrieve? Can't it use the logger of its LibraryManagement instance? Does IvyConfiguration really need a logger?

We need logger during the setup. Task logging needs to be split for last to work etc.

Both IvyPaths and IvyConfiguration need a baseDirectory. Couldn't they share the same? Fixed in #128

IvyPaths is used throughout testing, and I actually think it's a good idea to change them both at the same time, so I am keeping this one. I did move baseDirectory to external, so there are not duplicates.

We can get default resolvers using Resolver.withDefaultResolvers. I think that we should drop the with from the name. Fixed in #128

I don't think defaultResolvers is good since it's returning more than the default.
combineDefaultResolvers? (since we can't say for sure if it's prepend or append)

Why do we need resolvers and otherResolvers?

See Keys.scala.

val otherResolvers = TaskKey[Seq[Resolver]]("other-resolvers", "Resolvers not included in the main resolver chain, such as those in module configurations.", CSetting)

Resolver.withDefaultResolvers returns a Seq, but InlineIvyConfiguration wants a Vector. Fixed in #128

We should use Vector.

Why isn't there a way to set the configuration on an instance of ModuleDescriptor?

Not sure what you mean by this. You pass in the list of configurations to InlineConfigurations.

def moduleDescriptor takes a Vector[Configuration], but ModuleID takes Option[String].

I think this fine. The module descriptor has the authoritative list of configurations that it defines. ModuleID can have complex expressions like "test->test".

eed3si9n · 2017-07-11T22:48:49Z

Rebased and squashed.

Here's demo a code:

scala> import java.io.File
import java.io.File

scala> import sbt.librarymanagement._, syntax._
import sbt.librarymanagement._
import syntax._

scala> val log = sbt.util.LogExchange.logger("test")
log: sbt.internal.util.ManagedLogger = sbt.internal.util.ManagedLogger@c439b0f

scala> val lm = {
         import sbt.librarymanagement.ivy._
         val ivyConfig = InlineIvyConfiguration().withLog(log)
         IvyLibraryManagement(ivyConfig)
       }
lm: sbt.librarymanagement.LibraryManagement = sbt.librarymanagement.ivy.IvyLibraryManagement@11c07acb

scala> val module = "commons-io" % "commons-io" % "2.5"
module: sbt.librarymanagement.ModuleID = commons-io:commons-io:2.5

scala> lm.retrieve(module, scalaModuleInfo = None, new File("target"), log)
res0: Either[sbt.librarymanagement.UnresolvedWarning,Vector[java.io.File]] =
  Right(Vector(target/jars/commons-io/commons-io/commons-io-2.5.jar,
    target/jars/commons-io/commons-io/commons-io-2.5.jar,
    target/jars/commons-io/commons-io/commons-io-2.5.jar))

Duhemm · 2017-07-12T09:09:26Z

Very nice. I think that we now have a much better story in terms of user API.

If you think the change is worth it, this commit: Duhemm@460d4d7 changes the configurations in ModuleID to be a Vector[String] rather than Option[String]. The tests pass, but I admit that I am not 100% confident about this change as it is now. I may very well have broken lots of other things. If you think that this is an improvement, I'd be happy to work a bit more on it and making sure that I didn't break something.

I'll update my WIP PR to port Zinc to this new API, this will give us a new datapoint to assess the quality of this API.

dwijnand · 2017-07-12T10:00:04Z

@Duhemm I like what you've done - I've always been in favour of a more typed way to define module configurations. But I think in addition to splitting the string into a seq we should type the elements, i.e not have them as strings such as "compile" and "test->test".

Also, what's the thinking behind using a Vector("compile") default over empty vector?

eed3si9n · 2017-07-12T10:09:35Z

@Duhemm This PR should be refactoring, so the fact that the expected value on the specs are changing makes me a bit suspicious. Also String represents full expression such as runtime->*;test->default(compile) (see http://ant.apache.org/ivy/history/latest-milestone/ivyfile/dependency.html), so I think Option[String] is fine for now.

dwijnand · 2017-07-12T10:05:44Z

core/src/main/scala/sbt/librarymanagement/DependencyBuilders.scala

+      organizationArtifact(name, CrossVersion.binary)
+
+    private def organizationArtifact(name: String, cross: CrossVersion) = {
+      nonEmpty(name, "Artifact ID")


dwijnand · 2017-07-12T10:11:21Z

core/src/main/scala/sbt/librarymanagement/LibraryManagement.scala

+/**
+ * Helper mixin to provide methods for library management
+ */
+abstract class LibraryManagement extends LibraryManagementInterface {


(Forwarding my review of #123) Please could we define two different entities and use composition over inheritance?

Yes. Sorry about that.

dwijnand · 2017-07-12T10:13:01Z

core/src/main/scala/sbt/librarymanagement/LibraryManagementSyntax.scala

@@ -0,0 +1,24 @@
+package sbt.librarymanagement
+
+abstract class LibraryManagementSyntax extends DependencyBuilders {


Could this stay as a trait please? So it can be a mixin.

Is it really worth it? The trait-to-interface is still somewhat fragile (e.g. val must be final without return types), and I don't know who would ever need to mix this in.

We need it https://github.com/sbt/sbt/blob/1.0.x/sbt/src/main/scala/package.scala#L11

ok. I guess I'll turn them into traits and put comments to remind future developers to use final val.

dwijnand · 2017-07-12T10:13:23Z

core/src/main/scala/sbt/librarymanagement/ResolverExtra.scala

@@ -6,10 +6,10 @@ package sbt.librarymanagement
 import java.io.{ IOException, File }
 import java.net.URL
 import scala.xml.XML
-import org.apache.ivy.plugins.resolver.DependencyResolver
+// import org.apache.ivy.plugins.resolver.DependencyResolver


commented code

dwijnand · 2017-07-12T10:15:39Z

ivy/src/main/scala/sbt/internal/librarymanagement/MakePom.scala

@@ -259,7 +260,7 @@ class MakePom(val log: Logger) {
        types match {
          case Nil                                     => Artifact.PomType
          case xs if xs.contains(Artifact.DefaultType) => Artifact.DefaultType
-          case x :: xs                                 => x
+          case x :: (xs @ _)                           => x


what's going on here?

I think that's to silent the huge number of warnings. If I'm right, I would prefer case x :: _ => ...

Avoiding the compiler warning. Could also be _, but I grew up writing x :: xs.

TIL. Thank you.

dwijnand · 2017-07-12T10:16:08Z

core/src/main/scala/sbt/librarymanagement/VersionNumber.scala

@@ -145,7 +145,8 @@ object VersionNumber {
        case (v1, v2)
            if (v1.size >= 3) && (v2.size >= 3) => // A normal version number MUST take the form X.Y.Z
          (v1._1.get, v1._2.get, v1._3.get, v1.tags, v2._1.get, v2._2.get, v2._3.get, v2.tags) match {
-            case (x1, y1, 0, ts1, x2, y2, 0, ts2) if ts1.nonEmpty || ts2.nonEmpty =>
+            case (x1 @ _, y1 @ _, 0, ts1, x2 @ _, y2 @ _, 0, ts2)


what with all the @ _?

Avoiding compiler warnings.

dwijnand · 2017-07-12T10:21:07Z

ivy/src/main/scala/sbt/internal/librarymanagement/IvyActions.scala

+
+    // Todo. Fix publish ordering https://github.com/sbt/sbt/issues/2088#issuecomment-246208872
+    val ivyFile: Option[File] =
+      if (configuration.publishMavenStyle) None


Isn't this a blocker for this PR? Does this (with the todo above) mean we no longer publish POM files?

This is the first step towards folding deliver into publish.
I haven't changed anything with POM. I think it's handled as an artifact in Defaults.scala:

publishArtifact in makePom := publishMavenStyle.value && publishArtifact.value, artifact in makePom := Artifact.pom(moduleName.value),

dwijnand · 2017-07-12T10:26:21Z

ivy/src/main/scala/sbt/internal/librarymanagement/IvyActions.scala

+      case a: IArtifact => applyFilter(f, a)
+    }
+
+    def applyFilter(f: ArtifactTypeFilter, a: IArtifact): Boolean =


The f is redundant.

dwijnand · 2017-07-12T10:28:59Z

ivy/src/main/scala/sbt/internal/librarymanagement/ivyint/CachedResolutionResolveEngine.scala

@@ -209,7 +209,7 @@ private[sbt] class CachedResolutionResolveCache {
       else None) match {
        case Some(path) =>
          log.debug(s"parsing ${path.getAbsolutePath.toString}")
-          val ur = JsonUtil.parseUpdateReport(md, path, cachedDescriptor, log)
+          val ur = JsonUtil.parseUpdateReport( /* md, */ path, cachedDescriptor, log)


commented code

Duhemm · 2017-07-12T11:05:12Z

@dwijnand I'm not super familiar with this code, but my understanding is that when there is no configuration that is explicitly set (None), it means compile. Similarly, here an empty vector means compile.

@eed3si9n I agree, this is just a POC. I changed the tests to compensate for the :compile that is always shown. Previously, we wouldn't show it if it was None (which implicitly means compile).

Duhemm · 2017-07-12T11:48:58Z

InlineConfiguration does not follow the builder pattern. Is that on purpose?

eed3si9n · 2017-07-12T11:52:54Z

@Duhemm

Yes. There's a weird coupling between module: ModuleID and moduleInfo: ModuleInfo. Hopefully it's not a big deal since there are some helper functions in LibraryManagement to define one.

dwijnand · 2017-07-12T12:16:09Z

core/src/main/scala/sbt/librarymanagement/LibraryManagementInterface.scala

+/**
+ * Interface for library management intended for library management engine authors.
+ */
+abstract class LibraryManagementInterface {


Could the interfaces in this file be interfaces?

Also can we call this RetrieverInterface (or similar)?

retrieve is a specific feature so a generic sounding name would be DependencyResolutionInterface.

dwijnand · 2017-07-12T13:25:46Z

LGTM, pending sbt/zinc and sbt/sbt ripple PRs.

Duhemm

See comment

Duhemm · 2017-07-12T13:28:07Z

core/src/main/scala/sbt/librarymanagement/LibraryManagementInterface.scala

+   * if any.
+   */
+  def scalaModuleInfo: Option[ScalaModuleInfo]
+}


Could you please split this in 3 files (DependencyResolutionInterface.scala, PublisherInterface.scala and ModuleDescriptor.scala)?

I actually thought it would be a good idea to keep all the open-ended interfaces so we can link to the file and talk about it.

Forgot about transitiveScratch

Duhemm · 2017-07-13T08:47:50Z

I forgot about transitiveScratch, it's still part of this API. Do we really want it there? Could we get documentation?

Martin created ripple PRs

This splits the core of LM and Ivy-based implementation. - InlineConfiguration is renamed to ModuleConfiguration - IvyScala is renamed to ScalaModuleInfo - UpdateConfiguration, RetrieveConfiguration, PublishConfiguration are refactored to use builder pattern. - Adds ConfigRef for referencing Configuration - Moves syntax related things into `sbt.librarymagement.syntax`

This is used only by sbt, and we are not sure if it needs to be part of the API

eed3si9n · 2017-07-15T16:08:25Z

@Duhemm noticed that MakePomConfiguration was plain class, so I added a09af23. This is also rebased on config fix that exposes Configuration.of(..).

eed3si9n · 2017-07-15T18:07:00Z

Tests passing and transitiveScratch was ripped out, so I think this is good to go!

eed3si9n mentioned this pull request Jul 5, 2017

Library management API #123

Closed

Duhemm mentioned this pull request Jul 6, 2017

[Do not merge] Adapt Zinc to use the new LibraryManagement API sbt/zinc#335

Closed

eed3si9n requested review from dwijnand and Duhemm July 9, 2017 03:52

Duhemm mentioned this pull request Jul 11, 2017

ZincCompilerUtil#scalaCompiler doesn't accept ModuleID sbt/zinc#310

Open

eed3si9n mentioned this pull request Jul 11, 2017

Builder pattern for IvyConfiguration, remove IvyPaths #128

Closed

eed3si9n force-pushed the topic/api branch from 5779da6 to 0211517 Compare July 11, 2017 22:43

dwijnand previously requested changes Jul 12, 2017

View reviewed changes

dwijnand reviewed Jul 12, 2017

View reviewed changes

Duhemm requested changes Jul 12, 2017

View reviewed changes

Duhemm previously approved these changes Jul 13, 2017

View reviewed changes

eed3si9n mentioned this pull request Jul 13, 2017

Re-expose or offer alternative public API for things hidden in 1.0 sbt/sbt#3296

Closed

eed3si9n added 2 commits July 15, 2017 11:17

Minor cleanups per review

a98c5f4

eed3si9n added 6 commits July 15, 2017 11:17

Use delegation instead of inheritance

7844190

turn back syntax classes into traits

358b750

rename to DependencyResolutionInterface

0c28db9

Bump to latest Contraband

c95a885

Remove transitiveScratch

1d2f4d0

This is used only by sbt, and we are not sure if it needs to be part of the API

Builder pattern for MakePomConfiguration

a09af23

eed3si9n force-pushed the topic/api branch from cd974ac to a09af23 Compare July 15, 2017 16:06

Bump to latest IO and Util

509c80c

eed3si9n merged commit 0147e0c into 1.0 Jul 15, 2017

eed3si9n deleted the topic/api branch July 15, 2017 18:07

		@@ -0,0 +1,24 @@
		package sbt.librarymanagement

		abstract class LibraryManagementSyntax extends DependencyBuilders {

Library management API #124

Library management API #124

Conversation

eed3si9n commented Jul 5, 2017 • edited Loading

DependencyResolution

DependencyResolutionInterface

IvyLibraryManagement

Credits

Duhemm commented Jul 6, 2017 • edited Loading

Duhemm commented Jul 10, 2017 • edited Loading

Duhemm commented Jul 10, 2017

eed3si9n commented Jul 11, 2017

eed3si9n commented Jul 11, 2017

eed3si9n commented Jul 11, 2017

Duhemm commented Jul 12, 2017

dwijnand commented Jul 12, 2017

eed3si9n commented Jul 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Duhemm commented Jul 12, 2017

Duhemm commented Jul 12, 2017

eed3si9n commented Jul 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dwijnand commented Jul 12, 2017

Duhemm left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Duhemm commented Jul 13, 2017

eed3si9n commented Jul 15, 2017

eed3si9n commented Jul 15, 2017

eed3si9n commented Jul 5, 2017 •

edited

Loading

Duhemm commented Jul 6, 2017 •

edited

Loading

Duhemm commented Jul 10, 2017 •

edited

Loading

Duhemm left a comment •

edited

Loading