Switch to Play 3 and go all in Pekko
Re-raising @mkurz's PR:
- https://github.com/guardian/frontend/pull/26677
It has to be raised by someone in The Guardian organisation for the build to run.
What does this change?
Switches to Pekko and Play 3
Checklist
- [ ] Tested locally, and on CODE if necessary
- [ ] Will not break dotcom-rendering
- [ ] Will not break our database β if updating CAPI, updated and committed the database files
- [ ] Meets our accessibility standards
@mkurz π from here as well. I have pushed a couple of commits.
We are now unfortunately getting a binary incompatibility error:
[error] (common / update) found version conflict(s) in library dependencies; some are suspected to be binary incompatible:
[error]
[error] * org.scala-lang.modules:scala-xml_2.13:2.2.0 (early-semver) is selected over {1.2.0, 1.3.0}
[error] +- org.playframework:play-ws-standalone-xml_2.13:3.0.0 (depends on 2.2.0)
[error] +- org.playframework.twirl:twirl-api_2.13:2.0.1 (depends on 2.2.0)
[error] +- net.liftweb:lift-json_2.13:3.5.0 (depends on 1.3.0)
[error] +- com.typesafe.play:twirl-api_2.13:1.4.2 (depends on 1.2.0)
lift-json and com.typesafe.play:twirl-api are both coming from libraries that we own but it looks like this is going to be complicated. I will try to summarise my findings here:
lift-json
+-com.gu.identity:identity-model_2.13:3.255
| +-net.liftweb:lift-json_2.13:3.4.3
| | +-org.scala-lang.modules:scala-xml_2.13:1.3.0
Work needs to be done to go to a >= 4.10 identity version which uses the latest lift-json:3.5.0. 4.10 has breaking changes. But even when we do that lift-json:3.5.0 still uses https://index.scala-lang.org/lift/framework/artifacts/lift-json/3.5.0 org.scala-lang.modules:scala-xml:1.3.0.
com.typesafe.play:twirl-api
+-com.gu:atom-renderer_2.13:1.2.0 [S]
| +-org.http4s:http4s-twirl_2.13:0.21.0-M5
| | +-com.typesafe.play:twirl-api_2.13:1.4.2
This is actually a deprecated repo but we sometimes have to do upgrades if it blocks other pieces of work, Play 3 is a good example. We will have to either upgrade org.http4s:http4s-twirl there or completely remove atom-renderer dependency from frontend. I'm currently looking if it's possible to do the latter. Will give an update here once I know more!
@ioannakok scala-xml 1.x and 2.x are pretty much compatible, please see https://github.com/scala/scala-xml/discussions/605#discussioncomment-2828193, so you should be safe to just exclude it from the dependencies that pull in the old 1.x version.
I checked out this PR and eventually got it compiling. I pushed the commits to my switch-to-pekko-and-play3-mkurz branch:
- You need to switch the package from
org.fluentleniumtoio.fluentlenium: https://github.com/mkurz/guardian-frontend/commit/780ddbb898301233c9fe03b827b517f2126cd206 - You need to fix a deprecation: https://github.com/mkurz/guardian-frontend/commit/a436cbe61562ee9a326a71a683e8f240a0ab397d
- And fix another compiler error regarding implicits: https://github.com/mkurz/guardian-frontend/commit/7be6b8371d340c9a594c7a5975e0d6ce560bcda2
You can cherry-pick those commits if you like.
And now to make things compile I just excluded some libraries:
- https://github.com/mkurz/guardian-frontend/commit/c7ce14908150caacac21411b3a4ee38da54c486a
like the old scala-xml dependencies. Also some libraries, the identity ones, still pull in com.typesafe.play packages, which means those identity libraries haven't been upgraded to Play 3 yet (?). Also I don't know under which repo they would be hosted: https://github.com/orgs/guardian/repositories?q=identity&type=all&language=&sort= (?)
Liked said, I am pretty sure you could even go in production by having the old scala-xml excluded, however you should take care of the pre Play 3.0 dependencies... Also I saw that you still pull in com.typesafe.play:play-json somewhere... There aren't compiler errors, but you should see to get it upgraded in the affected dependencies...
Anyway, if you like, you could (at least temporary) cherry-pick that last commit of mine as well, just to make CI run the tests and see if something else needs to be fixed, so you at least are not blocked now.
@mkurz this is great, thanks so much for your help! I've also added a few commits:
- Bumped
io.lemonlabs:scala-urito4.0.3. This fixes quite a few of failing tests: https://github.com/guardian/frontend/pull/26755/commits/3ec7c20e08d1f55a4790fa52819f190a338abdc8 - Bumped
enumeratum-play-jsonto1.8.0. Usesorg.playframework::play-json: https://github.com/guardian/frontend/pull/26755/commits/e909ce3240ecfbb354add23e218ae47b3520fae5 - Temporarily commented out three test suites which hang forever and prevent the build from completing: https://github.com/guardian/frontend/pull/26755/commits/cde9a285f542a885744bf98681303f1a95dc3d04. I would like to see the rest of the failing tests.
- As per a suggestion in https://github.com/bizzabo/play-json-extensions/issues/94 I tried to exclude
com.typesafe.playfromai.x:play-json-extensionsbut was unsuccessful. Tried both ways that are commented out in the commit https://github.com/guardian/frontend/pull/26755/commits/1c1306e36f5391ece3c26d89252f915f6f2f1d13. I thought this would be enough since you've addedorg.playframework:play-jsonin https://github.com/guardian/frontend/commit/8e5eecc32e500930fbf289fe9ed04408b8c5b426 but it still pulls in thecom.typesafe.playone:
+-ai.x:play-json-extensions_2.13:0.42.0 [S]
| | +-com.typesafe.play:play-json_2.13:2.8.1 (evicted by: 2.8.2)
| | +-com.typesafe.play:play-json_2.13:2.8.2 [S]
Let me know if I'm misunderstanding anything here.
- The identity libraries are hosted under a private repo but will have to check with @guardian/identity devs whether there is a plan to update those to Play 3 any time soon.
Thanks again for your help.
For me it is working. Did you run reload within sbt before you tried again? Maybe you need to run clean within sbt or quit/restart sbt so changes take effect?
This is what I am using: https://github.com/mkurz/guardian-frontend/commit/ee7bafc1eeb576d748afeef056a4a9af0401f9b7
Assuming ai.x:play-json-extensions is "fixed", the only dependencies left pulling in the old play-json are these:
- For facia-json I opened a pull request (https://github.com/guardian/facia-scala-client/pull/298) and made a note in my branch: https://github.com/mkurz/guardian-frontend/commit/ac37835af7785d01b72c74867d64b0873905ed81
- It seems targeting-client is a private repo, at least I can not find it. According to on of its pom files on maven central it should be hosted here: https://github.com/guardian/targeting-client Added a note also: https://github.com/mkurz/guardian-frontend/commit/5c1cfb8725d62002110abedc5f7b6a3db23291fe
"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"
I think this should be rebased to main. You need any help?
"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"
IMHO still relevant.
@mchv latest Play version is 3.0.2 (release notes), same as play-json (release notes)
@mchv latest Play version is 3.0.2 (release notes), same as play-json (release notes)
Yes, I have updated in a further commit.
@ioannakok I have rebased against main and added a few commits (sbt update, 3.0.2 update, update targeting client, etc.) and this now build, but we need to uncomment remaining ignored test and investigate any new failure.
@ioannakok I have rebased against
mainand added a few commits (sbtupdate,3.0.2update, update targeting client, etc.) and this now build, but we need to uncomment remaining ignored test and investigate any new failure.
Thanks @mchv! Tests were fixed in the Play 2.9 upgrade:
- https://github.com/guardian/frontend/pull/26783
I can't see any other ignored tests in this PR, unless I'm missing sth?
@ioannakok I missed this, sorry! you are right all tests are now included. Great work, we seems quite close to be able to ship this π
Moving this to blocked until this ticket is completed:
- https://github.com/guardian/frontend/issues/26997
@ioannakok I see atom-renderer now got removed and you rebased this branch already. I will take a look at this PR once more later today if you don't mind.
@ioannakok You can upgrade scalatestplus-play:
- val scalaTestPlus = "org.scalatestplus.play" %% "scalatestplus-play" % "7.0.0" % Test
+ val scalaTestPlus = "org.scalatestplus.play" %% "scalatestplus-play" % "7.0.1" % Test
Also upgrading sbt-buildinfo in project/plugins.sbt should not hurt:
-addSbtPlugin("com.eed3si9n" % "sbt-buildinfo" % "0.11.0")
+addSbtPlugin("com.eed3si9n" % "sbt-buildinfo" % "0.12.0")
:+1: LGTM! I can see nothing that prevents this PR from merging! Happy to see the repo moving to Play 3 :tada:
This is a tremendous effort β thanks to everyone involved. π
I must ask, however, whether there were a few changes here that are not strictly necessary for the Play 3 upgrade, as we do recommend PRs remain as small as possible β ideally atomic. It's not a blocker, but it may muddle the waters if anything were to go awry and we'd need to identify the root cause. I'm specifically asking about the changes to SnapLink filtering, SBT version and Scala version.
Thanks @mxdvl for your review and everyone for their contributions! I will split these changes into separate PRs and add them in the PR description to the section "Previous PRs preparing for this upgrade".
Seen on FRONTS-PROD, ADMIN-PROD (merged by @ioannakok 12 minutes and 15 seconds ago)
- Check your changes on www.theguardian.com βοΈ
- Keep an eye on the deploy dashboard π