-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Handle new format of used_classes_* reports in GraalVM for JDK 24 #41935
Conversation
Starting with GraalVM for JDK 24 the format of the report has changed prefixing each line with the class loader name and a colon, e.g.: GraalVM for JDK 22 (and 23 which is not released yet): ``` org.postgresql.jdbc.PgSQLXML ``` GraalVM for JDK 24: ``` com.oracle.svm.hosted.NativeImageClassLoader:org.postgresql.jdbc.PgSQLXML ``` Closes quarkusio#41917
Status for workflow
|
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.
Ah nice to have it handled before we switch to it!
I added a small question.
// Starting with GraalVM for JDK 24 the format of the report has changed prefixing each line with | ||
// the class loader name and a colon. We need to strip that part. | ||
String[] line = scanner.nextLine().split(":"); | ||
set.add(line[line.length - 1]); |
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.
Given the comment, I would rather use indexOf()
and substring()
? Or you are absolutely sure there won't be any other colon?
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.
For the time being it's at most one colon, something like [<classloader>:]<classname>
, that's why I went for the simplest approach. If more colons show up in the future it will still work as long as the classname still appears last in the sequence (and the classname can't contain a colon). If something gets added after the classname then the test will break and we will notice.
I don't think it's worth "validating" the file's format, at least not for the time being.
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.
Let's merge then, thanks for the confirmation!
Starting with GraalVM for JDK 24 the format of the report has changed
prefixing each line with the class loader name and a colon, e.g.:
GraalVM for JDK 22 (and 23 which is not released yet):
GraalVM for JDK 24:
See oracle/graal@1d769c7#diff-01f83a129b979abfb9acd79deb25f1db51df940f9d4ca968c301cd9718bf627bR347
Closes #41917