Another attempt at Dotty cross-building #3486
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3486 +/- ##
=======================================
Coverage 91.77% 91.77%
=======================================
Files 383 383
Lines 8394 8394
Branches 205 215 +10
=======================================
Hits 7704 7704
Misses 690 690 |
| @@ -1,6 +1,8 @@ | |||
| package cats.laws | |||
|
|
|||
| import cats.{Apply, Id, NonEmptyTraverse, Semigroup} | |||
| // The `catsInstancesForId` import is necessary to work around a Dotty | |||
| // issue related to https://github.com/lampepfl/dotty/issues/9067. | |||
There was a problem hiding this comment.
The issue seems resolved in 0.25.0's release candidates. I guess we like to stay on stable versions ? I'm just curious.
There was a problem hiding this comment.
@barambani I thought about using 0.25.0-RC2, and I don't think we should avoid the Dotty RCs on principle, but the 3.2.0 ScalaTest dependencies are currently only published for 0.24.0, and I think it's more important to make it easy for people to work on the test issues.
There was a problem hiding this comment.
In general, I think the right thing for us (and the whole ecosystem) to do would be to publish for the latest stable Dotty (in this case, 0.24.0) as well as the exact latest RC (in this case, 0.25.0-RC2). Obviously it's a pain keeping up with RCs, but it helps quite a bit. Dependencies which don't do this, particularly those which lack macro usage, effectively force their downstream dependents to fall back on the 2.13 cross-publish, which is annoying.
Obviously though, in our case, we can't really do anything that ScalaTest doesn't do, so our hands are tied.
| if (isDotty.value) Nil else Seq(s"-P:semanticdb:targetroot:${baseDirectory.value}/target/.semanticdb", "-Yrangepos") | ||
| ), | ||
| libraryDependencies += | ||
| ("org.typelevel" %% "simulacrum-scalafix-annotations" % "0.5.0").withDottyCompat(scalaVersion.value), |
There was a problem hiding this comment.
Does this have to be in the runtime classpath?
There was a problem hiding this comment.
Hmm, this isn't new to this PR, but it's a good point—I'll change this to Provided before 2.2.0, and make sure that we're doing the same thing with Simulacrum Scalafix as we were with Simulacrum (the macro annotation version) in this respect. But I think that should be a follow-up.
| libraryDependencies ++= Seq( | ||
| ("org.typelevel" %%% "discipline-scalatest" % disciplineScalatestVersion % Test) | ||
| ).map( | ||
| _.exclude("org.scalatestplus", "scalacheck-1-14_2.13") | ||
| .exclude("org.scalactic", "scalactic_2.13") | ||
| .exclude("org.scalatest", "scalatest_2.13") | ||
| .withDottyCompat(scalaVersion.value) |
There was a problem hiding this comment.
I'm tentatively okay with merging this as-is, but only if we follow up on discipline-scalatest asap to update its dependency. Exclusion rules in published dependencies are dangerous on several levels.
There was a problem hiding this comment.
What do you think about changing the exclusion to only apply for Dotty (which we’re not currently publishing anyway)? I think it’s likely I’ll be the one publishing discipline-scalatest, and I don’t really want to start doing that for every Dotty release since it’s just a test dependency.
There was a problem hiding this comment.
@djspiewak Oh, nevermind, I forgot that this is only a test dependency (and that's configured here, so there's no danger of them accidentally missing an % Test at the use site), so I don't see any problem with keeping the exclusions as they are now.
There was a problem hiding this comment.
Ah if it's only a test dependency then I'm okay with it for now.
| @@ -773,29 +810,33 @@ lazy val crossVersionSharedSources: Seq[Setting[_]] = | |||
| } | |||
| } | |||
|
|
|||
| def commonScalacOptions(scalaVersion: String) = | |||
| def commonScalacOptions(scalaVersion: String, isDotty: Boolean) = | |||
There was a problem hiding this comment.
As an fyi, the isDotty setting is computed by string matching on scalaVersion. Though, it's not exposed as such, so there may be no way we can take advantage of it.
There was a problem hiding this comment.
Right, I just thought this was clearer than trying to figure out and replicate whatever logic sbt-dotty uses for isDotty, but I have no objection to using .startsWith("0") or something similar instead.
This PR partially includes and supersedes #3269. It also depends on #3484, which is ~trivial and should be merged soon.
There are a lot of issues with building and running our tests on Dotty, and I don't have the interest or will to deal with them right now, but I don't think that should block getting the core modules cross-building now. This PR enables Dotty cross-building (on the JVM only) for kernel, kernel-laws, core, laws, alleycats, and alleycats-laws, but does not run any tests.
Merging this now gets us a couple of things:
None of the changes here should have any impact on the Scala 2 build, except for the introduction of a package-private
FunctionKMacroMethodstrait.Note that I've disabled fatal warnings (for Dotty only) because of two issues:
ContTthat I think I investigated once and maybe even reported, but can't find now.