Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate build to sbt-typelevel #4143

Closed
armanbilge opened this issue Mar 7, 2022 · 18 comments · Fixed by #4160
Closed

Migrate build to sbt-typelevel #4143

armanbilge opened this issue Mar 7, 2022 · 18 comments · Fixed by #4160

Comments

@armanbilge
Copy link
Member

armanbilge commented Mar 7, 2022

@djspiewak asked me about this on Discord and I've had this on my mind. Cats is the last flagship project without a PR on typelevel/sbt-typelevel#83. Here's the plan that's been brewing.

  1. This is going to be a big and careful effort, but my intention is to completely overhaul and modernize the cats build. If there's no objections, I'd like to enable CI snapshots and CI release.

  2. I'd like to target this at a series/2.7 branch. Because sbt-typelevel enables MiMa for Scala 3, we will have to deal with cats 2.7.0 broke binary compatibility for Scala 3 (because mima wasn't run) #4062.

    IMHO the best way forward is to release 2.7.1 which patches that one binary-break, and declare 2.7.0 "cursed". Other options I considered:

    • Fix it in 2.8.0 and add a MiMa filter for the method. But this MiMa filter will chase us to the end of series/2.x and we will no longer get MiMa support to catch this mistake, which is bad.
    • Fix it in 2.8.0 and don't check bincompat at all against 2.7.0. But then who knows what other binary issues could fall through the cracks.
    • Fix it in 2.8.0 by splitting/duplicate the entirety of cats/package.scala across Scala 2/3 so we can apply new Scala 3-only techniques to fix it while also retaining bincompat to the cursed 2.7.0. As proposed by @smarter in:
      Enable MiMa for Scala 3 #4063 (comment)

    So would love to get ideas here, but to me a 2.7.1 patch release fixing this one thing is the safest and most convenient way forward.

  3. It doesn't have to be/won't be part of the initial migration PR, but I'd like to move the Cats site onto the sbt-typelevel-site plugin which uses Laika. Comments:

There isn't an immediate urgency for this. With Scala 3.1.2 on the horizon with the -Yscala-release flag and pressure building to release with Scala 3+Native support, I'd like to get this done before those changes ripple through.

Thanks all, would appreciate any and all input.

@johnynek
Copy link
Contributor

johnynek commented Mar 7, 2022

Just want to link to the discussion here:

#4016

I guess the plan is that sbt-typelevel can support the scala-release flag and we will set that.

An alternate approach would be to simply have CI run two scala 3 builds (also set up in sbt-typelevel): one for the publishing version of scala 3, one for the latest version of scala 3 (maybe even have a third one where we build with the old version of scala 3, but run the tests with the new version of scala 3 linked against the built version).

@djspiewak
Copy link
Member

@johnynek The other option is we just upgrade. :-) It's worth noting that Fs2 has been publishing for Scala 3.1.x for a while now, meaning that most of the practical swathe of the ecosystem has been tied to that version. Cats Effect will probably upgrade in 3.4 or 3.5. The scala-release flag is good if it works and we trust it, but I think it would be naive to claim that it's going to be perfect out of the box, and it's fair to have a discussion about the tradeoffs all around.

@johnynek
Copy link
Contributor

johnynek commented Mar 7, 2022

I think just upgrading is fine early, when almost no one is running scala 3, but later it will be the same issue we have with scala 2.

e.g. I use scala 2.11 at work, and if I work at Netflix for a good long time I might be able to help get us all the way up to scala 2.12.

So, if scala-release works great, then that's fine, but I think we should be publishing for a very conservative scala 3 version to maximize compatibility (if we want broad adoption, which isn't always a major goal).

@smarter
Copy link
Contributor

smarter commented Mar 7, 2022

The scala-release flag is good if it works and we trust it

For the record, this flag has been built for the express purpose of cats and other libraries taking a stance that they wouldn't upgrade without something like it. It's going to be a huge maintenance burden on the compiler and will make it harder for us to evolve the language, so as I mentioned in #4016 (comment) it'd be great to get some feedback on whether this is actually something the community will use before we commit ourselves to maintaining it ad infinitum (so far most of the discussion has just been bikeshedding on the option name, and seemingly no one has actually tried using the flag: https://contributors.scala-lang.org/t/improving-scala-3-forward-compatibility/5298).

@armanbilge
Copy link
Member Author

Welp, not the topic I had in mind for this issue, but while we're here, here's some of my thinking.

We should probably just upgrade to 3.1

It's the first Scala 3 version supported on JVM, JS, Native. It's still early days for Scala 3, and a lot of the ecosystem upgraded by means of fs2. So, yeah.

All the proposed solutions require some form of "cross-building"

The original proposal was to cross-compile on Scala 3.0 and Scala 3.1 and only publish the 3.0 artifact. But this is annoying to setup in your build config and it ended up breaking the community build in scala/scala3#14374 (comment).

Ok, but now we have -Yscala-release: thanks for the big effort to put it together.

What's kept me from trying the -Yscala-release flag so far is two things:

  • You have to hack the pom to replace the 3.1 stdlib with the 3.0 stdlib
  • You still have to run tests in 3.0 against artifacts compiled with 3.1.

Like ... I have no idea how to do that off the top of my head.

At this point, why don't we just cross-build and publish as usual?

E.g. _3.0, _3.1, etc. It's not as bad as it used to be: if your dependency has only published on older Scala 3s then you can always do for3_1Use3_0 or something. AFAICT Binary compatibility guarantees and eviction should keep this working fine. The trick is that your _3.1 artifacts should also check binary compatibility against your _3.0 artifacts in addition to the _3.1 artifacts. But that's super easy to set up actually.

