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

java-version-file with asdf's .tool-versions fails for Corretto, also non-strict semver versions #615

Open
2 of 5 tasks
rtyley opened this issue Mar 28, 2024 · 5 comments
Assignees
Labels
bug Something isn't working

Comments

@rtyley
Copy link

rtyley commented Mar 28, 2024

Description:
Support for asdf's .tool-versions file was added to the java-version-file parameter with PR #606 in response to issue #579 - unfortunately, there's a difference between what asdf requires for this file, vs what setup-java supports:

asdf requires that the Java version in .tool-versions is fully specified

So a file will typically look like this:

java corretto-21.0.2.13.1

If you shorten the version, eg to 21.0.2 or 21, and run asdf install java, you'll get this error from asdf:

Unknown release: corretto-21.0.2

setup-java only supports major versions or strict 3-part semver

setup-java supports version numbers of 1 to 3 dot-separated integers (eg major to major.minor.patch). This excludes several valid version numbers - here are some examples:

  • all Corretto version numbers, which are always at least 4-part (eg 21.0.2.13.1 or 8.322.06.4)
  • Zulu 8.74.0.17
  • sapmachine 11.0.16.1
  • Oracle 18.0.2.1
  • Microsoft 17.0.1.12.1

setup-java only supports major versions for Corretto

This is probably due to AWS Corretto only providing a simple API for looking up Java versions, that only supports getting the latest version for each major version of Java:

'https://corretto.github.io/corretto-downloads/latest_links/indexmap_with_checksum.json';

So, Corretto with the java-version-file parameter is doubly unsupported:

  • Corretto 5-part version numbers don't match the regex's used by setup-java to verify version numbers
  • Even if Corretto version numbers were accepted, corretto/installer.ts wouldn't be able to locate and download the specific Corretto version, because it can only support finding by major version number
    if (version.includes('.')) {
    throw new Error('Only major versions are supported');

Task version:

v4.2.1

Platform:

  • Ubuntu
  • macOS
  • Windows

Runner type:

  • Hosted
  • Self-hosted

Repro steps:
Define an invocation of actions/setup-java in a workflow, and point java-version-file to .tool-versions:

.github/workflows/ci.yml :

        uses: actions/setup-java@v4
        with:
          distribution: zulu
          java-version-file: .tool-versions

In .tool-versions, define a version like zulu-8.58.0.13 - and verify that works by executing asdf install java:

.tool-versions :

java zulu-8.58.0.13

The let the ci.yml workflow run.

Expected behavior:
Ideally, if the .tool-versions file is valid for asdf, setup-java should install that precise version of Java. This means setup-java needs to accept all versions output by asdf list-all java.

Where a distribution has been specified that setup-java is unable to search for precise Java versions (ie Corretto), if the user has set a java-version-accept-any-matching-major-version parameter to true it should just install the latest version of Java for that major version.

Actual behavior:

zulu-8.58.0.13 in .tool-versions (guardian/etag-caching@959d4dc) :
https://github.com/guardian/etag-caching/actions/runs/8470107933/job/23207054598#step:3:16

No supported version was found in file .tool-versions

image

cc @aparnajyothi-y @mahabaleshwars @HarithaVattikuti

@rtyley rtyley added bug Something isn't working needs triage labels Mar 28, 2024
@HarithaVattikuti
Copy link
Contributor

Hello @rtyley
Thank you for creating this issue. We will investigate it and get back to you as soon as we have some feedback.

@mahabaleshwars mahabaleshwars self-assigned this Apr 3, 2024
rtyley added a commit to guardian/gha-scala-library-release-workflow that referenced this issue 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).
@mahabaleshwars
Copy link
Contributor

Hello @rtyley, I hope this message finds you well. I wanted to kindly bring to your attention some observations regarding the adaptation of our support strategy for the asdf's tool versions file. Specifically, we've noted certain discrepancies with Java versions that do not entirely align with Semver standards.

Moreover, it has come to our attention that some of the currently supported distributions are not listed in the asdf java available versions. To ensure consistency and adherence to best practices, our focus will now be directed towards maintaining .tool-versions files that fully comply with Semver standards.

For your convenience, please note that we have also updated the documentation. Thank you for your attention to this matter.

@mahabaleshwars
Copy link
Contributor

Hello @rtyley, this is a polite reminder to verify if your issue has been resolved with the revised documentation.

@mahabaleshwars
Copy link
Contributor

Hi @rtyley, could you please confirm whether the revised documentation resolved your issue?

@rtyley
Copy link
Author

rtyley commented May 22, 2024

Hi @mahabaleshwars!

To ensure consistency and adherence to best practices, our focus will now be directed towards maintaining .tool-versions files that fully comply with Semver standards.

I can see the appeal of choosing a standard, but I think there's a possibility that strict semver (MAJOR.MINOR.PATCH) is not an applicable standard, when major JDK distributors like AWS, Oracle, Microsoft, Azul & SAP are releasing JDK versions that do not confirm to semver.

If the organisations releasing JDKs are not using semver, then what's so good about setup-java requiring semver?

could you please confirm whether the revised documentation resolved your issue?

Personally, I wouldn't say that the changes made to the documentation so far resolve the issue.

For your convenience, please note that we have also updated the documentation.

Incidentally, I appreciate you linked to the documentation, but as it's a large document, and you were asking me to comment on a change, it would actually be more helpful to include a link to the PR that made the change, which I've found is #622.

So, I think there are two reasonable ways to address the issue, and updating the documentation is one of them - obviously, I'd much prefer that setup-java supported the ideal behaviour I suggested in this issue description (if a .tool-versions file is valid for asdf, setup-java should install that precise version of Java, or a least make a best effort to install the right major version, etc), and I appreciate that's substantial work. If you don't have the available capacity for those alterations, it's good to instead update the documentation to warn users that support for .tool-versions is seriously limited (ie, it will reject a large proportion of valid asdf .tool-versions files).

Unfortunately, I feel the changes in #622 aren't quite clear enough to help users to understand the limitations - essentially the explanation added in the PR was just adding ", adhering to Semantic Versioning (semver)" to the end of "...the .tool-versions file supports version specifications in accordance with asdf":

image

Problems with this:

  • This doesn't link to the semver standard at https://semver.org/, so users are relying on their own recall of what the semver standard is (eg, they may well think it's just numbers-separated-by-dots, and not recall it requires specifically 3 numbers)
  • setup-java does not support the full semver standard, because the semver standard is actually quite extensive and allows pre-release suffixes like -alpha-a.b-c-somethinglong+build.1-aef.1-its-okay or just -snapshot.1, and setup-java doesn't support that
  • It would probably be clearer to explicitly state the actual supported format, eg "for versions that strictly adhere to MAJOR.MINOR.PATCH version numbers".

...personally, I don't think adhering to the semver standard here is a necessarily a gain for setup-java, and the truth is setup-java only has partial support for it.

Note that even if AWS Corretto did use strict semver, setup-java still wouldn't be able to support .tool-versions files that specify Corretto versions, because AWS only provide an API for looking up Corretto artifacts by major version number - semver numbering is not really the blocker there, so requiring it doesn't seem to help? To me, it would be better to support a accept-any-matching-major-version parameter instead, to allow falling back to supported distribution lookup methods.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants