Maven version range improvement

Previously, when the dependency resolver (Ivy) encountered a Maven version range such as `[1.3.0,)` it would go out to the Internet to find the latest version. This would result to a surprising behavior where the eventual version keeps changing over time *even when there's a version of the library that satisfies the range condition*. This changes to some Maven version ranges would be replaced with its lower bound so that when a satisfactory version is found in the dependency graph it will be used. You can disable this behavior using the JVM flag `-Dsbt.modversionrange=false`. Fixes #2954 Ref #2291 / #2953
2017-02-10 02:43:26 -05:00 · 2017-02-10 02:43:26 -05:00 · bb258f42a6
parent 728e57827f
commit bb258f42a6
5 changed files with 120 additions and 49 deletions
--- a/.gitignore
+++ b/.gitignore
@ -1,2 +1,3 @@
 tmp/
 target/
+__pycache__
--- a/librarymanagement/src/main/scala/sbt/internal/librarymanagement/CustomPomParser.scala
+++ b/librarymanagement/src/main/scala/sbt/internal/librarymanagement/CustomPomParser.scala
@ -162,6 +162,17 @@ object CustomPomParser {
      def isIdentity = false
    }

+  // TODO: It would be better if we can make dd.isForce to `false` when VersionRange.isVersionRange is `true`.
+  private[this] def stripVersionRange(dd: DependencyDescriptor): DependencyDescriptor =
+    VersionRange.stripMavenVersionRange(dd.getDependencyRevisionId.getRevision) match {
+      case Some(newVersion) =>
+        val id = dd.getDependencyRevisionId
+        val newId = ModuleRevisionId.newInstance(id.getOrganisation, id.getName, id.getBranch, newVersion, id.getExtraAttributes)
+        transform(dd, _ => newId)
+      case None => dd
+    }
+  private[sbt] lazy val versionRangeFlag = sys.props.get("sbt.modversionrange") map { _.toLowerCase == "true" } getOrElse true
+
  import collection.JavaConverters._
  def addExtra(properties: Map[String, String], dependencyExtra: Map[ModuleRevisionId, Map[String, String]], parser: ModuleDescriptorParser, md: ModuleDescriptor): ModuleDescriptor =
    {
@ -187,7 +198,10 @@ object CustomPomParser {
      IvySbt.addExtraNamespace(dmd)

      val withExtra = md.getDependencies map { dd => addExtra(dd, dependencyExtra) }
-      val unique = IvySbt.mergeDuplicateDefinitions(withExtra)
+      val withVersionRangeMod: Seq[DependencyDescriptor] =
+        if (versionRangeFlag) withExtra map { stripVersionRange }
+        else withExtra
+      val unique = IvySbt.mergeDuplicateDefinitions(withVersionRangeMod)
      unique foreach dmd.addDependency

      for (ed <- md.getInheritedDescriptors) dmd.addInheritedDescriptor(new DefaultExtendsDescriptor(md, ed.getLocation, ed.getExtendsTypes))
--- a/librarymanagement/src/main/scala/sbt/internal/librarymanagement/MakePom.scala
+++ b/librarymanagement/src/main/scala/sbt/internal/librarymanagement/MakePom.scala
@ -26,56 +26,10 @@ import sbt.io.IO

 object MakePom {
  /** True if the revision is an ivy-range, not a complete revision. */
-  def isDependencyVersionRange(revision: String): Boolean = {
-    (revision endsWith "+") ||
-      (revision contains "[") ||
-      (revision contains "]") ||
-      (revision contains "(") ||
-      (revision contains ")")
-  }
+  def isDependencyVersionRange(revision: String): Boolean = VersionRange.isVersionRange(revision)

  /** Converts Ivy revision ranges to that of Maven POM */
-  def makeDependencyVersion(revision: String): String = {
-    def plusRange(s: String, shift: Int = 0) = {
-      def pow(i: Int): Int = if (i > 0) 10 * pow(i - 1) else 1
-      val (prefixVersion, lastVersion) = (s + "0" * shift).reverse.split("\\.", 2) match {
-        case Array(revLast, revRest) =>
-          (revRest.reverse + ".", revLast.reverse)
-        case Array(revLast) => ("", revLast.reverse)
-      }
-      val lastVersionInt = lastVersion.toInt
-      s"[${prefixVersion}${lastVersion},${prefixVersion}${lastVersionInt + pow(shift)})"
-    }
-    val startSym = Set(']', '[', '(')
-    val stopSym = Set(']', '[', ')')
-    val DotPlusPattern = """(.+)\.\+""".r
-    val DotNumPlusPattern = """(.+)\.(\d+)\+""".r
-    val NumPlusPattern = """(\d+)\+""".r
-    val maxDigit = 5
-    try {
-      revision match {
-        case "+"                           => "[0,)"
-        case DotPlusPattern(base)          => plusRange(base)
-        // This is a heuristic.  Maven just doesn't support Ivy's notions of 1+, so
-        // we assume version ranges never go beyond 5 siginificant digits.
-        case NumPlusPattern(tail)          => (0 until maxDigit).map(plusRange(tail, _)).mkString(",")
-        case DotNumPlusPattern(base, tail) => (0 until maxDigit).map(plusRange(base + "." + tail, _)).mkString(",")
-        case rev if rev endsWith "+"       => sys.error(s"dynamic revision '$rev' cannot be translated to POM")
-        case rev if startSym(rev(0)) && stopSym(rev(rev.length - 1)) =>
-          val start = rev(0)
-          val stop = rev(rev.length - 1)
-          val mid = rev.substring(1, rev.length - 1)
-          (if (start == ']') "(" else start) + mid + (if (stop == '[') ")" else stop)
-        case _ => revision
-      }
-    } catch {
-      case e: NumberFormatException =>
-        // TODO - if the version doesn't meet our expectations, maybe we just issue a hard
-        //        error instead of softly ignoring the attempt to rewrite.
-        //sys.error(s"Could not fix version [$revision] into maven style version")
-        revision
-    }
-  }
+  def makeDependencyVersion(revision: String): String = VersionRange.fromIvyToMavenVersion(revision)
 }
 class MakePom(val log: Logger) {
  import MakePom._
--- a/librarymanagement/src/main/scala/sbt/internal/librarymanagement/VersionRange.scala
+++ b/librarymanagement/src/main/scala/sbt/internal/librarymanagement/VersionRange.scala
@ -0,0 +1,83 @@
+package sbt
+package internal
+package librarymanagement
+
+object VersionRange {
+  /** True if the revision is an ivy-range, not a complete revision. */
+  def isVersionRange(revision: String): Boolean = {
+    (revision endsWith "+") ||
+      (revision contains "[") ||
+      (revision contains "]") ||
+      (revision contains "(") ||
+      (revision contains ")")
+  }
+
+  // Assuming Ivy is used to resolve conflict, this removes the version range
+  // when it is open-ended to avoid dependency resolution hitting the Internet to get the latest.
+  // See https://github.com/sbt/sbt/issues/2954
+  def stripMavenVersionRange(version: String): Option[String] =
+    if (isVersionRange(version)) {
+      val noSpace = version.replaceAllLiterally(" ", "")
+      noSpace match {
+        case MavenVersionSetPattern(open1, x1, comma, x2, close1, rest) =>
+          // http://maven.apache.org/components/enforcer/enforcer-rules/versionRanges.html
+          (open1, Option(x1), Option(comma), Option(x2), close1) match {
+            case (_, None, _, Some(x2), "]") => Some(x2)
+            // a good upper bound is unknown
+            case (_, None, _, Some(x2), ")") => None
+            case (_, Some(x1), _, None, _)   => Some(x1)
+            case _                           => None
+          }
+        case _ => None
+      }
+    } else None
+
+  /** Converts Ivy revision ranges to that of Maven POM */
+  def fromIvyToMavenVersion(revision: String): String = {
+    def plusRange(s: String, shift: Int = 0) = {
+      def pow(i: Int): Int = if (i > 0) 10 * pow(i - 1) else 1
+      val (prefixVersion, lastVersion) = (s + "0" * shift).reverse.split("\\.", 2) match {
+        case Array(revLast, revRest) =>
+          (revRest.reverse + ".", revLast.reverse)
+        case Array(revLast) => ("", revLast.reverse)
+      }
+      val lastVersionInt = lastVersion.toInt
+      s"[${prefixVersion}${lastVersion},${prefixVersion}${lastVersionInt + pow(shift)})"
+    }
+    val DotPlusPattern = """(.+)\.\+""".r
+    val DotNumPlusPattern = """(.+)\.(\d+)\+""".r
+    val NumPlusPattern = """(\d+)\+""".r
+    val maxDigit = 5
+    try {
+      revision match {
+        case "+"                           => "[0,)"
+        case DotPlusPattern(base)          => plusRange(base)
+        // This is a heuristic.  Maven just doesn't support Ivy's notions of 1+, so
+        // we assume version ranges never go beyond 5 siginificant digits.
+        case NumPlusPattern(tail)          => (0 until maxDigit).map(plusRange(tail, _)).mkString(",")
+        case DotNumPlusPattern(base, tail) => (0 until maxDigit).map(plusRange(base + "." + tail, _)).mkString(",")
+        case rev if rev endsWith "+"       => sys.error(s"dynamic revision '$rev' cannot be translated to POM")
+        case rev if startSym(rev(0)) && stopSym(rev(rev.length - 1)) =>
+          val start = rev(0)
+          val stop = rev(rev.length - 1)
+          val mid = rev.substring(1, rev.length - 1)
+          (if (start == ']') "(" else start) + mid + (if (stop == '[') ")" else stop)
+        case _ => revision
+      }
+    } catch {
+      case e: NumberFormatException =>
+        // TODO - if the version doesn't meet our expectations, maybe we just issue a hard
+        //        error instead of softly ignoring the attempt to rewrite.
+        //sys.error(s"Could not fix version [$revision] into maven style version")
+        revision
+    }
+  }
+
+  def hasMavenVersionRange(version: String): Boolean =
+    if (version.length <= 1) false
+    else startSym(version(0)) && stopSym(version(version.length - 1))
+
+  private[this] val startSym = Set(']', '[', '(')
+  private[this] val stopSym = Set(']', '[', ')')
+  private[this] val MavenVersionSetPattern = """([\]\[\(])([\w\.\-]+)?(,)?([\w\.\-]+)?([\]\[\)])(,.+)?""".r
+}
--- a/librarymanagement/src/test/scala/sbt/internal/librarymanagement/VersionRangeSpec.scala
+++ b/librarymanagement/src/test/scala/sbt/internal/librarymanagement/VersionRangeSpec.scala
@ -0,0 +1,19 @@
+package sbt
+package internal
+package librarymanagement
+
+class VersionRangeSpec extends UnitSpec {
+  "Version range" should "strip 1.0 to None" in stripTo("1.0", None)
+  it should "strip (,1.0] to 1.0" in stripTo("(,1.0]", Some("1.0"))
+  it should "strip (,1.0) to None" in stripTo("(,1.0)", None)
+  it should "strip [1.0] to 1.0" in stripTo("[1.0]", Some("1.0"))
+  it should "strip [1.0,) to 1.0" in stripTo("[1.0,)", Some("1.0"))
+  it should "strip (1.0,) to 1.0" in stripTo("(1.0,)", Some("1.0"))
+  it should "strip (1.0,2.0) to None" in stripTo("(1.0,2.0)", None)
+  it should "strip [1.0,2.0] to None" in stripTo("[1.0,2.0]", None)
+  it should "strip (,1.0],[1.2,) to 1.0" in stripTo("(,1.0],[1.2,)", Some("1.0"))
+  it should "strip (,1.1),(1.1,) to None" in stripTo("(,1.1),(1.1,)", None)
+
+  def stripTo(s: String, expected: Option[String]) =
+    assert(VersionRange.stripMavenVersionRange(s) == expected)
+}