Rework property substitution

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))

tests/metadata

@@ -1 +1 @@
-Subproject commit ba7f6c18c146c992cc4b3cddc40f0687a4d1e349
+Subproject commit 9a00b31af466454fbf2604e871c273ba9c1c9938

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

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

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

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"),

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),

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 {