Skip to content

In CI use runner's JDK17 by default#14142

Closed
nineinchnick wants to merge 1 commit intotrinodb:masterfrom
nineinchnick:default-ci-runner-java
Closed

In CI use runner's JDK17 by default#14142
nineinchnick wants to merge 1 commit intotrinodb:masterfrom
nineinchnick:default-ci-runner-java

Conversation

@nineinchnick
Copy link
Copy Markdown
Member

Description

Fixes #14141. This makes me wonder if we should be downloading the JDK at all. We could replace actions/setup-java with actions/cache (since we do use it implicitly for the local Maven repository).

Non-technical explanation

Release notes

(x) This is not user-visible and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text:

@cla-bot cla-bot bot added the cla-signed label Sep 15, 2022
Copy link
Copy Markdown
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

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

This removes whatever little reproducibility we had. I'd argue to do opposite - explicitly unset JVM_HOME before running setup-java to make sure setup-java failures (even if silent) actually lead to failure.

@nineinchnick
Copy link
Copy Markdown
Member Author

I agree that turning silent failures into visible ones is much better. I updated this PR.

@findepi
Copy link
Copy Markdown
Member

findepi commented Sep 16, 2022

explicitly unset JVM_HOME before running setup-java to make sure setup-java failures (even if silent) actually lead to failure.

i am not sure this is sufficient

do you know why setup-java failures can be silent?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should we report this to GH?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, we should open an issue in https://github.com/actions/setup-java but I'm not sure how to describe it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

i guess #14141 (comment) is all we know so far.

@nineinchnick
Copy link
Copy Markdown
Member Author

do you know why setup-java failures can be silent?

No, I have no idea. If there would be some network issue I'd expect more errors to get logged, instead of the action just terminating.

@nineinchnick
Copy link
Copy Markdown
Member Author

Looks like this one lost traction. I'll close it for now and maybe we could get back to it once either #14865 or #12817 gets merged.

@nineinchnick nineinchnick deleted the default-ci-runner-java branch May 27, 2024 08:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

Random build failure because Java 11 gets picked

3 participants