Skip to content

Commit

Permalink
Improve version comparison
Browse files Browse the repository at this point in the history
Ref coursier/versions#10
`compat.isCompatible(...)` doesn't work when the version number includes
weird alphabet tags in there like `3.10.1.Final`.
This tries to work round the issue by calculating the minimum compatible
versions and comparing the two.
  • Loading branch information
eed3si9n committed Jan 9, 2021
1 parent e678aea commit 1836307
Show file tree
Hide file tree
Showing 7 changed files with 79 additions and 36 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -56,23 +56,23 @@ case class LintCommand(
val errors = deps.groupBy(_.dependency.map(_.module)).collect {
case (Some(dep), ts) if ts.size > 1 =>
dep -> ts.collect {
case TargetIndex(_, _, _, Some(dep)) => dep.version
case TargetIndex(_, _, _, Some(dep)) => dep
}
}
val isTransitive = errors.toList.flatMap {
case (m, vs) =>
for {
v <- vs
dep = SimpleDependency(m, v)
dep = SimpleDependency(m, v.version, v.classifier)
tdep <- index.dependencies(dep)
if tdep.dependency != Some(dep)
} yield tdep
}.toSet

val reportedErrors = errors.filter {
case (module, versions) =>
val deps = versions
.map(v => SimpleDependency(module, v))
case (module, deps0) =>
val deps = deps0
.map(v => SimpleDependency(module, v.version, v.classifier))
.flatMap(index.byDependency.get(_))
!deps.exists(isTransitive)
}
Expand All @@ -89,7 +89,7 @@ case class LintCommand(
(module, versions) <- errors
} {
log(
s"target '$root' depends on conflicting versions of the 3rdparty dependency '${module.repr}:{${versions.commas}}'.\n" +
s"target '$root' depends on conflicting versions of the 3rdparty dependency '${module.repr}:{${versions.map(_.version).commas}}'.\n" +
s"\tTo fix this problem, modify the dependency list of this target so that it only depends on one version of the 3rdparty module '${module.repr}'"
)
}
Expand Down
29 changes: 21 additions & 8 deletions multiversion/src/main/scala/multiversion/indexes/TargetIndex.scala
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,14 @@ object TargetIndex {
case _ => None
}
}
private object JvmClassifier {
private val Classifier: Regex = "jvm_classifier=(.+)".r
def unapply(s: String): Option[String] =
s match {
case Classifier(x) => Some(x)
case _ => None
}
}
def fromQuery(t: Target): TargetIndex = {
def getStringListAttribute(key: String): List[String] =
(for {
Expand All @@ -44,16 +52,21 @@ object TargetIndex {
} yield dep).toList
val deps = getStringListAttribute("deps")
val tags = getStringListAttribute("tags")
val dependency = tags match {
case collection.Seq(JvmModule(module), JvmVersion(version)) =>
val module = tags.collectFirst {
case JvmModule(module) => module
}
val version = tags.collectFirst {
case JvmVersion(version) => version
}
val classifier = tags.collectFirst {
case JvmClassifier(classifier) => classifier
}
val dependency = (module, version) match {
case (Some(m), Some(v)) =>
Some(
SimpleDependency(
SimpleModule(module.organization.value, module.name.value),
version
)
SimpleDependency(SimpleModule(m.organization.value, m.name.value), v, classifier)
)
case _ =>
None
case _ => None
}
TargetIndex(
name = t.getRule().getName(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -95,17 +95,21 @@ object ArtifactOutput {
"jars" -> Docs.array(mavenLabel),
"deps" -> Docs.array(depsRef: _*),
"exports" -> Docs.array(depsRef: _*),
"tags" -> Docs.array(
s"jvm_module=${dependency.module.repr}",
s"jvm_version=${dependency.version}"
),
"tags" -> Docs.array(tags(dependency): _*),
"visibility" -> Docs.array("//visibility:public")
)

genrule.toDoc /
scalaImport.toDoc
}

def tags(dep: Dependency): List[String] =
List(s"jvm_module=${dep.module.repr}", s"jvm_version=${dep.version}") ::: {
if (dep.publication.classifier.nonEmpty)
List(s"jvm_classifier=${dep.publication.classifier.value}")
else Nil
}

def buildEvictedDoc(
dep: Dependency,
winner: String,
Expand All @@ -122,11 +126,7 @@ object ArtifactOutput {
"jars" -> Docs.array(),
"deps" -> Docs.array(depsRef: _*),
"exports" -> Docs.array(depsRef: _*),
"tags" -> Docs.array(
s"jvm_module=${dep.module.repr}",
s"jvm_version=${dep.version}",
s"evicted=True"
),
"tags" -> Docs.array(tags(dep) ::: List("evicted=True"): _*),
"visibility" -> Docs.array("//visibility:public")
)
scalaImport.toDoc
Expand Down
Original file line number Diff line number Diff line change
@@ -1,19 +1,20 @@
package multiversion.outputs

import multiversion.resolvers.SimpleDependency
import multiversion.resolvers.SimpleModule
import org.typelevel.paiges.Doc

final case class LintOutput(
root: String,
conflicts: Map[SimpleModule, Set[String]],
conflicts: Map[SimpleModule, Set[SimpleDependency]],
isFailure: Boolean
) {
def toDoc: Doc = {
// Sort the conflicts to ensure the output is stable.
val sortedConflicts = conflicts
.map {
case (module, versions) =>
val versionsDoc = Docs.array(versions.toList.sorted: _*)
case (module, deps) =>
val versionsDoc = Docs.array(deps.toList.map(_.version).sorted: _*)
module.repr -> versionsDoc
}
.toList
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ import coursier.core.Dependency
import coursier.core.Module
import coursier.core.Version
import coursier.version.VersionCompatibility
import coursier.version.VersionInterval
import coursier.version.VersionParse
import multiversion.configs.ThirdpartyConfig
import multiversion.diagnostics.MultidepsEnrichments.XtensionDependency
import multiversion.resolvers.DependencyId
Expand Down Expand Up @@ -168,11 +170,7 @@ object ResolutionIndex {
val versions = deps.map(d => Version(d.version))
versions.foreach { version =>
val isCompatible = winners.exists { winner =>
// we need to check both ways
if (
compat.isCompatible(version.repr, winner.repr) ||
compat.isCompatible(winner.repr, version.repr)
) {
if (isCompat(version.repr, winner.repr, compat)) {
if (lessThan(winner, version) && !isUnstable(version)) {
winners.remove(winner)
winners.add(version)
Expand All @@ -189,9 +187,21 @@ object ResolutionIndex {
val result = (for {
dep <- deps
winner <- winners
if dep.version != winner.repr &&
compat.isCompatible(dep.version, winner.repr)
if (dep.version != winner.repr) && isCompat(dep.version, winner.repr, compat)
} yield dep -> winner.repr).toMap
result
}

def isCompat(version1: String, version2: String, compat: VersionCompatibility): Boolean = {
val min1 = minimumVersion(version1, compat)
val min2 = minimumVersion(version2, compat)
(min1 == min2) || (min1 + ".0" == min2) || (min1 == min2 + ".0")
}

def minimumVersion(version: String, compat: VersionCompatibility): String = {
val c = VersionParse.versionConstraint(version)
if (c.interval != VersionInterval.zero && c.interval.from.isDefined)
compat.minimumCompatibleVersion(c.interval.from.get.repr)
else compat.minimumCompatibleVersion(version)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,17 @@ import scala.util.hashing.MurmurHash3

case class SimpleDependency(
module: SimpleModule,
version: String
version: String,
classifier: Option[String],
) {
def repr: String = s"${module.repr}:$version"
private[this] def classifierStr: String =
classifier match {
case Some(x) => s"-$x"
case _ => ""
}
// https://repo1.maven.org/maven2/org/apache/kafka/kafka-clients/2.4.0/ lists
// kafka-clients-2.4.0-test.jar
def repr: String = s"${module.repr}:$version${classifierStr}"

override lazy val hashCode: Int = MurmurHash3.productHash(this)
}
12 changes: 11 additions & 1 deletion tests/src/test/scala/tests/commands/ExportCommandSuite.scala
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,16 @@ class ExportCommandSuite extends tests.BaseSuite {
|""".stripMargin
)

checkDeps(
"netty",
"""| - dependency: io.netty:netty:3.10.1.Final
| - dependency: io.netty:netty:3.7.0.Final
|""".stripMargin,
queryArgs = allGenrules,
expectedQuery = """|@maven//:genrules/io.netty_netty_3.10.1.Final
|""".stripMargin
)

checkDeps(
"basic",
"""| - dependency: com.google.guava:guava:29.0-jre
Expand Down Expand Up @@ -61,7 +71,7 @@ class ExportCommandSuite extends tests.BaseSuite {
| versionScheme: pvp
| - dependency: com.lihaoyi:pprint_2.12:0.5.9
|${scalaLibrary("MyApp.scala", "object MyApp { val x = 42 }")}
|""".stripMargin
|""".stripMargin,
)

checkDeps(
Expand Down

0 comments on commit 1836307

Please sign in to comment.