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

Switch to Play 3 and go all in on Pekko #26677

Closed
wants to merge 6 commits into from
Closed

Conversation

mkurz
Copy link
Contributor

@mkurz mkurz commented Nov 7, 2023

Hi!

Play maintainer here 😉 I see you switched most of the repo to Pekko already:

Not sure if you have seen it, we finally (thanks to your support!) released Play 3, which starts to use Pekko under the hood, more can be found here:

All the new features, release highlights and the migration guide from Play 2.9 apply to 3.0 as well:

I took a look at your repo and started the migration; it's probably not all that needs to be done, but to get started. I will try my best to help to get things working for you. I will see what fails and push more commits to this pull request.
E.g. it might be possible that webjars cause problems, since Play 3 uses sbt-web 1.5 now which was refactored a bit. We will see. Also I think some dependencies need to be upgraded, like https://github.com/bizzabo/play-json-extensions (?). If so, I am happy to help there also.

If you have any further questions I am available.

BTW: Thanks for your ongoing financial support for Play, without it we would definitely not have been able to push out those releases, thank you very much!

@mkurz mkurz requested a review from a team as a code owner November 7, 2023 16:25
@@ -144,7 +144,6 @@ object Filters {

}

//Note: still using akka (instead of pekko) materializer from Play as both filters extend Play's Filter, so the types need to match
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it ok to just remove that comment now? I didn't dig deeper if something else needs to be done.

jackModule,
jacksonDatabind,
)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Play 2.9/3 upgraded to Jackson 2.14: https://www.playframework.com/documentation/latest/Highlights29#Jackson-Upgraded-to-2.14
No need to upgrade explicitly anymore.


addSbtPlugin("org.scalameta" % "sbt-scalafmt" % "2.4.0")

addSbtPlugin("net.virtual-void" % "sbt-dependency-graph" % "0.10.0-RC1")
addDependencyTreePlugin
Copy link
Contributor Author

@mkurz mkurz Nov 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resolvers ++= Resolver.sonatypeOssRepos("releases") ++ Seq(
Classpaths.typesafeReleases,
Resolver.typesafeRepo("releases"),
)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These typesafe resolver should not be needed anymore 🤞


addSbtPlugin("com.typesafe.sbt" % "sbt-native-packager" % "1.3.1")
addSbtPlugin("com.github.sbt" % "sbt-native-packager" % "1.9.16")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually not sure if sbt-native-packager needs to be pulled in explicitly because Play should provide it anyway.

build itself. Once the build has succeeded, there is no further risk (ie of a runtime exception due to clashing
versions of `scala-xml`).
*/
libraryDependencySchemes += "org.scala-lang.modules" %% "scala-xml" % VersionScheme.Always
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This workaround should not be needed anymore after upgrading sbt to 1.9.x, Play 2.9+, native-packager, etc. They all come with scala-xml 2.x now.

Copy link
Member

@AshCorr AshCorr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @mkurz !

Wow! We were not expecting someone else to do this migration for us, thank you so much! We really really appreciate all the work you and the rest of the Play framework team put into maintaining such a great library!

As the group ID for the Play framework is changing to org.playframework now will we need to make sure that all of our dependencies that use Play also use the new group ID?

I'm thinking these 3 in particular which we maintain https://github.com/guardian/frontend/pull/26677/files#diff-ad2642dc77b3679e4ad2c7d3b2f59a2dcf48c1bf5b93a558a4fa44828315a34cL54-L56

@mkurz
Copy link
Contributor Author

mkurz commented Nov 8, 2023

Wow! We were not expecting someone else to do this migration for us, thank you so much!

We probably need to do this together 😝

We really really appreciate all the work you and the rest of the Play framework team put into maintaining such a great library!

❤️

As the group ID for the Play framework is changing to org.playframework now will we need to make sure that all of our dependencies that use Play also use the new group ID?

Yes, all dependencies need to switch their own dependencies to org.playframework now. There could be exceptions where it does not matter, but it's definitely much safer if everything switches.

I was looking at the libs you mentioned: It shouldn't be to hard to migrate them. I am somehow motivated to pick this up. However there are some things I would change if you don't mind:

  1. would it be ok if you first delete all those unrelevant, old branches which are "closed" (red) and "merged" (purble) from both repos, just to do a bit of cleanup:
  2. I see that you pull in v2.2.7 of the play-googleauth lib, but there is no git-tag for that:
  3. I see that in those projects you use an approach of creating a folder for each major Play release, like here, where you than just link the the play-v27 folder. Or here you link even both, 2.7 and 2.8, to 2.6. I would suggest to not do that, because e.g. now in play-googleauth, where you import akka.*, the setup would get even more complex, with maybe duplicated files, just to switch that import to org.apache.pekko. IMHO it would be better if each major play release would be maintained in its own branch. That could be done by naming the branches e.g. 1.x which contains the Play 2.7 code base, then for Play 2.8.x the branch would 2.x, and so on (this is just an example). Or name the branches after the play releases (play28 for Play 2.8, etc). Again just an example. However that would avoid of doing this folder-per-release thing with symlinking and instead back-/forwardporting bug fixes and features between branches which is IMHO much nicer and the setup simpler. What I would do now is:
    1. Create a branch on current HEAD of both repos, named play2 or similiar, to mark this is the branch that contains the (now "old") Play 2.x code base of those libs. Then I would start to clean up the main branch, by removing all that symlink stuff to have a nice setup. The next play-googleauth release would then be 3.0.0, from the master branch. For a future Play 3.1 release you could then branch off a 3.0.x branch from main and migrate to Play 3.1 in the main and so on. You could than always backport changes from main to 3.0.x if desired and cut release from that branch for Play 3.0 etc.

What do you think about that?

@johnduffell
Copy link
Member

johnduffell commented Nov 8, 2023

This is really helpful, thanks @mkurz as I was just starting this yesterday for https://github.com/guardian/members-data-api
I have just picked up a couple of tips from your PR and I will continue today.
The good thing is upgrading to play 2.9 or 3.0 will unlock the scala 3 upgrade path, and all the readability and learning curve benefits that scala 3 brings!

edit: PR here guardian/members-data-api#1037

rtyley added a commit to guardian/play-secret-rotation that referenced this pull request Nov 8, 2023
Play v2.9 introduces support for Scala 3 - Play v3.0 also switches from
Akka to Pekko, which is great from a licensing perspective (Play v2.9
had to ship with old versions of Akka to avoid the BSL license:
https://www.playframework.com/documentation/3.0.x/General#How-Play-Deals-with-Akkas-License-Change ).

Here, we're upgrading `play-secret-rotation` to support these new latest
versions of Play, and add the new welcome Scala 3 support! To reduce
maintenance build complexity, we're also dropping support for Play 2.6,
and Scala 2.12.

Note, Matthias Kurz made some interesting suggestions about
restructuring the multi-Play support in this project and also
`google-play-auth`:

guardian/frontend#26677 (comment)

...however, for `play-secret-rotation` at least, the changes didn't
appear necessary, as `play-secret-rotation` actually has a fairly
minimal dependency on Play, and doesn't touch Akka/Pekko related code.
@mkurz
Copy link
Contributor Author

mkurz commented Nov 9, 2023

BTW, I guess you still run on Java 11? I recommend that you switch to Java 17, or even better Java 21 already (both are LTS). We plan to drop Java 11 in Play 3.1.0 (ETA somehow next summer or autumn, so like roughly 1 year from now). However very likely just by switching to Java 17/21 you might see a performance boost (?), at least that is what we observed in benchmarks and one Play Framework sponsor reported that to me.

@AshCorr
Copy link
Member

AshCorr commented Nov 9, 2023

would it be ok if you first delete all those unrelevant, old branches which are "closed" (red) and "merged" (purble) from both repos, just to do a bit of cleanup

Can do, we tend not to use forks so we do end up with a fair few stale branches on our repos.

I see that you pull in v2.2.7 of the play-googleauth lib, but there is no git-tag for that
I see that in those projects you use an approach of creating a folder for each major Play release

Yea we could probably do with rethinking how we manage our various versions of these dependencies, our current approach doesn't seem that sustainable. Using tags/branches to keep track of older versions of the library feels like it could make upgrades a fair bit easier.

BTW, I guess you still run on Java 11?

This repo does, our recommendation is to use Java 17 but we haven't gotten around to upgrading everything yet.

@ioannakok ioannakok self-assigned this Nov 15, 2023
@ioannakok ioannakok added this to the Health milestone Nov 15, 2023
@ioannakok
Copy link
Contributor

ioannakok commented Dec 11, 2023

Hi @mkurz, thanks so much for your contribution! As others have already said, we really appreciate it. I have re-raised your PR here:

because in order for the tests to run someone from the Guardian organisation has to raise it. I hope that's okay! I am going to add a comment there with an update.

Copy link
Contributor

"This PR is stale because it has been open 30 days with no activity. Unless a comment is added or the “stale” label removed, this will be closed in 3 days"

@github-actions github-actions bot added the Stale label Jan 11, 2024
Copy link
Contributor

This PR was closed because it has been stalled for 3 days with no activity.

@github-actions github-actions bot closed this Jan 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants