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

Added Liberica Native Image Kit as new distribution #87

Closed
wants to merge 7 commits into from

Conversation

petermz
Copy link
Contributor

@petermz petermz commented Mar 5, 2024

This PR addresses #76. Liberica is supported in pretty much the same fashion as GraalVM, e.g.

distribution: liberica
java-version: 17.0.10

It appeared logical to me to reuse several functions from utils.ts, so I converted a few constants used there into parameters. Also added tests, notes, and bumped version.

@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Mar 5, 2024
@fniephaus fniephaus self-assigned this Mar 5, 2024
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.

Thanks for the PR, I'll try to find some time to review it in the next couple of days.

with:
distribution: liberica
java-version: ${{ matrix.java-version }}
version: ${{ matrix.version }}
Copy link
Member

Choose a reason for hiding this comment

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

We actually am phasing out version in favor of just the java-version. It seems there are different variants of Liberica. Can these also be pulled by setup-java and if yes, how?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AFAICT this is controlled by the java-package parameter:

java-package: The packaging variant of the chosen distribution. Possible values: jdk, jre, jdk+fx, jre+fx. Default value: jdk

The value of jdk corresponds to version: std, and jdk+fx corresponds to version: full

Copy link
Member

Choose a reason for hiding this comment

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

Thanks! Mapping jdk+fx to full seems like a hack, assuming fx stands for JavaFX? If Liberica users are more familiar with std and full, I'm ok with using those directly, maybe also allowing jdk and jdk+fx for full compatibility with setup-java. In any case, we should add the java-package input from setup-java and not use the version input for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I've added the java-package: jdk | jdk+fx input

@TKaxv-7S
Copy link

Hi, I was Fixed 'version' parameter at this Pull

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.

I went through the changes and they look good. I left a couple of comments. If you could address them, that'd be great. Thanks!

src/liberica.ts Outdated Show resolved Hide resolved
src/liberica.ts Outdated Show resolved Hide resolved
src/liberica.ts Show resolved Hide resolved
@fniephaus
Copy link
Member

The failure on Windows is unrelated to this PR. I'll take care of it.

@TKaxv-7S
Copy link

Hi, I was Fixed 'version' parameter at this Pull

This PULL also fixed the issue of not being able to download the latest version of graalvm.

@petermz
Copy link
Contributor Author

petermz commented Mar 18, 2024

Addressed rest of the comments

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, thanks for contributing!

@fniephaus
Copy link
Member

This is now live and available via v1.2.0. I rebase the commits and unfortunately GitHub didn't detect that, but this PR is merged now. Again, thanks for the contribution!

@fniephaus fniephaus closed this Mar 18, 2024
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.

3 participants