Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Basic support for exclusions in inline dependencies #169

Merged
merged 1 commit into from Sep 10, 2011

Conversation

Projects
None yet
3 participants
Member

indrajitr commented Sep 4, 2011

Hey Mark,

This is bit of a roughcut and not meant to be merged right away. It's more to solicit feedback and polish more. But let's see if we can converge on this long outstanding issue. I am suddenly finding this important, so thought of scratching the itch :)

The goal is to support the basic case of excluding a library from the dependency tree given it's organization and name, no more. This, I imagine, meets most use-cases barring a few esoteric ones (for all such things there is full blown ivyXML setting anyway).

So these should work now:

val dhttp = "net.databinder" %% "dispatch-http" % "0.7.8" excluding("commons-logging" % "commons-logging")
val dhttp = "net.databinder" %% "dispatch-http" % "0.7.8" excluding("commons-logging" % "commons-logging", "org.apache.httpcomponents" % "httpclient")

However, CacheIvy.moduleIDFormat is broken. It's not apparent because exclusions is the last parameter in ModuleID constructor. But I couldn't figure a nice fix because of the "nine parameters" limitation.

Let me know what you think about it and a suggestion to get around the problem in moduleIDFormat.

Contributor

ijuma commented Sep 4, 2011

Nice! I come across situations where I have to exclude transitive dependencies with relative frequency and this makes it much easier. Looking forward to it. :)

Owner

harrah commented Sep 4, 2011

I'd prefer an sbt type corresponding to ExcludeRule. Although users producing poms will usually want org+name and only org+name, Ivy-only users (like me) want the flexibility to specify one or the other or eventually the other parameters that are allowed.

There is no need to provide support for more than org+name at this time, but it would be best to have sbt.ExcludeRule or whatever to make expansion easier in the future.

Your approach does has the advantage of being able to specify multiple excludes in a varargs list, but I'd rather not mix the use of %, which is used to construct a ModuleID. I'd rather use named/default parameters on excluding.

For the moduleIDFormat issue, nesting the tuples is one way:

wrap[ModuleID, ((String,String,String,Option[String],Boolean),(Boolean,Seq[Artifact],Map[String,String],Boolean))](
m => ((m.organization,m.name,m.revision,m.configurations, m.isChanging), (m.isTransitive, m.explicitArtifacts, m.extraAttributes, m.crossVersion)),
{ case ((o,n,r,cs,ch),(t,as,x,cv)) => ModuleID(o,n,r,cs,ch,t,as,x,cv) }
)

If I'd have known better, I would have better mirrored Ivy's ModuleRevisionId and ModuleId types instead of a single sbt.ModuleID type.

Owner

harrah commented Sep 4, 2011

Also, you may want to link to this on the mailing list if you want feedback from others (especially if you don't like my opinions!).

Member

indrajitr commented Sep 4, 2011

I have huge respect for your opinion, mate! But I'll take your advise and post this to mailing list to have other's opinion.

Member

indrajitr commented Sep 5, 2011

Hi Mark,
Updated to use ExclusionID type instead of using the % trick.

However, I couldn't get CacheIvy to compile and I have no clue :(
If you have some time, can you please have a look at https://github.com/indrajitr/xsbt/tree/dep-exclude to see what's wrong.

Owner

harrah commented Sep 5, 2011

I'd prefer Rule instead of ID, since that more closely corresponds to Ivy's concept. I'll check out the CacheIvy problem now.

Owner

harrah commented Sep 5, 2011

Not sure the best way to do a reverse pull request, so here's the patch. (No, CacheIvy is not exactly a resounding success of scrapping of boilerplate.)

diff --git a/ivy/IvyInterface.scala b/ivy/IvyInterface.scala
index d9ea3a3..bbd3188 100644
--- a/ivy/IvyInterface.scala
+++ b/ivy/IvyInterface.scala
@@ -48,7 +48,7 @@ case class ModuleInfo(nameFormal: String, description: String = "", homepage: Op
    def organization(name: String, home: Option[URL]) = copy(organizationName = name, organizationHomepage = home)
 }
 /** Rule to exclude unwanted dependencies pulled in transitively by a module. */
-case class ExclusionID(organization: String = "*", name: String = "*", artifact: String = "*", configurations: Iterable[String] = Nil)
+case class ExclusionID(organization: String = "*", name: String = "*", artifact: String = "*", configurations: Seq[String] = Nil)
 sealed trait Resolver
 {
    def name: String
diff --git a/main/actions/CacheIvy.scala b/main/actions/CacheIvy.scala
index 88c359a..6ecd7e2 100644
--- a/main/actions/CacheIvy.scala
+++ b/main/actions/CacheIvy.scala
@@ -78,7 +78,9 @@ object CacheIvy
            a => (a.name, a.`type`, a.extension, a.classifier, a.configurations.toSeq, a.url, a.extraAttributes),
            { case (n,t,x,c,cs,u,e) => Artifact(n,t,x,c,cs,u,e) }
        )
-   implicit def moduleIDFormat(implicit sf: Format[String], af: Format[Artifact], bf: Format[Boolean]): Format[ModuleID] =
+   implicit def exclusionIDFormat(implicit sf: Format[String]): Format[ExclusionID] =
+       wrap[ExclusionID, (String, String, String, Seq[String])]( e => (e.organization, e.name, e.artifact, e.configurations), { case (o,n,a,cs) => ExclusionID(o,n,a,cs) })
+   implicit def moduleIDFormat(implicit sf: Format[String], af: Format[Artifact], bf: Format[Boolean], ef: Format[ExclusionID]): Format[ModuleID] =
        wrap[ModuleID, ((String,String,String,Option[String]),(Boolean,Boolean,Seq[Artifact],Seq[ExclusionID],Map[String,String],Boolean))](
            m => ((m.organization,m.name,m.revision,m.configurations), (m.isChanging, m.isTransitive, m.explicitArtifacts, m.exclusions, m.extraAttributes, m.crossVersion)),
            { case ((o,n,r,cs),(ch,t,as,excl,x,cv)) => ModuleID(o,n,r,cs,ch,t,as,excl,x,cv) }
@@ -141,6 +143,7 @@ object CacheIvy
        implicit def sshConnectionToHL = (s: SshConnection) => s.authentication :+: s.hostname :+: s.port :+: HNil

        implicit def artifactToHL = (a: Artifact) => a.name :+: a.`type` :+: a.extension :+: a.classifier :+: names(a.configurations) :+: a.url :+: a.extraAttributes :+: HNil
+       implicit def exclusionToHL = (e: ExclusionID) => e.organization :+: e.name :+: e.artifact :+: e.configurations :+: HNil

 /*     implicit def deliverConfToHL = (p: DeliverConfiguration) => p.deliverIvyPattern :+: p.status :+: p.configurations :+: HNil
        implicit def publishConfToHL = (p: PublishConfiguration) => p.ivyFile :+: p.resolverName :+: p.artifacts :+: HNil*/
@@ -152,6 +155,7 @@ object CacheIvy
    implicit def ivyFileIC: InputCache[IvyFileConfiguration] = wrapIn
    implicit def connectionIC: InputCache[SshConnection] = wrapIn
    implicit def artifactIC: InputCache[Artifact] = wrapIn
+   implicit def exclusionIC: InputCache[ExclusionID] = wrapIn
 /* implicit def publishConfIC: InputCache[PublishConfiguration] = wrapIn
    implicit def deliverConfIC: InputCache[DeliverConfiguration] = wrapIn*/

Owner

harrah commented Sep 5, 2011

Note that I changed Iterable[ExclusionID] to Seq[ExclusionID]. This helps ensure stability across jvm invocations, for example.

Member

indrajitr commented Sep 6, 2011

Thanks much for CacheIvy update. It does stack overflow in my head now :) I'll push another update in a while.

Regarding Iterable[_] vs Seq[_], I usually prefer Seq[_] but in this case retained Iterable[_] from IvyScala.excludeRule. Should we consider having Seq[_] there as well?

Member

indrajitr commented Sep 6, 2011

Getting closer now.

  • Updated CacheIvy
  • ExclusionID is now ExclusionRule

Let's iron out all the points first. Once done, I'll do a rebase with master and put a single commit for merge.

Owner

harrah commented Sep 6, 2011

Looks good to me. Not sure about the best way to avoid the slightly verbose excluding(ExcludeRule(...)) though. Yes, I think IvyScala.excludeRule should be Seq as well.

Member

indrajitr commented Sep 6, 2011

Exactly! excluding(ExcludeRule(...), ExcludeRule(...), ...) is too noisy. excluding was going well when we had % notation. But not anymore. Let's give it a while and think about it.

Good, there are quite a few certain cases of Iterable -> Seq translation (which I always wondered about). I'll take the obvious ones in one go and send a separate patch.

Member

indrajitr commented Sep 6, 2011

A quick thought:

  • Rename the method excluding to exclude
  • For simple cases when just the (org, name) pairs are to be provided, have exclude(("commons-logging", "commons-logging"), ("...", "...")). The Pairs to be converted to Seq[ExclusionRule] internally with method overload.
Member

indrajitr commented Sep 7, 2011

To expand on the previous comment, I'm thinking about something like this. Thoughts?

def exclude(exclRules: ExclusionRule*): ModuleID = copy(exclusions = this.exclusions ++ exclRules)
def exclude(exclPairs: (String,String)*)(implicit d: DummyImplicit): ModuleID = exclude(exclPairs.map(e => ExclusionRule(e._1, e._2)):_*)

Obviously the implicit is an uninvited guest to work around type erasure. This would allow both exclude(ExcludeRule(...)), exclude('org' -> 'name') either via varargs or using multiple exclude() clauses.

Owner

harrah commented Sep 8, 2011

I don't know that overloading is worth it here. Another possibility:

def excludeAll( rules: ExclusionRule*): ModuleID
def exclude(org: String, name: String): ModuleID

...exclude(org = "example", name = "demo").exclude("example", "demo2")
Member

indrajitr commented Sep 8, 2011

Don't you think def excludeAll (with All suffix) would be inconsistent with def artifacts which also takes vararg?

Since excludes is not plural of exclude, Another option could be:

def exclusions(rules: ExcludeRule*): ModuleID
def exclusion(org: String, name: String): ModuleID

Plural form intuitively takes varargs (and is consistent with def artifacts) and singular form is the simpler form.

Or alternately:

def exclusions(rules: ExcludeRule*): ModuleID
def exclude(org: String, name: String): ModuleID

They would be different enough to avoid typo in config.

Member

indrajitr commented Sep 9, 2011

Hey Mark,

Let me know your thought, and I'll make one final rebase and submit.

Owner

harrah commented Sep 9, 2011

Well, artifacts is a noun and excludeAll is an action, but I don't care much either way. I think that because the method accepting Strings is likely to be the common method, it is best to use exclude for that.

Member

indrajitr commented Sep 9, 2011

Okay, excludeAll( rules: ExclusionRule*): ModuleID and exclude(org: String, name: String): ModuleID stands :)

@indrajitr indrajitr Support for simple exclusion rules in inline dependencies
This support excluding a library from the dependency tree for a given
set of `ExclusionRule`s. There are two ways to achieve this:

- Using `organization` and `name` pairs:
val dep = "org" % "name" % "version" exclude("commons-codec", "commons-codec") exclude("org.slf4j", "slf4j-log4j")

- Using `ExclusionRule`:
val dep = "org" % "name" % "version" excludeAll(ExclusionRule("commons-codec", "commons-codec"), ExclusionRule("org.slf4j", "slf4j-log4j"))
bd8d1c0
Member

indrajitr commented Sep 9, 2011

All set for merge.

@harrah harrah added a commit that referenced this pull request Sep 10, 2011

@harrah harrah Merge pull request #169 from indrajitr/dep-exclude
Basic support for exclusions in inline dependencies
abcd9a0

@harrah harrah merged commit abcd9a0 into sbt:0.11 Sep 10, 2011

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment