Fix tests under Java 17 (Upgrade to Play 2.9 & update Guice) #94
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rtyley
commented
Jan 17, 2024
| scalaVersion := "2.12.18", | ||
| crossScalaVersions := Seq(scalaVersion.value, "2.13.12"), | ||
| scalaVersion := "2.13.12", |
There was a problem hiding this comment.
Dropping support for Scala 2.12, as it's no longer supported by Play, as of Play v2.9.
rtyley
commented
Jan 17, 2024
| "org.scalatestplus.play" %% "scalatestplus-play" % "5.1.0" % Test, | ||
| "org.scalatestplus.play" %% "scalatestplus-play" % "6.0.1" % Test, |
There was a problem hiding this comment.
This is the recommended version of scalatestplus-play for Play 2.9:
rtyley
force-pushed
the
fix-tests-under-java-17
branch
from
48940ac
to
422b7de
Compare
rtyley
changed the title
After #93 we found that unfortunately the Release workflow failed while running the tests: * The Release workflow uses Java 17 for all builds * The Atom Maker library uses Play with Guice for Dependency Injection * Guice gained Java 17 & 21 support with Guice v6: https://github.com/google/guice/wiki/Guice600 * Play only updated to Guice v6 (and gained general Java 17 support) with Play v2.9: playframework/playframework#11808 playframework/playframework@10ca54d#diff-3dc52110c1c1c453c2e9740ac58fe7e90d53121875a034ef3109c34ab030c29e
rtyley
force-pushed
the
fix-tests-under-java-17
branch
from
422b7de
to
b4d55b3
Compare
|
@rtyley has published a preview version of this PR with release workflow run #8, based on commit b4d55b3: 2.0.0-PREVIEW.fix-tests-under-java-17.2024-01-18T1039.b4d55b3d Want to make another preview release?Click 'Run workflow' in the GitHub UI, specifying the fix-tests-under-java-17 branch, or use the GitHub CLI command: gh workflow run release.yml --ref fix-tests-under-java-17 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 |
rtyley
added a commit
to guardian/atom-workshop
that referenced
this pull request
Jan 23, 2024
These upgrades were prompted by guardian/atom-maker#94, but they're all a good idea anyway: * Play: 2.8 → 2.9 * Scala: 2.12 → 2.13 * Java: 8 → 11
|
The updated libraries have been released as 2.0.0, and this time, as desired, the tests did not fail during the release workflow: https://github.com/guardian/atom-maker/actions/runs/7899574542 |
rtyley
changed the title
Merged
rtyley
added a commit
to guardian/gha-scala-library-release-workflow
that referenced
this pull request
Apr 30, 2024
This allows projects to specify, with an `asdf` (https://asdf-vm.com/)-formatted `.tool-versions` file, the Java version to be used by the workflow for building the project- `gha-scala-library-release-workflow` has always used a recent LTS version Java for the build (eg Java 17), and this has sometimes been _too recent_ (guardian/atom-maker#94) for some projects at the Guardian. We _want_ projects to update to Java 21 (see guardian/support-frontend#5792, guardian/scala-steward-public-repos#67 etc), but tying dozens of projects tightly to what `gha-scala-library-release-workflow` is using will make updating that version pretty hard. If, at some later point, _some_ projects want to experiment with Java 25, we shouldn't have to force all other projects to be compatible with that first. ## How to express the version of Java required... ### Configuration proliferation is a problem... One option would have been to simply add a new input parameter to the workflow, specifying the required Java version, but that has a downside: it proliferates the number of places in a project where the desired Java version is declared - and there are many in use at the Guardian, with no well-agreed canonical source-of-truth: * GHA `ci.yml`, in the [`java-version`](https://github.com/guardian/etag-caching/blob/7ecc04981f5a42a0f2ecb10631f28da571a49836/.github/workflows/ci.yml#L22) field of `actions/setup-java` - this has been my favourite in the past, as whatever CI runs with is usually pretty close to the truth * In sbt, the `scalacOptions` of `-target`, `-release`, `-java-output-version` (currently `-release` preferred, tho' once [support](scala/scala#10654) is pervasive, `-java-output-version` is probably best) and the `javacOptions` of `-target` & `-source` - these all effectively set lower-bounds on the version of Java supported, rather than guaranteeing a minimum upper bound of Java version the way CI does. * In apps running on Amigo AMI images; the Java version baked into the AMI, usually [referenced](https://github.com/guardian/mobile-apps-api/blob/3231e6bf064163c6d0e72c8dc862678c68eb0b62/mobile-fronts/conf/riff-raff.yaml#L10) by `riff-raff.yaml` * In AWS Lambdas; the cloudformation [`Runtime`](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-lambda-function.html#cfn-lambda-function-runtime) parameter, often set [by CDK](https://github.com/guardian/mobile-save-for-later/blob/1ac12e4c0100edb976ebae9e2a9975ad2321e26e/cdk/lib/mobile-save-for-later.ts#L44) ### ...`asdf`'s `.tool-versions` flle offers some consolidation Benefits of using `.tool-versions`: * Developers will automatically get the right Java version if they have `asdf` installed * actions/setup-java#606 has added early support for reading the version of Java used in CI by `setup-java` from the `.tool-versions` flle - unfortunately, it's of limited use to us at the moment because of actions/setup-java#615, but it's a promising start _(`setup-java` also has support for `jEnv` files, but they are not commonly used at the Guardian)_ * Format of the file is simple enough that it can be easily understood and used, even if `asdf` is not in use - **including the file doesn't _force_ developers to use `asdf`** (I know some devs have been having some problems with it, and maybe there are/will be better alternatives out there), but it clearly documents the Java version to be used. This does mean that **all library repos need to add a `.tool-versions` file** before this PR is merged. Helpful error messaging has been added with this PR to handle cases where the file is missing or invalid: #### Only Java _major_ version is guaranteed Note that although `asdf` requires a fully-specified Java version (eg `21.0.3.9.1`) in the `.tool-versions` file, currently the workflow will only match the *major* version of Java specified in the file (eg `21`), and will _always_ use the AWS Corretto distribution of Java. This is due to [limitations](actions/setup-java#615) in [`actions/setup-java`](https://github.com/actions/setup-java).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What's changing?
Why?
After PR #93, installing
gha-scala-library-release-workflow, we found that unfortunately the release workflow failed with anInaccessibleObjectExceptionwhile running the tests (which in this project's normal CI used only Java 8 & 11 up till now):How to test?
The new
gha-scala-library-release-workflowadded in #93 made it easy to publish a pre-release version of this PR (see #94 (comment) - even though release of main branch currently isn't working due to this issue, the preview release of this branch fixing the problem does work!):This has been slotted into guardian/atom-workshop#349, alongside an upgrade to Play 2.9 & Scala 2.13 there, to check that this all works.
Note that the automated version-compatibility testing has also correctly decided that this change requires a 'major' version bump, from 1.4.0 to 2.0.0!
Consumers affected by this update
Code search for
atom-publisher-lib&atom-manager-playconfirms that at the Guardian we have only 4 applications using the libraries published by this repo, of which 2 are archived: