From 2929c47652d6f460cbbffb159da9acc038aca664 Mon Sep 17 00:00:00 2001 From: Alexandre Archambault Date: Mon, 3 Jul 2017 12:56:34 +0200 Subject: [PATCH 1/4] Revert "Ensure a same artifact isn't downloaded twice in the same iteration" This reverts commit d437cfb87d0f4b5ddad4a46a8282dc3830557b18. --- .../coursier/core/ResolutionProcess.scala | 20 ++----------------- 1 file changed, 2 insertions(+), 18 deletions(-) diff --git a/core/shared/src/main/scala/coursier/core/ResolutionProcess.scala b/core/shared/src/main/scala/coursier/core/ResolutionProcess.scala index 9ccf4e557..195b237ec 100644 --- a/core/shared/src/main/scala/coursier/core/ResolutionProcess.scala +++ b/core/shared/src/main/scala/coursier/core/ResolutionProcess.scala @@ -64,22 +64,6 @@ final case class Missing( cont: Resolution => ResolutionProcess ) extends ResolutionProcess { - def uniqueModules: Missing = { - - // only try to fetch a single version of a given module in a same iteration, so that resolutions for different - // versions don't try to download the same URL in the same iteration, which the coursier.Cache.Logger API doesn't - // allow - - val missing0 = missing.groupBy(_._1).toSeq.map(_._2).map { - case Seq(v) => v - case Seq() => sys.error("Cannot happen") - case v => - v.maxBy { case (_, v0) => Version(v0) } - } - - copy(missing = missing0) - } - def next(results: Fetch.MD): ResolutionProcess = { val errors = results.collect { @@ -136,7 +120,7 @@ final case class Missing( Continue(res0, cont) } else - Missing(depMgmtMissing.toSeq, res, cont0).uniqueModules + Missing(depMgmtMissing.toSeq, res, cont0) } val current0 = current.copyWithCache( @@ -174,7 +158,7 @@ object ResolutionProcess { if (resolution0.isDone) Done(resolution0) else - Missing(resolution0.missingFromCache.toSeq, resolution0, apply).uniqueModules + Missing(resolution0.missingFromCache.toSeq, resolution0, apply) } } From 9b1d329d0b556dd7b32cf18134dc6c4bc7b9921d Mon Sep 17 00:00:00 2001 From: Alexandre Archambault Date: Mon, 3 Jul 2017 12:59:26 +0200 Subject: [PATCH 2/4] Prevent downloading the same artifact concurrently multiple times --- .../coursier/core/ResolutionProcess.scala | 54 ++++++++++++++----- 1 file changed, 41 insertions(+), 13 deletions(-) diff --git a/core/shared/src/main/scala/coursier/core/ResolutionProcess.scala b/core/shared/src/main/scala/coursier/core/ResolutionProcess.scala index 195b237ec..2e2657abc 100644 --- a/core/shared/src/main/scala/coursier/core/ResolutionProcess.scala +++ b/core/shared/src/main/scala/coursier/core/ResolutionProcess.scala @@ -1,10 +1,12 @@ package coursier package core -import scalaz._ import scala.annotation.tailrec import scala.language.higherKinds +import scalaz.{Monad, -\/, \/-} +import scalaz.Scalaz.{ToFunctorOps, ToTraverseOps, vectorInstance} + sealed abstract class ResolutionProcess { def run[F[_]]( @@ -12,8 +14,7 @@ sealed abstract class ResolutionProcess { maxIterations: Int = 50 )(implicit F: Monad[F] - ): F[Resolution] = { - + ): F[Resolution] = if (maxIterations == 0) F.point(current) else { val maxIterations0 = @@ -23,7 +24,7 @@ sealed abstract class ResolutionProcess { case Done(res) => F.point(res) case missing0 @ Missing(missing, _, _) => - F.bind(fetch(missing))(result => + F.bind(ResolutionProcess.fetchAll(missing, fetch))(result => missing0.next(result).run(fetch, maxIterations0) ) case cont @ Continue(_, _) => @@ -32,7 +33,6 @@ sealed abstract class ResolutionProcess { .run(fetch, maxIterations0) } } - } @tailrec final def next[F[_]]( @@ -40,20 +40,18 @@ sealed abstract class ResolutionProcess { fastForward: Boolean = true )(implicit F: Monad[F] - ): F[ResolutionProcess] = { - + ): F[ResolutionProcess] = this match { - case Done(res) => + case Done(_) => F.point(this) case missing0 @ Missing(missing, _, _) => - F.map(fetch(missing))(result => missing0.next(result)) + F.map(ResolutionProcess.fetchAll(missing, fetch))(result => missing0.next(result)) case cont @ Continue(_, _) => if (fastForward) cont.nextNoCont.next(fetch, fastForward = fastForward) else F.point(cont.next) } - } def current: Resolution } @@ -110,10 +108,10 @@ final case class Missing( val orderedSuccesses = order(depMgmtMissing0.map { case (k, v) => k -> v.intersect(modVer) }.toMap, Nil) val res0 = orderedSuccesses.foldLeft(res) { - case (acc, (modVer, (source, proj))) => + case (acc, (modVer0, (source, proj))) => acc.copyWithCache( projectCache = acc.projectCache + ( - modVer -> (source, acc.withDependencyManagement(proj)) + modVer0 -> (source, acc.withDependencyManagement(proj)) ) ) } @@ -139,7 +137,7 @@ final case class Continue( def next: ResolutionProcess = cont(current) - @tailrec final def nextNoCont: ResolutionProcess = + @tailrec def nextNoCont: ResolutionProcess = next match { case nextCont: Continue => nextCont.nextNoCont case other => other @@ -160,5 +158,35 @@ object ResolutionProcess { else Missing(resolution0.missingFromCache.toSeq, resolution0, apply) } + + private def fetchAll[F[_]](modVers: Seq[(Module, String)], fetch: Fetch.Metadata[F])(implicit F: Monad[F]) = { + + def uniqueModules(modVers: Seq[(Module, String)]): Stream[Seq[(Module, String)]] = { + + val res = modVers.groupBy(_._1).toSeq.map(_._2).map { + case Seq(v) => (v, Nil) + case Seq() => sys.error("Cannot happen") + case v => + // there might be version intervals in there, but that shouldn't matter... + val res = v.maxBy { case (_, v0) => Version(v0) } + (res, v.filter(_ != res)) + } + + val other = res.flatMap(_._2) + + if (other.isEmpty) + Stream(modVers) + else { + val missing0 = res.map(_._1) + missing0 #:: uniqueModules(other) + } + } + + uniqueModules(modVers) + .toVector + .traverse(fetch) + .map(_.flatten) + } + } From 5758e0d786068c8edf8fadca876ee7323f404396 Mon Sep 17 00:00:00 2001 From: Alexandre Archambault Date: Mon, 3 Jul 2017 13:08:59 +0200 Subject: [PATCH 3/4] Rework property substitution --- .../main/scala/coursier/core/Resolution.scala | 83 +++++++++++-------- tests/metadata | 2 +- .../org.drools/drools-compiler/7.0.0.Final | 13 +++ .../org.jitsi/jitsi-videobridge/1.0-SNAPSHOT | 2 +- .../resolutions/org.nd4j/nd4j-native/0.5.0 | 3 +- .../scala/coursier/test/CentralTests.scala | 7 ++ .../scala/coursier/test/ResolutionTests.scala | 4 +- .../test/scala/coursier/test/package.scala | 27 ++++++ 8 files changed, 100 insertions(+), 41 deletions(-) create mode 100644 tests/shared/src/test/resources/resolutions/org.drools/drools-compiler/7.0.0.Final diff --git a/core/shared/src/main/scala/coursier/core/Resolution.scala b/core/shared/src/main/scala/coursier/core/Resolution.scala index c45646005..5030f171a 100644 --- a/core/shared/src/main/scala/coursier/core/Resolution.scala +++ b/core/shared/src/main/scala/coursier/core/Resolution.scala @@ -117,12 +117,9 @@ object Resolution { } } + @deprecated("Originally intended for internal use only", "1.0.0-RC7") def propertiesMap(props: Seq[(String, String)]): Map[String, String] = - props.foldLeft(Map.empty[String, String]) { - case (acc, (k, v0)) => - val v = substituteProps(v0, acc) - acc + (k -> v) - } + substitute(props).toMap /** * Substitutes `properties` in `dependencies`. @@ -361,26 +358,25 @@ object Resolution { def projectProperties(project: Project): Seq[(String, String)] = { // vague attempt at recovering the POM packaging tag - def packagingOpt = project.publications.collectFirst { + val packagingOpt = project.publications.collectFirst { case ("compile", pub) => pub.`type` } // FIXME The extra properties should only be added for Maven projects, not Ivy ones - val properties0 = Seq( + val properties0 = project.properties ++ Seq( // some artifacts seem to require these (e.g. org.jmock:jmock-legacy:2.5.1) // although I can find no mention of them in any manual / spec "pom.groupId" -> project.module.organization, "pom.artifactId" -> project.module.name, - "pom.version" -> project.version, + "pom.version" -> project.actualVersion, // Required by some dependencies too (org.apache.directory.shared:shared-ldap:0.9.19 in particular) "groupId" -> project.module.organization, "artifactId" -> project.module.name, - "version" -> project.version - ) ++ project.properties ++ Seq( + "version" -> project.actualVersion, "project.groupId" -> project.module.organization, "project.artifactId" -> project.module.name, - "project.version" -> project.version + "project.version" -> project.actualVersion ) ++ packagingOpt.toSeq.map { packaging => "project.packaging" -> packaging } ++ project.parent.toSeq.flatMap { @@ -395,6 +391,11 @@ object Resolution { // loose attempt at substituting properties in each others in properties0 // doesn't try to go recursive for now, but that could be made so if necessary + substitute(properties0) + } + + private def substitute(properties0: Seq[(String, String)]): Seq[(String, String)] = { + val done = properties0 .collect { case kv @ (_, value) if propRegex.findFirstIn(value).isEmpty => @@ -402,10 +403,20 @@ object Resolution { } .toMap - properties0.map { + var didSubstitutions = false + + val res = properties0.map { case (k, v) => - k -> substituteProps(v, done) + val res = substituteProps(v, done) + if (!didSubstitutions) + didSubstitutions = res != v + k -> res } + + if (didSubstitutions) + substitute(res) + else + res } /** @@ -422,7 +433,7 @@ object Resolution { // section numbers in the comments refer to withDependencyManagement - val properties = propertiesMap(projectProperties(project)) + val properties = project.properties.toMap val (actualConfig, configurations) = withParentConfigurations(from.configuration, project.configurations) @@ -791,13 +802,13 @@ final case class Resolution( project.parent.toSet else { - val approxProperties0 = - project.parent - .flatMap(projectCache.get) - .map(_._2.properties) - .fold(project.properties)(project.properties ++ _) + val parentProperties0 = project + .parent + .flatMap(projectCache.get) + .map(_._2.properties.toMap) + .getOrElse(Map()) - val approxProperties = propertiesMap(approxProperties0) ++ projectProperties(project) + val approxProperties = parentProperties0 ++ projectProperties(project) val profileDependencies = profiles( @@ -862,6 +873,11 @@ final case class Resolution( ) } + private def withFinalProperties(project: Project): Project = + project.copy( + properties = projectProperties(project) + ) + /** * Add dependency management / inheritance related items to `project`, * from what's available in cache. @@ -903,14 +919,14 @@ final case class Resolution( // A bit fragile, but seems to work - val approxProperties0 = - project.parent - .filter(projectCache.contains) - .map(projectCache(_)._2.properties) - .fold(project.properties)(_ ++ project.properties) + val parentProperties0 = project + .parent + .flatMap(projectCache.get) + .map(_._2.properties) + .getOrElse(Seq()) // 1.1 (see above) - val approxProperties = propertiesMap(approxProperties0) ++ projectProperties(project) + val approxProperties = parentProperties0.toMap ++ projectProperties(project) val profiles0 = profiles( project, @@ -923,18 +939,13 @@ final case class Resolution( // 1.2 made from Pom.scala (TODO look at the very details?) // 1.3 & 1.4 (if only vaguely so) - val properties0 = - (project.properties /: profiles0) { (acc, p) => - acc ++ p.properties - } - - val project0 = project.copy( - properties = project.parent // belongs to 1.5 & 1.6 - .flatMap(projectCache.get) - .fold(properties0)(_._2.properties ++ properties0) + val project0 = withFinalProperties( + project.copy( + properties = parentProperties0 ++ project.properties ++ profiles0.flatMap(_.properties) // belongs to 1.5 & 1.6 + ) ) - val propertiesMap0 = propertiesMap(projectProperties(project0)) + val propertiesMap0 = project0.properties.toMap val dependencies0 = addDependencies( (project0.dependencies +: profiles0.map(_.dependencies)).map(withProperties(_, propertiesMap0)) diff --git a/tests/metadata b/tests/metadata index ba7f6c18c..9a00b31af 160000 --- a/tests/metadata +++ b/tests/metadata @@ -1 +1 @@ -Subproject commit ba7f6c18c146c992cc4b3cddc40f0687a4d1e349 +Subproject commit 9a00b31af466454fbf2604e871c273ba9c1c9938 diff --git a/tests/shared/src/test/resources/resolutions/org.drools/drools-compiler/7.0.0.Final b/tests/shared/src/test/resources/resolutions/org.drools/drools-compiler/7.0.0.Final new file mode 100644 index 000000000..68b39a241 --- /dev/null +++ b/tests/shared/src/test/resources/resolutions/org.drools/drools-compiler/7.0.0.Final @@ -0,0 +1,13 @@ +com.google.protobuf:protobuf-java:2.6.0:compile +com.thoughtworks.xstream:xstream:1.4.9:compile +commons-codec:commons-codec:1.10:compile +org.antlr:antlr-runtime:3.5:compile +org.drools:drools-compiler:7.0.0.Final:compile +org.drools:drools-core:7.0.0.Final:compile +org.eclipse.jdt.core.compiler:ecj:4.4.2:compile +org.kie:kie-api:7.0.0.Final:compile +org.kie:kie-internal:7.0.0.Final:compile +org.mvel:mvel2:2.3.0.Final:compile +org.slf4j:slf4j-api:1.7.7:compile +xmlpull:xmlpull:1.1.3.1:compile +xpp3:xpp3_min:1.1.4c:compile \ No newline at end of file diff --git a/tests/shared/src/test/resources/resolutions/org.jitsi/jitsi-videobridge/1.0-SNAPSHOT b/tests/shared/src/test/resources/resolutions/org.jitsi/jitsi-videobridge/1.0-SNAPSHOT index 6e4462a68..9ed368f25 100644 --- a/tests/shared/src/test/resources/resolutions/org.jitsi/jitsi-videobridge/1.0-SNAPSHOT +++ b/tests/shared/src/test/resources/resolutions/org.jitsi/jitsi-videobridge/1.0-SNAPSHOT @@ -57,7 +57,7 @@ org.fusesource:sigar:1.6.4:compile org.gnu.inet:libidn:1.15:compile org.hamcrest:hamcrest-core:1.1:compile org.igniterealtime:tinder:1.2.3:compile -org.igniterealtime.smack:smack:3.2.2-jitsi-2-20170425.180418-3:compile +org.igniterealtime.smack:smack:3.2.2-jitsi-2-20170425.180420-2:compile org.igniterealtime.smack:smackx:3.2.2-jitsi-2-20170425.180420-2:compile org.igniterealtime.whack:core:2.0.0:compile org.jitsi:bccontrib:1.0:compile diff --git a/tests/shared/src/test/resources/resolutions/org.nd4j/nd4j-native/0.5.0 b/tests/shared/src/test/resources/resolutions/org.nd4j/nd4j-native/0.5.0 index 83dc7191a..67b3009c0 100644 --- a/tests/shared/src/test/resources/resolutions/org.nd4j/nd4j-native/0.5.0 +++ b/tests/shared/src/test/resources/resolutions/org.nd4j/nd4j-native/0.5.0 @@ -6,6 +6,7 @@ org.apache.commons:commons-math3:3.4.1:compile org.bytedeco:javacpp:1.2.3:compile org.javassist:javassist:3.18.2-GA:compile org.nd4j:nd4j-api:0.5.0:compile +org.nd4j:nd4j-backend-impls:0.5.0:compile org.nd4j:nd4j-buffer:0.5.0:compile org.nd4j:nd4j-common:0.5.0:compile org.nd4j:nd4j-context:0.5.0:compile @@ -13,4 +14,4 @@ org.nd4j:nd4j-native:0.5.0:compile org.nd4j:nd4j-native-api:0.5.0:compile org.projectlombok:lombok:1.16.4:compile org.reflections:reflections:0.9.10:compile -org.slf4j:slf4j-api:1.7.10:compile +org.slf4j:slf4j-api:1.7.10:compile \ No newline at end of file diff --git a/tests/shared/src/test/scala/coursier/test/CentralTests.scala b/tests/shared/src/test/scala/coursier/test/CentralTests.scala index 0fae3072e..cd29d7c91 100644 --- a/tests/shared/src/test/scala/coursier/test/CentralTests.scala +++ b/tests/shared/src/test/scala/coursier/test/CentralTests.scala @@ -340,6 +340,13 @@ abstract class CentralTests extends TestSuite { ) } + 'propertySubstitution - { + resolutionCheck( + Module("org.drools", "drools-compiler"), + "7.0.0.Final" + ) + } + 'artifactIdProperties - { resolutionCheck( Module("cc.factorie", "factorie_2.11"), diff --git a/tests/shared/src/test/scala/coursier/test/ResolutionTests.scala b/tests/shared/src/test/scala/coursier/test/ResolutionTests.scala index ebf24abf6..0f5d2e028 100644 --- a/tests/shared/src/test/scala/coursier/test/ResolutionTests.scala +++ b/tests/shared/src/test/scala/coursier/test/ResolutionTests.scala @@ -259,7 +259,7 @@ object ResolutionTests extends TestSuite { val dep = Dependency(Module("acme", "config"), "1.3.0") val res = await(resolve0( Set(dep) - )).clearFinalDependenciesCache + )).clearFinalDependenciesCache.clearProjectProperties val expected = Resolution( rootDependencies = Set(dep), @@ -276,7 +276,7 @@ object ResolutionTests extends TestSuite { val trDep = Dependency(Module("acme", "play-json"), "2.4.0") val res = await(resolve0( Set(dep) - )).clearFinalDependenciesCache + )).clearFinalDependenciesCache.clearProjectProperties val expected = Resolution( rootDependencies = Set(dep), diff --git a/tests/shared/src/test/scala/coursier/test/package.scala b/tests/shared/src/test/scala/coursier/test/package.scala index 75c32ae88..302ae7eb2 100644 --- a/tests/shared/src/test/scala/coursier/test/package.scala +++ b/tests/shared/src/test/scala/coursier/test/package.scala @@ -6,6 +6,22 @@ package object test { def withCompileScope: Dependency = underlying.copy(configuration = "compile") } + private val projectProperties = Set( + "pom.groupId", + "pom.artifactId", + "pom.version", + "groupId", + "artifactId", + "version", + "project.groupId", + "project.artifactId", + "project.version", + "project.packaging", + "project.parent.groupId", + "project.parent.artifactId", + "project.parent.version" + ) + implicit class ResolutionOps(val underlying: Resolution) extends AnyVal { // The content of these fields is typically not validated in the tests. @@ -22,6 +38,17 @@ package object test { ) def clearFilter: Resolution = underlying.copy(filter = None) + def clearProjectProperties: Resolution = + underlying.copy( + projectCache = underlying + .projectCache + .mapValues { + case (s, p) => + (s, p.copy(properties = p.properties.filter { case (k, _) => !projectProperties(k) })) + } + .iterator + .toMap + ) } object Profile { From f665ab1ac268f9edaee3aec8aea76833dbab21cc Mon Sep 17 00:00:00 2001 From: Alexandre Archambault Date: Thu, 6 Jul 2017 18:14:51 +0200 Subject: [PATCH 4/4] Add stub for bin compat --- .../src/main/scala/coursier/core/ResolutionProcess.scala | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/core/shared/src/main/scala/coursier/core/ResolutionProcess.scala b/core/shared/src/main/scala/coursier/core/ResolutionProcess.scala index 2e2657abc..3de4f6e56 100644 --- a/core/shared/src/main/scala/coursier/core/ResolutionProcess.scala +++ b/core/shared/src/main/scala/coursier/core/ResolutionProcess.scala @@ -128,6 +128,10 @@ final case class Missing( cont0(current0) } + @deprecated("Intended for internal use only", "1.0.0-RC7") + def uniqueModules: Missing = + this + } final case class Continue(