-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Update builder images to jdk-17.0.7 #34120
Conversation
Note - unrelated to that PR, I've also deprecated the native S2i (which included Maven / Gradle...). (FYI: @tqvarnst and @maxandersen) |
@Karm seems like the AWT substitutions would need some love (see the documentation build). |
This comment has been minimized.
This comment has been minimized.
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.
You mention the image is deprecated...but it does not seem explicitly called out/mentioned anywhere ?
@@ -17,8 +17,8 @@ | |||
<properties> | |||
<!-- The Graal version we suggest using in documentation - as that's | |||
what we work with by self downloading it: --> | |||
<graal-sdk.version-for-documentation>22.3</graal-sdk.version-for-documentation> |
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.
Is this really necessary to change everywhere ? I'm here thinking of handling updates/backports of docs.
Maybe not a super big deal but just wondering why change this perfectly valid key?
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.
Because we use that variable to indicate where to download the GraalVM Community packages, and we want to recommend jdk-17.0.7 (aka 23.0)
I would say, let's fix this properly once 3.2 is out and we have more time and won't be affected by people being off |
That's the good news: the Quarkus codebase does not reference that image (but it's still pulled a lot...). I've updated the description on Quay. See https://quay.io/repository/quarkus/ubi-quarkus-graalvmce-s2i (let me know if you don't have access to the usage log) |
@maxandersen @Karm is it expected that the new SDK is compiled with Java 17 - which obviously will break our java 11 support. |
breaks some build components I reckon or do we have an actual runtime/just-for-java-not-native-image dependency on this? |
@@ -71,7 +71,7 @@ | |||
<jboss-logmanager-embedded.version>1.0.11</jboss-logmanager-embedded.version> | |||
<slf4j-jboss-logmanager.version>1.1.0.Final</slf4j-jboss-logmanager.version> | |||
<slf4j-api.version>1.7.36</slf4j-api.version> | |||
<graal-sdk.version>22.3.2</graal-sdk.version> | |||
<graal-sdk.version>23.0.0</graal-sdk.version> |
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 makes it only build on java 11.
afaics it only brings in the annotations - that library shouldn't be java 17 bound IMO.
I've pinged on graalvm slack what their recommendation is.
In worst case we stay on the old version for this as it just brings substitution annotations.
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.
imo we should for now stay on 22.3.2 for the jar dependency - then we at least can kick of right testing etc.
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.
one piece of evidence to stay on 22.3.2 is that there are no API differences:
https://diff.revapi.org/?groupId=org.graalvm.sdk&artifactId=graal-sdk&old=22.3.2&new=23.0.0
35fa7cf
to
fd7b2b2
Compare
🙈 The PR is closed and the preview is expired. |
This comment has been minimized.
This comment has been minimized.
fd7b2b2
to
96e1889
Compare
public static final Version VERSION_22_3_0 = new Version("GraalVM 22.3.0", "22.3.0", Distribution.ORACLE); | ||
public static final Version VERSION_22_2_0 = new Version("GraalVM 22.2.0", "22.2.0", Distribution.ORACLE); | ||
public static final Version VERSION_23_0_0 = new Version("GraalVM 23.0.0", "23.0.0", Distribution.ORACLE); | ||
public static final Version VERSION_23_1_0 = new Version("GraalVM 23.1.0", "23.1.0", Distribution.ORACLE); | ||
|
||
public static final Version MINIMUM = VERSION_22_2_0; | ||
public static final Version CURRENT = VERSION_22_3_2; | ||
public static final Version CURRENT = VERSION_23_0_0; |
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.
Are we going to detect the version correctly with the new version scheme? Or GraalVM still has 23 in the version string? My recollection is that you said it doesn't.
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.
Yes, that's a problem. I mentioned this in the PR description. I believe the class needs to be adjusted.
if (linesList.size() == 3) { | ||
static Version of(Stream<String> output) { | ||
List<String> lines = output | ||
.dropWhile(l -> !l.startsWith("GraalVM") && !l.startsWith("native-image")) |
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.
@gsmet How is this not dropping lines of the form (third line in the new output):
Substrate VM GraalVM CE 20.0.1+9.1 (build 20.0.1+9, serial gc)
and
OpenJDK Runtime Environment Mandrel-23.0.0.0-Final (build 17.0.7+7)
What am I missing?
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.
I think the safer option is to pull the image before running --version
on it.
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.
@jerboaa it's dropWhile
: it's dropping the lines while the condition is true, then it keeps all the lines.
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.
As for changing entirely the way it is done, I'm not comfortable to do it now but could be convinced for a future version.
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.
FWIW, I discovered this method today while implementing this :).
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.
@gsmet Gotcha. Fair enough then.
@cescoffier Could you be more specific, please? The doc build log seem ok...? |
No, I was suspecting a problem, but it was due to the broken version detection |
- Update the GraalVM Community builder image to the tag `jdk-17` / `jdk-17.0.7` - Update the Mandrel builder image to the tag `jdk-17`(Mandrel 23.0) - Update GraalVM SDK version to 23.0 (yes, it does not follow the same version scheme) Note that the version scheme and name also changed: GraalVM CE -> GraalVM Community. We kept the name of the image unchanged to avoid a massive breaking change.
It's a hack because the new jar use Java 17 bytecode. We would need to be sure that this version is not used for anything more than the SVM artifact version.
We need to filter the output coming from Docker downloading the image.
5fe6d49
to
ccd299f
Compare
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.
LGTM, I restarted a CI run with some adjusted timeouts. I'll merge this evening.
jdk-17
/jdk-17.0.7
jdk-17
(Mandrel 23.0)Note that the version scheme and name also changed: GraalVM CE -> GraalVM Community. We kept the name of the image unchanged to avoid a massive breaking change.