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

Path to Scala 3 #274

Open
6 of 9 tasks
felixbr opened this issue Apr 24, 2020 · 42 comments
Open
6 of 9 tasks

Path to Scala 3 #274

felixbr opened this issue Apr 24, 2020 · 42 comments

Comments

@felixbr
Copy link
Contributor

felixbr commented Apr 24, 2020

This is an issue for discussing and tracking necessary step to prepare for a Dotty/Scala3 cross-build.

Inspired by the awesome "Scala Love" conference, I attempted to cross-build util-core with Dotty 0.23.0-RC1. So far it went smoother than expected. In the end there were about 4 compile-errors that I couldn't directly address.

In any case there's some work that has to be done before a real cross-build can be achieved. So far I've identified the following but I will try and update this list as we learn more.

Preparations:

  • Update sbt to recent version (e.g. 1.3.10; needed by sbt-dotty)
  • Update Scalatest to 3.1.1 (which is cross-published for Dotty)
  • Update twitter/util to Scalatest version available for latest Dotty (Dependency on Twitter codebase) | @mosesn
  • Remove deprecated Config code | @mosesn
  • Wait for Dotty to fix varargs override issue (see Overriding an @varargs method fails under separate compilation scala/scala3#9463)
  • Wait for Scalatest 3.2.0 to be published for Dotty version which includes the varargs fix
  • Refactor code which is not supported in Scala 3 (e.g. usage of Manifest/TypeTag reflection)
  • Add Scala 3 to the CI matrix to prevent regressions in already supported modules
  • Cross-build and cross-publish modules which are needed by Finagle and twitter-server (e.g. util-test and util-tunable)

Resources for Dotty migration/cross-building:

I hope you don't mind this type of issue. This is not meant to push anyone, just as a place to discuss and organize things.

Cheers
~ Felix

@mosesn
Copy link
Contributor

mosesn commented Apr 24, 2020

@felixbr yep, we're perfectly happy to have those kinds of conversations here. If it turns into a big effort, we'll probably set up a ticket like twitter/finagle#771 for doing it. We're going to upgrade scalatest soon to support twitter/finatra#527.

I can file an internal ticket around upgrading our sbt version, but we're going through some turbulence with our builds, so there's a good chance that we'll have to hold off on that until we've worked out some of the details.

@mosesn
Copy link
Contributor

mosesn commented Apr 24, 2020

Oh, also, as we find things that are challenging to migrate, that's probably good feedback for the scala compiler team. Especially if there are things that scalafix handles poorly. Please let us know what your experience in trying it out is like! Thanks for taking this on, I'm excited to see what you find =)

@cacoco
Copy link
Contributor

cacoco commented May 2, 2020

@felixbr FYI, we've updated to sbt 1.3.10 in 6ee7b49. Scalatest upgrade may take a bit longer. Thanks.

@mosesn
Copy link
Contributor

mosesn commented Jul 17, 2020

@felixbr sorry for the delay, scalatest was a beast. It's finally updated db63a6d

@felixbr
Copy link
Contributor Author

felixbr commented Jul 17, 2020

Oh, that's great news! :)

@mosesn
Copy link
Contributor

mosesn commented Jul 27, 2020

@felixbr what do you think are the next steps to supporting scala 3? We've tackled the two you suggested before.

@felixbr
Copy link
Contributor Author

felixbr commented Jul 27, 2020

The biggest problem is that Dotty/Scala 3 is not stable yet, so (if I understand correctly) for any given Dotty version we want to use for a cross-build, the dependencies have to be available. It's a moving target and that makes it difficult at the moment.

I just looked at maven-central and it seems that for Dotty 0.23 a Scalatest 3.1.2 version and for Dotty 0.24 a Scalatest 3.2.0 version exists. The latest Dotty version is 0.25 (released 5 days ago).

My guess is that moving to Scalatest 3.2.0 could be a lot of work again, so for the moment we could attempt the cross-build with Dotty 0.23 but I have no idea how outdated this is compared to 0.25.

Maybe that's something we should clarify with the Dotty-Team before we attempt cross-building with an older version.

@mosesn
Copy link
Contributor

mosesn commented Jul 28, 2020

@felixbr I'm inclined to just start with 0.23, and we can move to newer versions after we upgrade our scalatest. I think scalatest 3.1.x's scalafix rewrite will be easier to use than the 3.0.8 one, so I'm cautiously optimistic that the 3.2.0 upgrade won't take that long.

@SethTisue what do you think is the right way forward here?

@mosesn
Copy link
Contributor

mosesn commented Jul 28, 2020

Poking around a bit, it looks like @martijnhoekstra also tried porting util to dotty, and ran into some issues: https://contributors.scala-lang.org/t/scala-3-migration-guide/4237/6

