Migrate to sbt-typelevel #4160

armanbilge · 2022-04-04T03:28:06Z

Closes #4143. Fixes #4062. Fixes #4149.

Things seem to be working locally. But the battle with CI is just beginning.

Note this PR doesn't deal with the documentation/site at all (they are currently not being checked in CI). I'll prepare that in a follow-up for easier review, although they should probably merge together.

armanbilge · 2022-04-04T03:32:53Z

project/plugins.sbt

+val sbtTypelevelVersion = "0.4.7"
+addSbtPlugin("org.typelevel" % "sbt-typelevel-ci-release" % sbtTypelevelVersion)
+addSbtPlugin("org.typelevel" % "sbt-typelevel-settings" % sbtTypelevelVersion)
+addSbtPlugin("org.typelevel" % "sbt-typelevel-site" % sbtTypelevelVersion)


I forgot to mention: in the end, I didn't actually migrate to sbt-typelevel since that would require adding copyright headers to all the files. Instead we use a combination of sbt-typelevel-ci-release and sbt-typelevel-settings. So the build still has a little more boilerplate than it could, but it's okay.

I don't see a particular reason not to bite the bullet and add the headers, though it would be nice to get this substantial work to a landing.

Yep, super easy to make that change. Mostly it just creates a massive diff :)

Also, I wasn't completely certain what to make of Cats' COPYING which includes a Scalaz notice as well. E.g. whether it's appropriate to put a blanket license over every file.

In http4s, we generate our copyright and also preserve the original copyright on derived files. This is a fairly common practice, and one that doesn't irritate the tooling.

Right, http4s has been very diligent about that which helped. But AFAICT not a single file in Cats has a copyright header, whether its derived or not.

Oh, I see. I don't know how we'd go back and add that now. I say we made a good faith effort with the blanket COPYING file, and I'd be happy to add a second header if anyone cares enough to point out the derived works.

armanbilge · 2022-04-04T03:34:27Z

core/src/main/scala/cats/Invariant.scala

+  @deprecated("Added for bincompat", "2.8.0")
+  @cats.compat.targetName("catsInstancesForId")
+  def catsInstancesForIdCompat2_6_1: Comonad[Id] =
+    cats.catsInstancesForId


This is the fix for #4062. Added @targetName as a dummy annotation in Scala 2 so we don't have to split this file (I tried compat traits first and it didn't work).

armanbilge · 2022-04-04T03:35:24Z

core/src/main/scala/cats/instances/StaticMethods.scala

-private[instances] object instances {
+private[instances] object StaticMethods {


Fixes #4149.

armanbilge · 2022-04-04T03:36:26Z

mima.sbt

@@ -0,0 +1,166 @@
+ThisBuild / mimaBinaryIssueFilters ++= {


I moved these to their own file so they don't clutter up the build.sbt.

armanbilge · 2022-04-04T03:38:03Z

build.sbt

+  Compile / packageSrc / mappings ++= {
+    val base = (Compile / sourceManaged).value
+    (Compile / managedSources).value.map { file =>
+      file -> file.relativeTo(base).get.getPath
+    }
+  },


I'd like to upstream this in typelevel/sbt-typelevel#239.

armanbilge · 2022-04-04T03:40:40Z

build.sbt

+      file -> file.relativeTo(base).get.getPath
+    }
+  },
+  scalacOptions ~= { _.filterNot(x => x.startsWith("-Wunused:")) },


This was the most disappointing thing. Seems that Cats has got lots and lots of unused warnings. So we have to disable them if we want fatal warnings for everything else. It would truly be a great thing if someone could chase down and at least @nowarn them if they are legit or necessary for bincompat. Then at least we can avoid adding more unused stuff.

Seems that Scala 3.0.2 ignores any @nowarn annotations, unfortunately. I found it out while working on #4083.

Yes, that's right. But @nowarn is supported in Scala 3.1 and I believe we should bump for that and other reasons. See:

Drop Scala 3.0, publish for 3.1? scalacheck#878

We won't be getting any "unused" warnings anymore after this change, is that correct?

That's right, no more unused warnings :(

It's a good question. So IIUC, you mean enable all warnings, but disable fatal warnings?

The only problem is that there are so many unused warnings they drown out everything else maybe.

Yes, I also hate seeing warning:) This is why I attempted to make some relief in #4083. And I would be glad to continue working on that.

Answering your question – yes, I think seeing warnings is still better than suppressing them completely.

Yes, now that I came face-to-face with the problem myself, I truly appreciate your heroic efforts in #4083 :)

Ok, that's fair. I'll restore all the default compiler flags from sbt-typelevel, but disable fatal warnings.

Btw: there're also loads of deprecation warnings in some modules (which mostly don't make any real sense). And there're so many of them that it is quite easy to miss something really important.

Right, that's exactly my concern. The only way to be sure you don't miss warnings, is to make them fatal. But that means we have to disable the warnings that we can't solve today.

armanbilge · 2022-04-04T03:51:31Z

build.sbt

+lazy val unidocs = project
+  .enablePlugins(TypelevelUnidocPlugin)
+  .settings(
+    name := "cats-docs",
+    ScalaUnidoc / unidoc / unidocProjectFilter := inProjects(kernel.jvm,
+                                                             core.jvm,
+                                                             free.jvm,
+                                                             algebra.jvm,
+                                                             alleycatsCore.jvm
+    ),
+    scalacOptions ~= { _.filterNot(_.startsWith("-W")) }, // weird nsc bug
+    ScalaUnidoc / unidoc / scalacOptions ++= Seq("-groups", "-diagrams")
+  )
+  .settings(commonSettings)


This introduces a new API-docs only cats-docs module that lets us publish unidocs to Sonatype/Maven. Then they can be browsed at javadoc.io.

Example:
https://www.javadoc.io/doc/org.http4s/http4s-docs_2.13/0.23.11/org/http4s/index.html

armanbilge · 2022-04-04T04:33:55Z

mima.sbt

+// TODO: remove these filters after MiMa 1.1.0 is released with improved support for Scala 3
+ThisBuild / mimaBinaryIssueFilters ++= {


@dwijnand has very kindly fixed this in lightbend-labs/mima#683 :)

Sorry I've not cut a release yet - life things shuffled my priorities the last 3 weeks.

No worries, just means we'll appreciate it even more when it arrives! :)

armanbilge · 2022-04-08T04:23:29Z

Yep, rolled out 16k lines of headers 😬 Also applied Sergey's suggestion to keep all the warnings enabled, non-fatally. Merged in my separate site PR.

Oh, I need to set a proper start year for the headers ...

satorg · 2022-04-08T05:13:55Z

rolled out 16k lines of headers

915 files changed versus ~140 before that.. Seems that Git has lost tracks of small files after they received the headers..

What are the key files for the review? I doubt if I can make all 915 :)

armanbilge · 2022-04-08T05:17:07Z

build.sbt is really the key file. But honestly sbt is hard to understand for a good review :) best way to review IMO is to interact with the sbt console. Check that settings are configured the way you expect. Check that MiMa previous versions includes what it should (make sure tags on your local are updated). Try previewing the website with docs/tlSitePreview. Stuff like that :)

armanbilge · 2022-04-09T23:09:06Z

Any thoughts on landing this one? 🙏 I need to update this PR to account for the just-merged #4166 ...

satorg · 2022-04-10T02:35:29Z

Any thoughts on landing this one?

Agree. I would vote to hold off other merges until we get this landed.

Regarding the merging of this one.... I found it tremendously difficult to reason about the safety of the changes made. They look good to me, but I realize that I cannot follow everything here.

So... What about scheduling an "RC" release after merging this PR? E.g. "2.8.0-RC1" or something. Just to let everyone (who would like to participate) test the result artifacts for potential BinCompat issues first. Would it make sense?

armanbilge · 2022-04-10T02:46:57Z

Sure, I'm 👍 to publishing Ms, RCs, etc. :) actually, as soon as this merges it should immediately publish a snapshot if all goes well.

Just to let everyone (who would like to participate) test the result artifacts for potential BinCompat issues first. Would it make sense?

Depends on what you want to test :) if you want to test the fix for #4062, for sure, that makes sense.

If you want to test whether the build will prevent the publishing of binary-breaking changes, then we'd have to try and introduce one first :) because besides the fix for #4062 this doesn't really change any code. I.e. everything should be 100% binary compatible, because it already was.

satorg

I think we should go ahead and merge it.

armanbilge · 2022-04-10T10:41:33Z

https://s01.oss.sonatype.org/content/repositories/snapshots/org/typelevel/cats-core_3/

satorg · 2022-04-11T07:11:48Z

@armanbilge now when I run sbt +prePR on main I receive a lot of errors like these below:

[error] /Users/satorg/projects/typelevel/cats/algebra-laws/shared/src/test/scala/algebra/laws/LawTests.scala:74:23: object DeMorganLaws in package laws is deprecated (since 2.7.0): Laws moved to LogicLaws
[error]   checkAll("Boolean", DeMorganLaws[Boolean].deMorgan(DeMorgan.fromBool(Bool[Boolean])))
[error]                       ^
[error] /Users/satorg/projects/typelevel/cats/algebra-laws/shared/src/test/scala/algebra/laws/LawTests.scala:189:26: object GroupLaws in package laws is deprecated (since 2.7.0): Provided by cats.kernel.laws
[error]     checkAll("Order[N]", GroupLaws[Order[N]].monoid)
[error]                          ^
[error] /Users/satorg/projects/typelevel/cats/algebra-laws/shared/src/test/scala/algebra/laws/LawTests.scala:193:25: object GroupLaws in package laws is deprecated (since 2.7.0): Provided by cats.kernel.laws
[error]       checkAll("Eq[N]", GroupLaws[Eq[N]].boundedSemilattice)
[error]                         ^
[error] /Users/satorg/projects/typelevel/cats/algebra-laws/shared/src/test/scala/algebra/laws/LawTests.scala:197:25: object GroupLaws in package laws is deprecated (since 2.7.0): Provided by cats.kernel.laws
[error]       checkAll("Eq[N]", GroupLaws[Eq[N]].semilattice)
[error]                         ^

Just to clarify: is it supposed to run this way?

armanbilge · 2022-04-11T10:57:58Z

@satorg hmm, no, that's a bug 😕 try setting ThisBuild / tlFatalWarningsInCi := false.

satorg · 2022-04-13T20:57:52Z

Unfortunately, changing values of either tlFatalWarnings or tlFatalWarningsInCi from a command line does not work since prePR reloads the project before starting doing everything else....

armanbilge · 2022-04-13T20:59:51Z

Sorry, I meant we should permanently add it to build.sbt. It's my fault for not setting this up right to begin with :)

satorg · 2022-04-13T21:18:23Z

No worries. I can go ahead and file a PR for that.

satorg · 2022-04-14T17:55:42Z

@armanbilge although it is merged now... But seems I missed it while was reviewing.

cats/build.sbt

Line 26 in fe40bc2

ThisBuild / scalaVersion := Scala212

It sets the default Scala version to 2.12, which is different from how it was before (2.13).
I wonder was it made for some specific reason? Because to me it seems less convenient since requires switching versions manually more often while compiling from SBT command line. Moreover, Metals will use 2.12 by default to generate Bloop files.

armanbilge · 2022-04-14T18:00:54Z

@satorg I agree, 2.13 makes more sense. I changed it to 2.12 in ce873ff because mdoc wasn't working in 2.13, since the sbt-typelevel-site plugin uses the default Scala for building/deploying documentation. In principle, we can hack around this in the build ... but just like fatal warnings, best option is to fix the root problem. I.e. get the docs building on Scala 2.13. We can make an issue for it and a contributor may be interested to work on it.

satorg · 2022-04-14T18:33:25Z

Ah I see – it was for the reason.
Yeah it is definitely worth the fixing. I'm not sure if it is going to be a "good first issue" though... :)

armanbilge added 18 commits April 3, 2022 16:15

Well, the build.sbt compiles

499f288

Regenerate workflow

ec57223

Consolidation

56db9ab

Fix compile errors

b30190e

Disable doctests on foreign platforms

809fe50

Fix js stuff

b302d04

Downgrade to sbt-typelevel-ci-release

05a58ca

Setup unidocs project

4afb353

Add organization settings

8b650e6

Formatting

9c8a013

Disable unused warnings

0de5744

Disable unused warnings, except imports :(

1899e0f

Fix bincompat

fb08e89

Give up on unused warnings

f4e33f8

Disable fatal warnings for scaladoc

1c68ec2

Add some scaladoc flags

d9ee77e

Fix typelevel#4149

fd958d9

Fix unidocs

7d4bf21

armanbilge commented Apr 4, 2022

View reviewed changes

Make workflow definition independent of ++

205d0cc

armanbilge commented Apr 4, 2022

View reviewed changes

armanbilge added 2 commits April 4, 2022 03:54

Use latest munit

29b8c7b

Add Scala 3 only mima filters

2306446

armanbilge commented Apr 4, 2022

View reviewed changes

Fix binCompatTest on Scala 3

00e2efd

armanbilge dismissed rossabaker’s stale review via 4085701 April 8, 2022 04:20

armanbilge added 2 commits April 8, 2022 04:28

Cats is 7 years old :')

71d97d4

scaladoc bugs ...

2c16802

armanbilge added 2 commits April 9, 2022 23:09

Merge remote-tracking branch 'upstream/main' into topic/sbt-typelevel

ba5e479

More headers

0bda83a

satorg approved these changes Apr 10, 2022

View reviewed changes

armanbilge merged commit 708cbab into typelevel:main Apr 10, 2022

armanbilge mentioned this pull request Apr 11, 2022

Rethink tlFatalWarningsInCi typelevel/sbt-typelevel#254

Closed

satorg mentioned this pull request Apr 14, 2022

temporarily disable tlFatalWarningsInCi #4186

Merged

armanbilge mentioned this pull request Jun 22, 2022

Build docs on 2.13 #4238

Merged

		private[instances] object instances {
		private[instances] object StaticMethods {

		// TODO: remove these filters after MiMa 1.1.0 is released with improved support for Scala 3
		ThisBuild / mimaBinaryIssueFilters ++= {

Migrate to sbt-typelevel #4160

Migrate to sbt-typelevel #4160

Conversation

armanbilge commented Apr 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

armanbilge Apr 4, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

armanbilge commented Apr 8, 2022 • edited

satorg commented Apr 8, 2022 • edited

armanbilge commented Apr 8, 2022

armanbilge commented Apr 9, 2022

satorg commented Apr 10, 2022

armanbilge commented Apr 10, 2022 • edited

satorg left a comment • edited

Choose a reason for hiding this comment

armanbilge commented Apr 10, 2022

satorg commented Apr 11, 2022

armanbilge commented Apr 11, 2022

satorg commented Apr 13, 2022 • edited

armanbilge commented Apr 13, 2022

satorg commented Apr 13, 2022

satorg commented Apr 14, 2022 • edited

armanbilge commented Apr 14, 2022

satorg commented Apr 14, 2022

armanbilge Apr 4, 2022 •

edited

armanbilge commented Apr 8, 2022 •

edited

satorg commented Apr 8, 2022 •

edited

armanbilge commented Apr 10, 2022 •

edited

satorg left a comment •

edited

satorg commented Apr 13, 2022 •

edited

satorg commented Apr 14, 2022 •

edited