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

use java 21 for scala steward #67

Merged
merged 1 commit into from
Mar 14, 2024
Merged

use java 21 for scala steward #67

merged 1 commit into from
Mar 14, 2024

Conversation

johnduffell
Copy link
Member

@johnduffell johnduffell commented Mar 14, 2024

Recently we have upgraded support-service-lambdas to java 21.
PR: guardian/support-service-lambdas#2195 (comment)

This has caused scala steward to fail due to the java 21 check.

We could relax the check - as it was probably only added becasue scala didn't used to support recent version of java, and you would get weird errors.

Having said that, why not run scala steward on a recent LTS java, as it might be quicker?

This may be a bit optimistic, as only recent patch versions of scala 2.13, 2.13 and 3 support java 21, so this is more likely to show issues in other projects.

This PR updates scala steward to use java 21 as per https://github.com/marketplace/actions/scala-steward-github-action#:~:text=triggering%20a%20run-,Specify%20JVM%20version,-Updating%20a%20specific

@johnduffell johnduffell requested a review from rtyley March 14, 2024 14:50
Copy link
Member

@rtyley rtyley left a comment

Choose a reason for hiding this comment

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

Looks great! Hopefully - if a project is on Scala Steward, it will be using a relatively recent version of Scala, that can handle Java 21.

@rtyley rtyley merged commit 20c78c6 into main Mar 14, 2024
@rtyley rtyley deleted the jd-java-21 branch March 14, 2024 16:47
@rtyley
Copy link
Member

rtyley commented Mar 14, 2024

Unfortunately, this failed in the setup-java phase:

image

Run actions/setup-java@v4
Installed distributions
Creating settings.xml with server-id: github
Writing to /home/runner/.m2/settings.xml
Error: No file in /home/runner/work/scala-steward-public-repos/scala-steward-public-repos matched to [/*.sbt,/project/build.properties,/project/.scala,/project/.sbt], make sure you have checked out the target repository

This may be because in this PR we specified that sbt resources should be cached:

...but there's no sbt project available in the working directory at this point, so the setup-java step isn't going to cache it. Note that it looks like scala-steward-action takes care of it's own dependency caching.

I'll try removing the cache config in #68

@rtyley
Copy link
Member

rtyley commented Mar 15, 2024

Now that #68 has allowed Scala Steward to run, we can see a couple of repos we need to fix up:

@johnduffell could you review the fixups?

@rtyley
Copy link
Member

rtyley commented Mar 19, 2024

A few more have cropped up - not sure why it is exactly that they didn't all show their faces immediately, I guess Scala Steward is configured to not even attempt to analyse repos too frequently?

I'm afraid I've got a busy day today @johnduffell, but maybe you could have a look at those repos, if https://github.com/guardian/mobile-apps-api/pull/2990 & guardian/mobile-save-for-later#105 provide good examples?

With both of those repos removed from the scala-steward-private-repos run, the Scala Steward job is currently passing:

image

@johnduffell
Copy link
Member Author

thanks @rtyley I think identity repo might be the more tricky one as it's got a lot going on, it's a bit of a mono repo. Perhaps @kelvin-chappell can weigh in? I am trying to get a couple of PRs hammered out today but I will have a go at one or both if I have time soon.

rtyley added a commit to guardian/mobile-logstash-encoder that referenced this pull request Mar 20, 2024
Scala Steward can't run on this repo until it allows building under Java 21, due to
guardian/scala-steward-public-repos#67 ... we're now running
our Scala Steward workflow with Java 21 LTS, which means that all projects that want
to have Scala Steward updates (like this one) need to be able to build under Java 21
(even if the projects are still _running_ in production on Java 8).

Being able to build with Java 21 [requires](https://docs.scala-lang.org/overviews/jdk-compatibility/overview.html#jdk-21-compatibility-notes)
using recent versions of Scala (2.12.18+) and sbt (1.9.0+) - for this repo, all that's
needed is a bump of sbt version!
rtyley added a commit to guardian/typerighter that referenced this pull request Mar 20, 2024
This upgrade to Play 2.9 was prompted by guardian/scala-steward-public-repos#67
which means that all projects that want to have Scala Steward updates (like this one)
need to be able to _build_ under Java 21 (even if the projects are still _running_ in
production on Java 11).

Building under Java 21 requires a recent version of sbt, and that leads to a version
bump for `scala-xml` that gives version-compatibility errors:

```
[error] 	* org.scala-lang.modules:scala-xml_2.12:2.1.0 (early-semver) is selected over {1.2.0, 1.1.1}
[error] 	    +- org.scala-lang:scala-compiler:2.12.18              (depends on 2.1.0)
[error] 	    +- com.typesafe.sbt:sbt-native-packager:1.5.2 (scalaVersion=2.12, sbtVersion=1.0) (depends on 1.1.1)
[error] 	    +- com.typesafe.play:twirl-api_2.12:1.5.1             (depends on 1.2.0)
```

...the nicest way to fix these errors is to upgrade to Play 2.9 or above, so here we are!

https://github.com/guardian/pan-domain-authentication didn't get Play 2.9 artifacts until
relatively recently, so upgrading the Panda version was also necessary.

See also:
* guardian/maintaining-scala-projects#1
rtyley added a commit to guardian/typerighter that referenced this pull request Mar 20, 2024
This upgrade to Play 2.9 was prompted by guardian/scala-steward-public-repos#67
which means that all projects that want to have Scala Steward updates (like this one)
need to be able to _build_ under Java 21 (even if the projects are still _running_ in
production on Java 11).

Building under Java 21 requires a recent version of sbt, and that leads to a version
bump for `scala-xml` that gives version-compatibility errors:

```
[error] 	* org.scala-lang.modules:scala-xml_2.12:2.1.0 (early-semver) is selected over {1.2.0, 1.1.1}
[error] 	    +- org.scala-lang:scala-compiler:2.12.18              (depends on 2.1.0)
[error] 	    +- com.typesafe.sbt:sbt-native-packager:1.5.2 (scalaVersion=2.12, sbtVersion=1.0) (depends on 1.1.1)
[error] 	    +- com.typesafe.play:twirl-api_2.12:1.5.1             (depends on 1.2.0)
```

...the nicest way to fix these errors is to upgrade to Play 2.9 or above, so here we are!

https://github.com/guardian/pan-domain-authentication didn't get Play 2.9 artifacts until
relatively recently, so upgrading the Panda version was also necessary.

See also:
* guardian/maintaining-scala-projects#1
@rtyley
Copy link
Member

rtyley commented Apr 9, 2024

@rtyley
Copy link
Member

rtyley commented Apr 12, 2024

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).
ioannakok added a commit to guardian/frontend that referenced this pull request May 17, 2024
This is not necessary. We pass `-release:11` in Scala options so the Scala compiler is already going to enforce that Java 11 is the minimum supported version. Because of this PR: guardian/scala-steward-public-repos#67 running Scala steward for frontend was failing due to this assertion because Scala Steward is running with Java 21.

Co-authored-by: Roberto Tyley <roberto.tyley@gmail.com>
ioannakok added a commit to guardian/frontend that referenced this pull request May 17, 2024
This is not necessary. We pass `-release:11` in Scala options so the Scala compiler is already going to enforce that Java 11 is the minimum supported version. Because of this PR: guardian/scala-steward-public-repos#67 running Scala steward for frontend was failing due to this assertion because Scala Steward is running with Java 21.

Co-authored-by: Roberto Tyley <roberto.tyley@gmail.com>
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.

None yet

2 participants