What is the usual strategy that people take? Do they keep two different sets of code, or do they use the subset of scala that is supported by both scala 2 and scala 3? I think one approach I would be happy with would be to keep developing in scala 2, and then have a tag that continuously tracks "develop but scala-3-ified"

@martijnhoekstra
Copy link
Contributor

martijnhoekstra commented Jul 28, 2020

@mosesn to me it made sense at the time to report the encountered issues to dotty and not bother with workarounds yet, since introducing (sometimes ugly) workarounds for bugs that probably won't be in scala 3.0 release didn't make sense to me.

IIRC the biggest issue was the dropped support for macros which is used in IIRC the deprecated Configuration stuff (with existing notes about breakages). Apart from that, cross-building with dotty seems plausible on a unified codebase without any scala-2/scala-3 specific stuff.

Until scala 3.1 rolls along that is, but it seems too early to worry excessively about that.

@mosesn
Copy link
Contributor

mosesn commented Jul 28, 2020

Cool! I'm glad to hear it. Which Configuration stuff? Util doesn't have any macros afaik

@martijnhoekstra
Copy link
Contributor

I took another look @mosesn

It wasn't macros, but it was a dependency on scala-reflect in Config (filtering out synthetics).

Apart from that, for now the dealbreakers look like the same as mentioned on the contributors thread: no @varargs support just yet (so no java-compatible varargs forwarders and no overriding of java-compatible varargs, needed in InMemoryStatCounter) and no good story for cross-compiling dynamic member selection (on unsafe in jvm/HotSpot)

@martijnhoekstra
Copy link
Contributor

develop...martijnhoekstra:dotty-compat is a quick port -- in this state all main projects compile (but test crash-and-burns)

@mosesn
Copy link
Contributor

mosesn commented Jul 28, 2020

Got it. What do you think would be the right way to interact with the dotty team to get them to prioritize this stuff? It would be really neat for us to be able to add util to the dotty community build.

@felixbr
Copy link
Contributor Author

felixbr commented Jul 28, 2020

@martijnhoekstra I found pretty much the same blockers when I attempted the cross-build. I didn't really understand what exactly scala-reflect was used for. Do you think we can just remove it?

Maybe we should gather the missing features and then talk to the Dotty team, which of them can be expected to be solved by them and which need a separate workaround anyway.

  • Dependency on scala-reflect for "synthetic term names" (?)
  • Java-compatible varargs support
  • Dependency on scalactic.source.Position macro for logging
  • "dynamic member selection" (?)

Are these all blockers right now (apart from tests, of course)?

In general I'm in favor of trying a unified codebase first even though writing code that works on 2.11 through 3.0 will be interesting 😄

@martijnhoekstra
Copy link
Contributor

I bet it would be neat for the dotty team too, I think 2.13 would have been smoother if we ported earlier in the RC cycle too and it would be a shame if that happens here. Now is the time to do that.

The dotty team is really responsive to issues. They merged fixes for issues I opened earlier in a matter of hours after reporting. I can check tomorrow what issues are still open and give them a ping and then we can open new tickets for what's still pending.

Then maybe link them here, so it's easy to keep track what workarounds can be undone/what blockers there still are, so you can give that a nudge every now and then if it doesn't move.

Getting scalatest working and getting CI going for this repo will also help. I'm not sure what the plan is for the scalatest people, whether they'll release everything to 0.26 or just the latest version. I see they have a PR open for 0.26 but I don't know how soon they plan on releasing that.

Moving this repo to scalatest 3.2 was kind of annoying, but not too bad, even without the scalafix they have. If you still need to upgrade dependencies company-wide, then there isn't too much use in a PR for that I would think?

@larsrh
Copy link

larsrh commented Jul 29, 2020

I have a bit time at my hands. Would you be interested in me sending a PR porting to ScalaTest 3.2? (I think this is a good idea regardless of Dotty.)

@martijnhoekstra
Copy link
Contributor

Reflective access is fixed in scala/scala3#9420 -- it's not released yet. To workaround, we could maybe use a scala-2 specific shim. That probably needs some sbt-foo to get a scala-2 only directory analogous to the 2.13+/2.12- directories. Or is that built-in in the dotty plugin?

The thing with scalatest is that (at least it used to be the case that) dependencies have to be updated twitter-wide, so it's broader than just this repo.

@larsrh
Copy link

larsrh commented Jul 29, 2020

The thing with scalatest is that (at least it used to be the case that) dependencies have to be updated twitter-wide, so it's broader than just this repo.

Yeah, @felixbr told me as much. Based on a quick investigation it also requires

  • running the Scalafix rules to avoid the deprecated names in ScalaTest (can be done with ST 3.1)
  • upgrading Mockito

@martijnhoekstra
Copy link
Contributor

martijnhoekstra commented Jul 29, 2020

What I thought was a varargs issue now looks like an overriding bug in dotty: scala/scala3#9463

