frontend icon indicating copy to clipboard operation
frontend copied to clipboard

Switch to Play 3 and go all in Pekko

Open ioannakok opened this issue 2 years ago β€’ 6 comments

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

ioannakok avatar Dec 11 '23 18:12 ioannakok

@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 avatar Dec 11 '23 18:12 ioannakok

@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.fluentlenium to io.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 avatar Dec 12 '23 10:12 mkurz

@mkurz this is great, thanks so much for your help! I've also added a few commits:

  • Bumped io.lemonlabs:scala-uri to 4.0.3. This fixes quite a few of failing tests: https://github.com/guardian/frontend/pull/26755/commits/3ec7c20e08d1f55a4790fa52819f190a338abdc8
  • Bumped enumeratum-play-json to 1.8.0. Uses org.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.play from ai.x:play-json-extensions but 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 added org.playframework:play-json in https://github.com/guardian/frontend/commit/8e5eecc32e500930fbf289fe9ed04408b8c5b426 but it still pulls in the com.typesafe.play one:
+-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.

ioannakok avatar Dec 12 '23 19:12 ioannakok

  • I tried to exclude com.typesafe.play from ai.x:play-json-extensions but was unsuccessful. Tried both ways that are commented out in the commit 1c1306e. I thought this would be enough since you've added org.playframework:play-json in 8e5eecc but it still pulls in the com.typesafe.play one:

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

mkurz avatar Dec 12 '23 22:12 mkurz

"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[bot] avatar Jan 15 '24 06:01 github-actions[bot]

I think this should be rebased to main. You need any help?

mkurz avatar Jan 29 '24 18:01 mkurz

"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[bot] avatar Mar 04 '24 06:03 github-actions[bot]

IMHO still relevant.

mkurz avatar Mar 04 '24 09:03 mkurz

@mchv latest Play version is 3.0.2 (release notes), same as play-json (release notes)

mkurz avatar Mar 18 '24 12:03 mkurz

@mchv latest Play version is 3.0.2 (release notes), same as play-json (release notes)

Yes, I have updated in a further commit.

mchv avatar Mar 18 '24 14:03 mchv

@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.

mchv avatar Mar 18 '24 14:03 mchv

@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.

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 avatar Mar 18 '24 19:03 ioannakok

@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 πŸš€

mchv avatar Mar 19 '24 13:03 mchv

Moving this to blocked until this ticket is completed:

  • https://github.com/guardian/frontend/issues/26997

ioannakok avatar Mar 27 '24 10:03 ioannakok

@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.

mkurz avatar Apr 09 '24 10:04 mkurz

@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

mkurz avatar Apr 09 '24 11:04 mkurz

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")

mkurz avatar Apr 09 '24 11:04 mkurz

:+1: LGTM! I can see nothing that prevents this PR from merging! Happy to see the repo moving to Play 3 :tada:

mkurz avatar Apr 09 '24 11:04 mkurz

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".

ioannakok avatar Apr 10 '24 10:04 ioannakok

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 πŸ“‰

prout-bot avatar Apr 10 '24 14:04 prout-bot