From e1d98a1ec2ce42d8abe5b5180e63ebf06ddd7d0b Mon Sep 17 00:00:00 2001 From: jvican Date: Wed, 26 Apr 2017 14:15:03 +0200 Subject: [PATCH] Fix sbt/sbt#1518: Handle cross-versioned exclusions The following PR does two things: * Removes the unnecessary `SbtExclusionRule` that was introduced to exclude artifacts at the project level (and not the dependency level). This change was done in an independent class to avoid breaking bincompat in 0.13.x series. * Handle exclusion rules correctly, propagating the cross version to the exclusions of the dependencies. To fix sbt/sbt#1518, this PR takes the avenue taken in `SbtExclusionRule`, it accepts `GroupArtifactID` which should be the preferred way to specify dependencies from now on. Unlike `SbtExclusionRule`, it also supports `ModuleID` for those that want to exclude a concrete dependency. `InclExcl` did not have any tests. The following commit also adds a testing suite for it, showing how the issue it's fixed and how you should use `exclude` if you're calling directly `ExclusionRule` instead of passing in `GroupArtifactID` and `ModuleID`. --- .../main/contraband/librarymanagement.json | 28 +++----- .../sbt/internal/librarymanagement/Ivy.scala | 27 +++++--- .../SbtExclusionRuleExtra.scala | 18 ----- .../librarymanagement/CrossVersionExtra.scala | 12 ++-- .../sbt/librarymanagement/IvyInterface.scala | 23 ++++++- .../sbt/librarymanagement/ModuleIDExtra.scala | 4 +- .../src/test/scala/InclExclSpec.scala | 67 +++++++++++++++++++ 7 files changed, 120 insertions(+), 59 deletions(-) delete mode 100644 librarymanagement/src/main/scala/sbt/internal/librarymanagement/SbtExclusionRuleExtra.scala create mode 100644 librarymanagement/src/test/scala/InclExclSpec.scala diff --git a/librarymanagement/src/main/contraband/librarymanagement.json b/librarymanagement/src/main/contraband/librarymanagement.json index e9f16c123..c93091568 100644 --- a/librarymanagement/src/main/contraband/librarymanagement.json +++ b/librarymanagement/src/main/contraband/librarymanagement.json @@ -207,18 +207,20 @@ "target": "Scala", "type": "record", "doc": [ - "Rule to either:", + "Defines a rule to either:", "", - "Which one depends on the parameter name which it is passed to, but the filter has the same fields in both cases." + "The use case that is applied depends on the parameter name which it is passed to, but the", + "filter has the same fields in both cases." ], "fields": [ { "name": "organization", "type": "String", "default": "\"*\"", "since": "0.0.1" }, { "name": "name", "type": "String", "default": "\"*\"", "since": "0.0.1" }, { "name": "artifact", "type": "String", "default": "\"*\"", "since": "0.0.1" }, - { "name": "configurations", "type": "String*", "default": "Vector.empty", "since": "0.0.1" } + { "name": "configurations", "type": "String*", "default": "Vector.empty", "since": "0.0.1" }, + { "name": "crossVersion", "type": "sbt.librarymanagement.CrossVersion", "default": "sbt.librarymanagement.Disabled()", "since": "0.0.1"} ], "parentsCompanion": "sbt.librarymanagement.InclExclRuleFunctions" }, @@ -374,7 +376,7 @@ { "name": "moduleInfo", "type": "sbt.librarymanagement.ModuleInfo" }, { "name": "dependencies", "type": "sbt.librarymanagement.ModuleID*" }, { "name": "overrides", "type": "Set[sbt.librarymanagement.ModuleID]", "default": "Set.empty", "since": "0.0.1" }, - { "name": "excludes", "type": "sbt.internal.librarymanagement.SbtExclusionRule*", "default": "Vector.empty", "since": "0.0.1" }, + { "name": "excludes", "type": "sbt.librarymanagement.InclExclRule*", "default": "Vector.empty", "since": "0.0.1" }, { "name": "ivyXML", "type": "scala.xml.NodeSeq", "default": "scala.xml.NodeSeq.Empty", "since": "0.0.1" }, { "name": "configurations", "type": "sbt.librarymanagement.Configuration*", "default": "Vector.empty", "since": "0.0.1" }, { "name": "defaultConfiguration", "type": "Option[sbt.librarymanagement.Configuration]", "default": "None", "since": "0.0.1" }, @@ -781,20 +783,6 @@ { "name": "configurationsToRetrieve", "type": "Option[Set[sbt.librarymanagement.Configuration]]", "default": "None", "since": "0.0.1" } ] }, - { - "name": "SbtExclusionRule", - "namespace": "sbt.internal.librarymanagement", - "target": "Scala", - "type": "record", - "fields": [ - { "name": "organization", "type": "String" }, - { "name": "name", "type": "String" }, - { "name": "artifact", "type": "String" }, - { "name": "configurations", "type": "String*" }, - { "name": "crossVersion", "type": "sbt.librarymanagement.CrossVersion" } - ], - "parentsCompanion": "sbt.internal.librarymanagement.SbtExclusionRuleFunctions" - }, { "name": "UpdateReportLite", "namespace": "sbt.internal.librarymanagement", diff --git a/librarymanagement/src/main/scala/sbt/internal/librarymanagement/Ivy.scala b/librarymanagement/src/main/scala/sbt/internal/librarymanagement/Ivy.scala index b5084ffb6..bd83242f9 100644 --- a/librarymanagement/src/main/scala/sbt/internal/librarymanagement/Ivy.scala +++ b/librarymanagement/src/main/scala/sbt/internal/librarymanagement/Ivy.scala @@ -567,22 +567,30 @@ private[sbt] object IvySbt { ) } - private def substituteCross(m: ModuleSettings): ModuleSettings = + private def substituteCross(m: ModuleSettings): ModuleSettings = { m.ivyScala match { case None => m case Some(is) => substituteCross(m, is.scalaFullVersion, is.scalaBinaryVersion) } + } + private def substituteCross( m: ModuleSettings, scalaFullVersion: String, scalaBinaryVersion: String ): ModuleSettings = { - val sub = CrossVersion(scalaFullVersion, scalaBinaryVersion) m match { case ic: InlineConfiguration => - ic.withModule(sub(ic.module)) - .withDependencies(ic.dependencies map sub) - .withOverrides(ic.overrides map sub) + val applyCross = CrossVersion(scalaFullVersion, scalaBinaryVersion) + def propagateCrossVersion(moduleID: ModuleID): ModuleID = { + val crossExclusions: Vector[ExclusionRule] = + moduleID.exclusions.map(CrossVersion.substituteCross(_, ic.ivyScala)) + applyCross(moduleID) + .withExclusions(crossExclusions) + } + ic.withModule(applyCross(ic.module)) + .withDependencies(ic.dependencies.map(propagateCrossVersion)) + .withOverrides(ic.overrides map applyCross) case _ => m } } @@ -852,13 +860,12 @@ private[sbt] object IvySbt { def addExcludes( moduleID: DefaultModuleDescriptor, - excludes: Seq[SbtExclusionRule], + excludes: Seq[ExclusionRule], ivyScala: Option[IvyScala] - ): Unit = - excludes foreach addExclude(moduleID, ivyScala) + ): Unit = excludes.foreach(exclude => addExclude(moduleID, ivyScala)(exclude)) + def addExclude(moduleID: DefaultModuleDescriptor, ivyScala: Option[IvyScala])( - exclude0: SbtExclusionRule - ): Unit = { + exclude0: ExclusionRule): Unit = { // this adds _2.11 postfix val exclude = CrossVersion.substituteCross(exclude0, ivyScala) val confs = diff --git a/librarymanagement/src/main/scala/sbt/internal/librarymanagement/SbtExclusionRuleExtra.scala b/librarymanagement/src/main/scala/sbt/internal/librarymanagement/SbtExclusionRuleExtra.scala deleted file mode 100644 index 6c5c01245..000000000 --- a/librarymanagement/src/main/scala/sbt/internal/librarymanagement/SbtExclusionRuleExtra.scala +++ /dev/null @@ -1,18 +0,0 @@ -package sbt.internal.librarymanagement - -import sbt.internal.librarymanagement.impl._ -import sbt.librarymanagement._ - -abstract class SbtExclusionRuleFunctions { - def apply(organization: String, name: String): SbtExclusionRule = - SbtExclusionRule(organization, name, "*", Vector.empty, Disabled()) - - def apply(organization: String): SbtExclusionRule = apply(organization, "*") - - implicit def groupIdToExclusionRule(organization: GroupID): SbtExclusionRule = - apply(organization.groupID) - implicit def stringToExclusionRule(organization: String): SbtExclusionRule = apply(organization) - - implicit def groupArtifactIDToExclusionRule(gaid: GroupArtifactID): SbtExclusionRule = - SbtExclusionRule(gaid.groupID, gaid.artifactID, "*", Vector.empty, gaid.crossVersion) -} diff --git a/librarymanagement/src/main/scala/sbt/librarymanagement/CrossVersionExtra.scala b/librarymanagement/src/main/scala/sbt/librarymanagement/CrossVersionExtra.scala index 7e90f8c59..29ef28327 100644 --- a/librarymanagement/src/main/scala/sbt/librarymanagement/CrossVersionExtra.scala +++ b/librarymanagement/src/main/scala/sbt/librarymanagement/CrossVersionExtra.scala @@ -1,6 +1,5 @@ package sbt.librarymanagement -import sbt.internal.librarymanagement.SbtExclusionRule import sbt.internal.librarymanagement.cross.CrossVersionUtil final case class ScalaVersion(full: String, binary: String) @@ -69,9 +68,7 @@ abstract class CrossVersionFunctions { /** Constructs the cross-version function defined by `module` and `is`, if one is configured. */ def apply(module: ModuleID, is: Option[IvyScala]): Option[String => String] = - is flatMap { i => - apply(module, i) - } + is.flatMap(i => apply(module, i)) /** Cross-version each `Artifact` in `artifacts` according to cross-version function `cross`. */ def substituteCross( @@ -94,9 +91,9 @@ abstract class CrossVersionFunctions { /** Cross-versions `exclude` according to its `crossVersion`. */ private[sbt] def substituteCross( - exclude: SbtExclusionRule, + exclude: ExclusionRule, is: Option[IvyScala] - ): SbtExclusionRule = { + ): ExclusionRule = { val fopt: Option[String => String] = is flatMap { i => CrossVersion(exclude.crossVersion, i.scalaFullVersion, i.scalaBinaryVersion) @@ -111,8 +108,7 @@ abstract class CrossVersionFunctions { private[sbt] def substituteCrossA( as: Vector[Artifact], cross: Option[String => String] - ): Vector[Artifact] = - as.map(art => substituteCross(art, cross)) + ): Vector[Artifact] = as.map(art => substituteCross(art, cross)) /** * Constructs a function that will cross-version a ModuleID diff --git a/librarymanagement/src/main/scala/sbt/librarymanagement/IvyInterface.scala b/librarymanagement/src/main/scala/sbt/librarymanagement/IvyInterface.scala index 82ddace88..25eb5bc13 100644 --- a/librarymanagement/src/main/scala/sbt/librarymanagement/IvyInterface.scala +++ b/librarymanagement/src/main/scala/sbt/librarymanagement/IvyInterface.scala @@ -5,9 +5,30 @@ package sbt.librarymanagement import org.apache.ivy.core.module.descriptor import org.apache.ivy.util.filter.{ Filter => IvyFilter } +import sbt.internal.librarymanagement.impl.{ GroupArtifactID, GroupID } abstract class InclExclRuleFunctions { - def everything = InclExclRule("*", "*", "*", Vector.empty) + def everything = InclExclRule("*", "*", "*", Vector.empty, Disabled()) + + def apply(organization: String, name: String): InclExclRule = + InclExclRule(organization, name, "*", Vector.empty, Disabled()) + + def apply(organization: String): InclExclRule = apply(organization, "*") + + implicit def groupIdToExclusionRule(organization: GroupID): InclExclRule = + apply(organization.groupID) + implicit def stringToExclusionRule(organization: String): InclExclRule = apply(organization) + + implicit def groupArtifactIDToExclusionRule(gaid: GroupArtifactID): InclExclRule = + InclExclRule(gaid.groupID, gaid.artifactID, "*", Vector.empty, gaid.crossVersion) + + implicit def moduleIDToExclusionRule(moduleID: ModuleID): InclExclRule = { + val org = moduleID.organization + val name = moduleID.name + val version = moduleID.revision + val crossVersion = moduleID.crossVersion + InclExclRule(org, name, version, Vector.empty, crossVersion) + } } abstract class ArtifactTypeFilterExtra { diff --git a/librarymanagement/src/main/scala/sbt/librarymanagement/ModuleIDExtra.scala b/librarymanagement/src/main/scala/sbt/librarymanagement/ModuleIDExtra.scala index 7fbb5f348..5b89b5419 100644 --- a/librarymanagement/src/main/scala/sbt/librarymanagement/ModuleIDExtra.scala +++ b/librarymanagement/src/main/scala/sbt/librarymanagement/ModuleIDExtra.scala @@ -121,11 +121,11 @@ abstract class ModuleIDExtra { * Applies the provided exclusions to dependencies of this module. Note that only exclusions that specify * both the exact organization and name and nothing else will be included in a pom.xml. */ - def excludeAll(rules: InclExclRule*) = copy(exclusions = this.exclusions ++ rules) + def excludeAll(rules: ExclusionRule*) = copy(exclusions = this.exclusions ++ rules) /** Excludes the dependency with organization `org` and `name` from being introduced by this dependency during resolution. */ def exclude(org: String, name: String) = - excludeAll(InclExclRule().withOrganization(org).withName(name)) + excludeAll(ExclusionRule().withOrganization(org).withName(name)) /** * Adds extra attributes for this module. All keys are prefixed with `e:` if they are not already so prefixed. diff --git a/librarymanagement/src/test/scala/InclExclSpec.scala b/librarymanagement/src/test/scala/InclExclSpec.scala new file mode 100644 index 000000000..c3a867550 --- /dev/null +++ b/librarymanagement/src/test/scala/InclExclSpec.scala @@ -0,0 +1,67 @@ +package sbt.librarymanagement + +import org.scalatest.Assertion +import sbt.internal.librarymanagement.BaseIvySpecification +import sbt.internal.librarymanagement.impl.{ DependencyBuilders, GroupArtifactID } + +class InclExclSpec extends BaseIvySpecification with DependencyBuilders { + def createLiftDep(toExclude: ExclusionRule): ModuleID = + ("net.liftweb" %% "lift-mapper" % "2.6-M4" % "compile").excludeAll(toExclude) + + def createMetaDep(toExclude: ExclusionRule): ModuleID = + ("org.scalameta" %% "paradise" % "3.0.0-M8" % "compile") + .cross(CrossVersion.full) + .excludeAll(toExclude) + + def getIvyReport(dep: ModuleID, scalaVersion: Option[String]): UpdateReport = { + cleanIvyCache() + val ivyModule = module(defaultModuleId, Vector(dep), scalaVersion) + ivyUpdate(ivyModule) + } + + def testLiftJsonIsMissing(report: UpdateReport): Assertion = { + assert( + !report.allModules.exists(_.name.contains("lift-json")), + "lift-json has not been excluded." + ) + } + + def testScalahostIsMissing(report: UpdateReport): Assertion = { + assert( + !report.allModules.exists(_.name.contains("scalahost")), + "scalahost has not been excluded." + ) + } + + val scala210 = Some("2.10.4") + it should "exclude any version of lift-json via a new exclusion rule" in { + val toExclude = ExclusionRule("net.liftweb", "lift-json_2.10") + val report = getIvyReport(createLiftDep(toExclude), scala210) + testLiftJsonIsMissing(report) + } + + it should "exclude any version of lift-json with explicit Scala version" in { + val excluded: GroupArtifactID = "net.liftweb" % "lift-json_2.10" + val report = getIvyReport(createLiftDep(excluded), scala210) + testLiftJsonIsMissing(report) + } + + it should "exclude any version of cross-built lift-json" in { + val excluded: GroupArtifactID = "net.liftweb" %% "lift-json" + val report = getIvyReport(createLiftDep(excluded), scala210) + testLiftJsonIsMissing(report) + } + + val scala2122 = Some("2.12.2") + it should "exclude a concrete version of lift-json when it's full cross version" in { + val excluded: ModuleID = ("org.scalameta" % "scalahost" % "1.7.0").cross(CrossVersion.full) + val report = getIvyReport(createMetaDep(excluded), scala2122) + testScalahostIsMissing(report) + } + + it should "exclude any version of lift-json when it's full cross version" in { + val excluded = new GroupArtifactID("net.liftweb", "lift-json", CrossVersion.full) + val report = getIvyReport(createMetaDep(excluded), scala2122) + testScalahostIsMissing(report) + } +}