@mosesn
Copy link
Contributor

mosesn commented Jul 29, 2020

I'm talking to a team in a couple hours that might be willing to move Twitter to scalatest 3.2, let me get back to you. If they don't have bandwidth, I can (probably) do it myself.

Re: Config, I think we can delete it, it has been deprecated for 7 years. That'll be an action item for me, we have to make a few changes in our codebase.

@martijnhoekstra
Copy link
Contributor

That leaves the varargs overriding above that I haven't yet found a workaround for, and scalatest 3.2 actually getting released for dotty. The scala2 shim for reflectiveSelectable works fine, at the cost of some sbt config and a scala2-specific source directory.

@felixbr
Copy link
Contributor Author

felixbr commented Jul 29, 2020

I've updated the issue with the action items you talked about and the two external dependencies we're waiting for.

edit: This time for real, the first edit got lost somehow...

@mosesn
Copy link
Contributor

mosesn commented Jul 30, 2020

The meeting we had yesterday wasn't fully resolved, I'll take a stab at applying the scalafix patch (it's complicated because of the scale of our codebase) and see how it goes. Unfortunately our scalafix experts are on vacation, but hopefully I don't run into anything too bad 🤞

@martijnhoekstra
Copy link
Contributor

I asked about the dotty 0.29 plans of scalatest on their mailing list, but my message is still in moderation.

@martijnhoekstra
Copy link
Contributor

I see there is a PR open now to bring 0.29 support to scalatest 3.1.x which I seemingly prematurely assumed to be EOL.

@mosesn
Copy link
Contributor

mosesn commented Aug 3, 2020

Rad! I'm not going to try to push moving Twitter to 3.2 forward immediately then. I've taken a stab at Config, but it seems like we used it in a bad way with util-eval and we're going to need to unravel that.

@smarter
Copy link

smarter commented Nov 5, 2020

I see there is a PR open now to bring 0.29 support to scalatest 3.1.x which I seemingly prematurely assumed to be EOL.

FWIW, it looks like scalatest 3.1 hasn't been published for dotty 0.27.0-RC1, so it might in fact be EOL by now: https://repo1.maven.org/maven2/org/scalatest/scalatest_0.27/, though it might be worth asking the maintainers.

@mosesn
Copy link
Contributor

mosesn commented Nov 8, 2020

Ok, that's good to know. We'll start trying to push for scalatest 3.2. Scalafix seems to have a hard time on codebases of our size, so we're going to need to do some work to get us there.

@martijnhoekstra
Copy link
Contributor

Scalafix seems to have a hard time on codebases of our size

Bug reports about that are probably appreciated, even if they don't turn out to be actionable in a timeframe to be useful to this issue.

@mosesn
Copy link
Contributor

mosesn commented Nov 11, 2020

@martijnhoekstra we've basically built a distributed scalafix-ing framework that lets us chunk up scalafixing. I'm not sure there's a ton that scalafix can do to improve this–we just can't hold all of Twitter's scala code in RAM.

@mosesn
Copy link
Contributor

mosesn commented Nov 21, 2020

OK, @joybestourous removed Config, and it looks like scalatest 3.2.x is published for scala 3, so I think the last thing we're waiting on is migrating to the newer scalatest now. We're still having some problems with the scalafix rule. Although the distributed scalafix job seems to be working, we're now having a problem with renaming collisions.

@felixbr
Copy link
Contributor Author

felixbr commented Feb 28, 2021

FYI: Scalatest has support for all Scala 3 releases so far:

  • Scalatest 3.2.3 supports all Scala 3 milestone releases (3.0.0-M1, 3.0.0-M2, 3.0.0-M3)
  • Scalatest 3.2.5 supports Scala 3.0.0-RC1
  • Scalatest 3.2.7 supports Scala 3.0.0-RC2
  • Scalatest 3.2.8 supports Scala 3.0.0-RC3
  • Scalatest 3.2.9 supports Scala 3.0.0

https://mvnrepository.com/artifact/org.scalatest/scalatest

Just thought it might be good to know 🙂

@mosesn
Copy link
Contributor

mosesn commented Apr 15, 2021

Thanks for following up @felixbr! We have had some issues with the scalatest 3.2.x migration (short version is that scalafix runs into a ton of edge cases it doesn't handle well in the rest of our codebase) and I haven't had a chance yet to either fix the edge cases by hand or patch scalafix. I think if someone wants to make forward progress, they should try building util against 3.1.x in Scala 2.x and 3.2.x in Scala 3.x so that we don't get blocked by this.

@felixbr
Copy link
Contributor Author

felixbr commented May 24, 2021

I've opened PRs #288 and #289 as preparation steps for a real Scala 3.0.0 cross-build. 🙂

@felixbr
Copy link
Contributor Author

felixbr commented Jun 7, 2022

It's been a while since I've last looked into it and a lot of util-* modules support Scala 3 now, which is great!

The end goal for me personally is to use Finagle and twitter-server with Scala 3.

I checked the finagle project and most of the util-* modules finagle-core depends on are already ported. The ones missing seem to be:

  • util-security
  • util-tunable
  • util-test

Furthermore, scrooge (which is required for finagle-thrift) needs the following, which are also not ported yet:

  • util-validator
  • util-validator-constraints

I don't know if any of those aren't done because there are blockers or if there just wasn't any need so far. If you have any information about that, please let me know. 🙂

@mosesn
Copy link
Contributor

mosesn commented Jun 15, 2022

@felixbr thanks for reaching out! util-validator is a new project, it was just an oversight to not publish it for 3.x. I'm not sure about util-{security,tunable,test}. We would be excited about PRs to get them publishable w/ 3.x!

@pjfanning
Copy link
Contributor

pjfanning commented Jun 22, 2022

util-jackson does not yet seem to support Scala 3

https://mvnrepository.com/artifact/com.twitter/util-jackson

jackson-module-scala supports Scala 3 but there is one issue that will affect you. jackson-module-scala ScalaObjectMapper is deprecated but is still supported on Scala 2 (and will be for a while) but for Scala 3, you will need to use ClassTagExtensions instead. ClassTagExtensions works in Scala 2 but there may be edge cases where jackson-module-scala ScalaObjectMapper works and ClassTagExtensions does not.

This is the util class that will need to change if you want to support Scala 3:

https://github.com/twitter/util/blob/develop/util-jackson/src/main/scala/com/twitter/util/jackson/ScalaObjectMapper.scala

@pjfanning
Copy link
Contributor

Looks like a lot of modules depend on util-reflect which won't compile in Scala 3 because many scala reflection features have been replaced in Scala 3.

[info] compiling 3 Scala sources to /Users/pj.fanning/code/util/util-reflect/target/scala-3.0.2/classes ...
[error] -- Error: /Users/pj.fanning/code/util/util-reflect/src/main/scala/com/twitter/util/reflect/Classes.scala:15:65 
[error] 15 |  val simpleName: Class[_] => String = Memoize { clazz: Class[_] =>
[error]    |                                                                 ^
[error]    |parentheses are required around the parameter of a lambda
[error]    |This construct can be rewritten automatically under -rewrite -source 3.0-migration.
[error] -- Error: /Users/pj.fanning/code/util/util-reflect/src/main/scala/com/twitter/util/reflect/Types.scala:21:67 
[error] 21 |  val isCaseClass: Class[_] => Boolean = Memoize { clazz: Class[_] =>
[error]    |                                                                   ^
[error]    |parentheses are required around the parameter of a lambda
[error]    |This construct can be rewritten automatically under -rewrite -source 3.0-migration.
[error] -- [E008] Not Found Error: /Users/pj.fanning/code/util/util-reflect/src/main/scala/com/twitter/util/reflect/Types.scala:5:21 
[error] 5 |import scala.reflect.api.TypeCreator
[error]   |       ^^^^^^^^^^^^^^^^^
[error]   |       value api is not a member of reflect
[error] -- [E008] Not Found Error: /Users/pj.fanning/code/util/util-reflect/src/main/scala/com/twitter/util/reflect/Types.scala:6:21 
[error] 6 |import scala.reflect.runtime.universe._
[error]   |       ^^^^^^^^^^^^^^^^^^^^^
[error]   |       value runtime is not a member of reflect
[error] four errors found

Also util-mock depends on mockito-scala and that lib does not support Scala 3 - see mockito/mockito-scala#364

@felixbr
Copy link
Contributor Author

felixbr commented Jul 8, 2022

@mosesn There are several PRs for Scala 3 related updates open. Is there any chance to get them in?

I'd love to have everything in place so Scala 3 work on finagle-core is unblocked 🙂

@pjfanning
Copy link
Contributor

The Scala team have clarified how best to support Scala 3.x builds.

I was under the false impression that building with Scala 3.0 was best but the article above suggests building with latest Scala version even if that prevents lib users from using the latest lib releases with older versions of Scala 3 libs.

Scala 3.2.0 was released recently.

@felixbr
Copy link
Contributor Author

felixbr commented May 18, 2023

Hi, it's me again 👋

I've rebased and updated all my PRs regarding Scala 3 support (util-test, util-tunable, Adding Scala 3 to CI, and hopefully a fix for the flaky test in util-app).

I've tried to keep them as minimal as possible because I understand that Scala 3 might not be a high priority at Twitter currently and justifying time for that might be difficult.

That said, I'd appreciate if someone could merge the PRs or at least tell me what needs to be improved so they can be merged.

Cheers

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

No branches or pull requests

7 participants