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 DISCO APi for getting latest Mandrel releases #67

Merged
merged 4 commits into from
Nov 3, 2023

Conversation

zakkak
Copy link
Collaborator

@zakkak zakkak commented Oct 30, 2023

  • Improve error log for wrong mandrel version and javaVersion comb.
  • Use Foojay.io DISCO API to get latest Mandrel release
  • Relax requirement of mandrel versions starting with mandrel-
  • Restore windows latest test & add test for non-prefixed mandrel ver

Closes #64

@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Oct 30, 2023
@zakkak
Copy link
Collaborator Author

zakkak commented Oct 30, 2023

@fniephaus I went with a mandrel specific patch for now, let me know if you would like to make it more generic so that we could use it for both mandrel and graal.

@zakkak zakkak force-pushed the 2023-10-30-mandrel-disco branch 3 times, most recently from b8c9c59 to af83447 Compare October 31, 2023 19:22
Since we can now define the distribution there is no longer the need to
include the mandrel- prefix in Mandrel versions.
Copy link
Member

@fniephaus fniephaus left a comment

Choose a reason for hiding this comment

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

LGTM. I have two minor comments, but feel free to address them in a follow up PR.

graalVMHome = await setUpMandrel(graalVMVersion, javaVersion)
} else {
throw new Error(
`Mandrel requires the 'version' option (see https://github.com/graalvm/setup-graalvm/tree/main#options).`
Copy link
Member

Choose a reason for hiding this comment

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

You probably still want to throw this error if graalVMVersion.length == 0, no? If I read this correctly, version: 'mandrel-22.2.0.0-Final' and version: '22.2.0.0-Final' will now do the same thing, but version is still required.

javaVersion: string
): Promise<string> {
const url = `${DISCO_API_BASE}?jdk_version=${javaVersion}&distribution=${c.DISTRIBUTION_MANDREL}&architecture=${c.JDK_ARCH}&operating_system=${c.JDK_PLATFORM}&latest=per_distro`
const _http = new httpClient.HttpClient('http-client-tests')
Copy link
Member

Choose a reason for hiding this comment

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

Not sure http-client-tests is supposed to be used in production code?

@fniephaus fniephaus merged commit 40dc6ae into graalvm:main Nov 3, 2023
54 of 55 checks passed
@zakkak zakkak deleted the 2023-10-30-mandrel-disco branch November 3, 2023 11:59
@zakkak
Copy link
Collaborator Author

zakkak commented Nov 3, 2023

LGTM. I have two minor comments, but feel free to address them in a follow up PR.

Thank you @fniephaus, I will.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OCA Verified All contributors have signed the Oracle Contributor Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using version: mandrel-latest with java-version: 17 results in 404 error
2 participants