typelevel / general

Repository for general Typelevel information, activity and issues
19 stars 8 forks source link

A typelevel sbt plugin for unifying the partial unification setting #69

Closed kailuowang closed 7 years ago

kailuowang commented 7 years ago

There are several typelevel projects that need to manage user experience around SI-2712. Currently, there is fiadliel/sbt-partial-unification which tries to provide an easy way to enable partial unification for Scala projects, i.e. set the scalacOption for 2.11.10+ and use the s2712 fix plugin by @milessabin for 2.10.8 and 2.11.8. However this plugin has some issues and the owner didn't respond to a PR that address them. Shall we start a new sbt plugin under typelevel to provide the same thing? If so, shall we consider making it part of sbt-catalysts or a new repo?

related issue: https://github.com/typelevel/cats/pull/1583

milessabin commented 7 years ago

Surely this is just a handful of lines of goop in a build.sbt isn't it? Is a plugin really worth the effort?

kailuowang commented 7 years ago

It is just a handful lines of code. But since we are going to put this burden onto users, I still think there is a meaningful difference between copying a handful of code and one line of addSbtPlugin. @non, since you originally suggested a plugin here, do you have any input on this?

tpolecat commented 7 years ago

It's non-obvious goop and I recall being enraged at some point, but yeah maybe a plugin isn't worth the effort since the doc for it would have to explain most of the implementation. A blog post on how to do it might be as effective.

milessabin commented 7 years ago

Is this just for support for 2.10.x? If it is then we should reconsider https://github.com/typelevel/scala/issues/122.

kailuowang commented 7 years ago

For the most part. But also this plugin helps projects that cross build. Without a plugin, our projects have to include a documentation more or less like the following:

This library will be easier to use if you turn on partial unification for your scalac. If you are on 2.10.6 please use this plugin in your plugins.sbt

addCompilerPlugin("com.milessabin" % "si2712fix-plugin_2.10.6" % "1.2.0")

If you are on 2.11.x please make sure you upgrade to 2.11.10+ and enable this scalac flag

scalacOptions += "-Ypartial-unification"

If you are on 2.12.x pelase enabled the flag as above.

if you are cross building you would need these lines of code goop

  def pluginOrFlag(scalaVersion: String, pluginModule: ModuleID): Either[ModuleID, String] = {
    val pluginDependency = Left(compilerPlugin(pluginModule cross CrossVersion.full))
    val flag = Right("-Ypartial-unification")
    val VersionNumber((major +: minor +: patch +: _), _, _) = scalaVersion

    (major, minor, patch) match {
      case (2, 10, p) if p >= 6 => pluginDependency
      case (2, 11, 8) => pluginDependency
      case (2, 11, p) if p >= 9 => flag
      case (2, 12, _) => flag
      case (2, 13, _) => flag
      case _ => sys.error(s"scala version $scalaVersion is not supported.")
    }
  }

libraryDependencies ++= pluginOrFlag(scalaVersion.value, partialUnificationModule.value).left.toSeq,
    scalacOptions ++= pluginOrFlag(scalaVersion.value, partialUnificationModule.value).right.toSeq

<<<<<<<<<<<<<<<<<<<<<<<<<<<

I think that's a lot for the users to consume.

tpolecat commented 7 years ago

Hm, yeah that's a lot of goop. I switch my vote to 👍

adelbertc commented 7 years ago

👍 from me

milessabin commented 7 years ago

Very disappointing to see so much enthusiasm for this and so little for TLS for 2.10.x.

kailuowang commented 7 years ago

@milessabin from my part, I need this for pushing through for cats 1.0.0-RC1, this sounds an easier and quicker solution than TLS 2.10.x. Not saying that we shouldn't pursue TLS 2.10.x

tpolecat commented 7 years ago

I guess I see this as a way for 2.10 projects to limp along a bit longer, as opposed to TLS 2.10 which seems like a bigger and more long-term commitment. I want my reality to be rid of 2.10 by the end of the year (if not earlier) so quick fixes don't bother me.

fiadliel commented 7 years ago

That SBT project was somehow marked as "not watching", so didn't see the change.

I've merged those changes. I previously had seen limited interest in the plugin, but I can either keep it updated, or you can do whatever you want with a fork.

I'm also very much open to whatever seems reasonable with adding some level of support for TLS - at the very least, it should do the right thing if scalaOrganization is modified; it could in theory modify scalaOrganization itself, though it increases the feature profile, and is more likely to conflict with some other setting that people want in their builds.

kailuowang commented 7 years ago

Thanks very much for merging and releasing @fiadliel. I'd rather continue using yours than forking. I am going to close this issue.

Blaisorblade commented 7 years ago

TL;DR: Use https://github.com/fiadliel/sbt-partial-unification. Opened https://github.com/typelevel/cats/pull/1699 to document it.