Plus, doesn't this make it easier for the compiler team to evolve the language?

@smarter
Copy link
Contributor

smarter commented Mar 7, 2022

You have to hack the pom to replace the 3.1 stdlib with the 3.0 stdlib

Will be solved by sbt/sbt#6814

You still have to run tests in 3.0 against artifacts compiled with 3.1.

I brought this up but should have clarified: this is the same situation you are in when you're publishing against 2.12.14 but pretending you support 2.12.0: your library might not actually work properly because of inference differences or plain bugs. People deal with this by either not caring about or explicitly not supporting old versions. That was my point: I don't think sbt-typelevel should default to supporting Scala 3.0 because each project will have their own minimal supported version, just like you shouldn't commit to supporting Scala 2.12.0 by default.

Plus, doesn't this make it easier for the compiler team to evolve the language?

Not really, users should be able to add a library using just %%, not hunt down which combination of _3.x suffixes work.

@armanbilge
Copy link
Member Author

armanbilge commented Mar 7, 2022

Thanks, those are helpful clarifications.

not hunt down which combination of _3.x suffixes work.

Pretty sure tooling/coursier could do this automatically. (Edit: well, not too automatically because then we need lockfiles :) maybe not such a good idea...)

It's going to be a huge maintenance burden on the compiler and will make it harder for us to evolve the language

So, then I'm very confused what does relieve that burden?

@smarter
Copy link
Contributor

smarter commented Mar 7, 2022

Not sure I understand the question, nothing relieves the burden, it's just endless pain :). It's just that we're willing to take on more pain by having a flag to support forward-compatibility if that's what it takes to get cats to upgrade, but if cats doesn't use the flag, then this is pointless, so it's important for us to know if cats is actually going to use the flag it said it wanted (#4016 (comment)).

@smarter
Copy link
Contributor

smarter commented Mar 7, 2022

Also, apologies for hijacking this thread, just reacted since I was @-ed, but further discussions should probably go to https://contributors.scala-lang.org/t/improving-scala-3-forward-compatibility/5298

@djspiewak
Copy link
Member

@smarter I'm happy to start trying it. I do think though that some of the ecosystem transitivity on this has diminished somewhat due to Fs2's upgrade to 3.1. There are also a few other upgrades around the ecosystem which have forced things transitively (iirc some testing libraries), so in other words the halo of this is shallower than it would have been previously.

Now with that said, there's going to be a 3.2 someday, and we don't want to relitigate this whole mess, so I definitely think the option is a good thing, and we should try to get some confidence in it now. What I was inelegantly putting forward earlier was just the reduction of effective blast radius because of Fs2.

Appreciate the chime-in though. Really helpful thoughts and context.

@mpilquist
Copy link
Member

We need a hexagonal logo for team upgrade! :)

@smarter
Copy link
Contributor

smarter commented Mar 8, 2022

there's going to be a 3.2 someday

Yes, current timeline is 3.2.0-RC1 in 7 weeks (due to delay in the current milestone) which means 3.2.0 would land 6 weeks later: https://github.com/lampepfl/dotty/milestones

@djspiewak
Copy link
Member

Exactly. So I think ensuring we're in a good place with respect to release targeting before that point would be a good use of time.

@armanbilge
Copy link
Member Author

Ok, going to try and do this over the weekend. In case anyone has some additional thoughts about my plan in #4143 (comment).

@smarter
Copy link
Contributor

smarter commented Apr 1, 2022

IMHO the best way forward is to release 2.7.1 which patches that one binary-break, and declare 2.7.0 "cursed". Other options I considered: [...]
Fix it in 2.8.0 by splitting/duplicate the entirety of cats/package.scala across Scala 2/3 so we can apply new Scala 3-only techniques to fix it while also retaining bincompat to the cursed 2.7.0. As proposed by @smarter in:
#4063 (comment)

I'm still a proponent of this approach, especially given how long it's been since 2.7.0 has been released.

@armanbilge
Copy link
Member Author

@smarter thanks for your input. I definitely agree about that. And looking again, maybe duplicating that file is not so bad as I thought.

However, looking again I am confused how your proposal works. Don't both the 2.6.1 and 2.7.0 compat methods have the same binary name, so they will conflict?

import scala.annotation.targetName
// ...
// 2.6.1 compat
private[cats] def catsInstancesForId: Distributive[Id] with Comonad[Id] = ...
// 2.7.0 compat
private[cats] @targetName("catsInstancesForId") def catsInstancesForIdCompat270:
     : Distributive[Id] with Bimonad[Id] with CommutativeMonad[Id] with NonEmptyTraverse[Id] = ...
// new method
implicit def catsInstancesForIdBinCompat1
    : Distributive[Id] with Bimonad[Id] with CommutativeMonad[Id] with NonEmptyTraverse[Id] = ...

@smarter
Copy link
Contributor

smarter commented Apr 1, 2022

they have the same name but different bytecode signatures, so from the point of view of the JVM they don't conflict.

@armanbilge
Copy link
Member Author

armanbilge commented Apr 1, 2022

Yeah, right, should have tried first 😅 this new technique is really powerful :) previously these sorts of bincompat issues seemed unfixable.

//> using scala "3.0.2"
package test
import scala.annotation._
class A
class B
object Test {
  def foo: A = new A
  @targetName("foo")
  def fooCompat: B = new B
}
public final class test.Test$ implements java.io.Serializable {
  public static final test.Test$ MODULE$;
  public static {};
  public test.A foo();
  public test.B foo();
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants