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

Drop Scala 2.12 and Play 2.7, begin partial support for Scala 3 #318

Merged
merged 3 commits into from
Jul 24, 2024

Conversation

rtyley
Copy link
Member

@rtyley rtyley commented Jul 24, 2024

Fixes #317, meaning that PRs like #287 can use Scala 2.13 features. Also dropping Play 2.7- Play 2.8 was end-of-life (EOL) on 31st May 2024, so dropping support for Play 2.7 seems very reasonable!

Running the tests under Scala 3 required 3690941, fixing a test so that it doesn't rely on JSON object's field order.

Consumers of facia-scala-client

All 7 consumers of the facia-scala-client library are now on Scala 2.13 (last one to upgrade was guardian/story-packages#195!), and Play 2.8 or above, so they do not pose blockers to dropping Scala 2.12 and Play 2.7:

JSON objects have unordered keys, so there's no 'correct' order for the fields
to appear in when the JSON is converted into a String. This means it's a bad idea
to write tests that assert that JSON should match a specific string - because
the string may change with new versions of the underlying software libraries, eg
with changes to Java or Scala versions.

This particular 'should serialize territories' test in `facia-scala-client`
works at the moment, but spontaneously fails under Scala 3 with the changes
in #287 :

https://github.com/guardian/facia-scala-client/actions/runs/9894534871/job/27332773020#step:5:2147

```
[info] - should serialize territories *** FAILED ***
[info]   "...nt-content=true"},"t[argetedTerritory":"NZ","uneditable":true,"type":"news/most-popular]"}" was not equal to "...nt-content=true"},"t[ype":"news/most-popular","uneditable":true,"targetedTerritory":"NZ]"}" (ConfigSpec.scala:58)
```

A quick fix is to not make an assertion against a _string_, but to make an assertion
against the JSON _type_ (in our case, `JsValue`) which has proper equality rules, and
does not mind about _ordering_ of the fields.
The version of the mocking library Mockito that we're currently using does
not support Java 21, and so the tests currently fail under Java 21 with
`java.lang.NoClassDefFoundError`

```
[info] com.gu.facia.api.contentapi.ContentApiTest *** ABORTED ***
[info]   java.lang.NoClassDefFoundError: Could not initialize class org.mockito.internal.creation.cglib.ClassImposterizer$3
[info]   at org.mockito.internal.creation.cglib.ClassImposterizer.createProxyClass(ClassImposterizer.java:95)
[info]   at org.mockito.internal.creation.cglib.ClassImposterizer.imposterise(ClassImposterizer.java:57)
[info]   at org.mockito.internal.creation.cglib.ClassImposterizer.imposterise(ClassImposterizer.java:49)
[info]   at org.mockito.internal.creation.cglib.CglibMockMaker.createMock(CglibMockMaker.java:24)
[info]   at org.mockito.internal.util.MockUtil.createMock(MockUtil.java:33)
[info]   at org.mockito.internal.MockitoCore.mock(MockitoCore.java:59)
[info]   at org.mockito.Mockito.mock(Mockito.java:1285)
[info]   at org.mockito.Mockito.mock(Mockito.java:1163)
[info]   at org.scalatestplus.mockito.MockitoSugar.mock(MockitoSugar.scala:73)
[info]   at org.scalatestplus.mockito.MockitoSugar.mock$(MockitoSugar.scala:72)
[info]   ...
[info]   Cause: java.lang.ExceptionInInitializerError: Exception java.lang.ExceptionInInitializerError [in thread "pool-7-thread-11"]
[info]   at org.mockito.cglib.core.KeyFactory$Generator.generateClass(KeyFactory.java:167)
[info]   at org.mockito.cglib.core.DefaultGeneratorStrategy.generate(DefaultGeneratorStrategy.java:25)
[info]   at org.mockito.cglib.core.AbstractClassGenerator.create(AbstractClassGenerator.java:217)
[info]   at org.mockito.cglib.core.KeyFactory$Generator.create(KeyFactory.java:145)
[info]   at org.mockito.cglib.core.KeyFactory.create(KeyFactory.java:117)
[info]   at org.mockito.cglib.core.KeyFactory.create(KeyFactory.java:109)
[info]   at org.mockito.cglib.core.KeyFactory.create(KeyFactory.java:105)
[info]   at org.mockito.cglib.proxy.Enhancer.<clinit>(Enhancer.java:70)
[info]   at org.mockito.internal.creation.cglib.ClassImposterizer.createProxyClass(ClassImposterizer.java:95)
[info]   at org.mockito.internal.creation.cglib.ClassImposterizer.imposterise(ClassImposterizer.java:57)
[info]   ...
```
@rtyley rtyley changed the title Drop Scala 2.12 and Play 2.7 Drop Scala 2.12 and Play 2.7, begin partial support for Scala 3 Jul 24, 2024
@rtyley rtyley force-pushed the drop-scala-2.12-and-play-27 branch from 001eb33 to 7607e05 Compare July 24, 2024 11:10
organization := "com.gu",
resolvers ++= Resolver.sonatypeOssRepos("releases"),
scalaVersion := "2.13.14",
crossScalaVersions := Seq(scalaVersion.value) ++ (if (supportScala3) Seq("3.3.3") else Seq.empty),
Copy link
Member Author

@rtyley rtyley Jul 24, 2024

Choose a reason for hiding this comment

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

Note that Scala 2.12 is no longer listed here, and for artifacts that can support it (ie the facia-json artifacts) we now compile to Scala 3 as well.

@rtyley rtyley force-pushed the drop-scala-2.12-and-play-27 branch from 7607e05 to 2f129bb Compare July 24, 2024 11:33
java corretto-21.0.3.9.1
java corretto-11.0.23.9.1
Copy link
Member Author

@rtyley rtyley Jul 24, 2024

Choose a reason for hiding this comment

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

The version of Mockito that we're currently using does not support Java 21, and so the tests currently fail under Java 21 with java.lang.NoClassDefFoundError

[info] com.gu.facia.api.contentapi.ContentApiTest *** ABORTED ***
[info]   java.lang.NoClassDefFoundError: Could not initialize class org.mockito.internal.creation.cglib.ClassImposterizer$3
[info]   at org.mockito.internal.creation.cglib.ClassImposterizer.createProxyClass(ClassImposterizer.java:95)
[info]   at org.mockito.internal.creation.cglib.ClassImposterizer.imposterise(ClassImposterizer.java:57)
[info]   at org.mockito.internal.creation.cglib.ClassImposterizer.imposterise(ClassImposterizer.java:49)
[info]   at org.mockito.internal.creation.cglib.CglibMockMaker.createMock(CglibMockMaker.java:24)
[info]   at org.mockito.internal.util.MockUtil.createMock(MockUtil.java:33)
[info]   at org.mockito.internal.MockitoCore.mock(MockitoCore.java:59)
[info]   at org.mockito.Mockito.mock(Mockito.java:1285)
[info]   at org.mockito.Mockito.mock(Mockito.java:1163)
[info]   at org.scalatestplus.mockito.MockitoSugar.mock(MockitoSugar.scala:73)
[info]   at org.scalatestplus.mockito.MockitoSugar.mock$(MockitoSugar.scala:72)
[info]   ...
[info]   Cause: java.lang.ExceptionInInitializerError: Exception java.lang.ExceptionInInitializerError [in thread "pool-7-thread-11"]
[info]   at org.mockito.cglib.core.KeyFactory$Generator.generateClass(KeyFactory.java:167)
[info]   at org.mockito.cglib.core.DefaultGeneratorStrategy.generate(DefaultGeneratorStrategy.java:25)
[info]   at org.mockito.cglib.core.AbstractClassGenerator.create(AbstractClassGenerator.java:217)
[info]   at org.mockito.cglib.core.KeyFactory$Generator.create(KeyFactory.java:145)
[info]   at org.mockito.cglib.core.KeyFactory.create(KeyFactory.java:117)
[info]   at org.mockito.cglib.core.KeyFactory.create(KeyFactory.java:109)
[info]   at org.mockito.cglib.core.KeyFactory.create(KeyFactory.java:105)
[info]   at org.mockito.cglib.proxy.Enhancer.<clinit>(Enhancer.java:70)
[info]   at org.mockito.internal.creation.cglib.ClassImposterizer.createProxyClass(ClassImposterizer.java:95)
[info]   at org.mockito.internal.creation.cglib.ClassImposterizer.imposterise(ClassImposterizer.java:57)
[info]   ...

@rtyley rtyley marked this pull request as ready for review July 24, 2024 11:35
@gu-scala-library-release
Copy link
Contributor

@rtyley has published a preview version of this PR with release workflow run #23, based on commit 2f129bb:

8.0.1-PREVIEW.drop-scala-212-and-play-27.2024-07-24T1450.2f129bb2

Want to make another preview release?

Click 'Run workflow' in the GitHub UI, specifying the drop-scala-2.12-and-play-27 branch, or use the GitHub CLI command:

gh workflow run release.yml --ref drop-scala-2.12-and-play-27

Want to make a full release after this PR is merged?

Click 'Run workflow' in the GitHub UI, leaving the branch as the default, or use the GitHub CLI command:

gh workflow run release.yml

Fixes #317, meaning that PRs like #287 can use Scala 2.13 features.
Also dropping Play 2.7 - Play 2.8 was end-of-life (EOL) on 31st May 2024,
so dropping support for Play 2.7 seems very reasonable!

Running the tests under Scala 3 required commit 3690941, fixing a test so
that it doesn't rely on JSON object's field order.

All 7 consumers of the facia-scala-client library are now on Scala 2.13,
and Play 2.8 or above, so they do not pose blockers to dropping Scala 2.12 and Play 2.7:

* guardian/ophan - Play 3.0.4
* guardian/frontend - Play 3.0.4
* guardian/apple-news - Play 3.0.2
* guardian/editors-picks-uploader - AWS Lambda, doesn't use Play
* guardian/facia-tool - Play 3.0.2
* guardian/story-packages - Play 2.8.19
* guardian/mobile-apps-api - Play 3.0.3
@rtyley rtyley force-pushed the drop-scala-2.12-and-play-27 branch from 2f129bb to 3aa8aa1 Compare July 24, 2024 15:04
@gu-scala-library-release
Copy link
Contributor

@rtyley has published a preview version of this PR with release workflow run #24, based on commit 3aa8aa1:

8.0.1-PREVIEW.drop-scala-212-and-play-27.2024-07-24T1515.3aa8aa15

Want to make another preview release?

Click 'Run workflow' in the GitHub UI, specifying the drop-scala-2.12-and-play-27 branch, or use the GitHub CLI command:

gh workflow run release.yml --ref drop-scala-2.12-and-play-27

Want to make a full release after this PR is merged?

Click 'Run workflow' in the GitHub UI, leaving the branch as the default, or use the GitHub CLI command:

gh workflow run release.yml

scalaVersion := "2.13.14",
crossScalaVersions := Seq(scalaVersion.value) ++ (if (supportScala3) Seq("3.3.3") else Seq.empty),
scalacOptions := Seq(
"-release:8",
Copy link
Contributor

Choose a reason for hiding this comment

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

release:11 has become release:8?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, I'm afraid so- guardian/story-packages is still running on Java 8:

https://github.com/guardian/story-packages/blob/041d6946a1da3d24f687dd4ff643798d8d160887/riff-raff.yaml#L17

@rtyley rtyley merged commit f358a51 into main Jul 24, 2024
10 checks passed
rtyley added a commit that referenced this pull request Aug 2, 2024
After #318 dropped
support for Scala 2.12, release 8.0.1 correctly no longer published Scala 2.12
artifacts, but unfortunately the 'root' project (which does not have any
code!) was still defaulting to using Scala 2.12, because that is the
default sbt uses if no `scalaVersion` is set.

The next time we tried to do a release (#319 (comment))
the automated-compatibility checking failed, because it saw that one project
was using Scala 2.12, and so it attempted to download all the non-existent
Scala 2.12 artifacts for release 8.0.1 - and failed.

To fix this, we can simply ensure that all modules in this sbt build default
to Scala 2.13, not 2.12 - ie particularly, the 'root' module!
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 this pull request may close these issues.

Drop support for Scala 2.12
2 participants