- Sponsor
-
Notifications
You must be signed in to change notification settings - Fork 308
Switch JAVA_HOME to 21 for JRuby #721
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
Conversation
22e5251
to
061b308
Compare
@eregon This is a quick attempt to implement what I need. I am not sure how to detect if the user has already reassigned JAVA_HOME, either manually or by setup-java. It may not be possible. I am trusting that Not sure how to make this more robust. |
JRuby 10 requires Java 21. Since the previous default was 17 and all JRuby releases should work fine on 21, we do this for all JRuby installs. Implements ruby#718
061b308
to
4158eb5
Compare
FWIW GitHub may soon bump the default Java to 21, at which point we won't need this anymore. |
Interesting, any issue I can follow about that? |
No, I just assume they eventually move the default version up. Obviously they moved it up to 17 at some point. |
The JAVA_HOME_* variables use 'arm64' on ARM.
5d3234e
to
ef40e17
Compare
Latest run appears to be green and handles arch properly on macos. Ruby version shows JRuby is running with 21. |
@eregon Let me know if there's additional work needed here. Whenever this goes live, we can start unpinning jruby-head builds from 9.4. |
One idea given https://github.com/eregon/actions-shell/actions/runs/13798182243/job/38594765309#step:4:5
would be to only set it if You could get the Java version from |
Detecting the existing JAVA_HOME version is problematic; we'll have to parse the output of either |
Could you add the check with |
I believe a more robust option is to run JRuby and only update JAVA_HOME if it fails. |
Is parsing the output of
We could match the first line there, e.g. by matching the first number ( Using |
Feel free to try it, e.g. in another PR. |
Parsing version output will be subject to any changes that might happen in the future, and won't parse right if it's not OpenJDK (OpenJ9 has a different -version output for example). Simply attempting to run |
29d6dd3
to
7ffdbb4
Compare
Not sure why the exec call is producing that error. I am not familiar with how async/await or this exec module work and tried to mimic the other call I saw. |
The new logic is working (update only if |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good, just a few more tweaks and it should be ready to be merged.
Yes, I think there is no way to know until we get a JRuby 10 build, i.e. when we switch jruby-dev-builder to use master. |
d69712b
to
dad12fe
Compare
Changes from latest review are in and green. |
common.js
Outdated
@@ -6,6 +6,8 @@ const stream = require('stream') | |||
const crypto = require('crypto') | |||
const core = require('@actions/core') | |||
const tc = require('@actions/tool-cache') | |||
const exec = require('@actions/exec') | |||
const common = require('./common') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a require of this file, it seems safer to avoid it, as I guess it might cause some weird stuff.
I'll fix it.
There is some weird output e.g. in https://github.com/ruby/setup-ruby/actions/runs/13817559138/job/38654730800?pr=721#step:3:18
i.e. as if The raw log gives:
So these two steps seem to interleave somehow, maybe a missing |
9877756
to
9b80b4d
Compare
FWIW the first But, we do So not a big increase in overall time. |
That first invocation is likely slow just because the JVM itself has to be paged in. I'll contemplate how to do this check more quickly in the future. Thank you for the assistance. |
JRuby 10 requires Java 21. Since the previous default was 17 and all JRuby releases should work fine on 21, we do this for all JRuby installs.
Implements #718