Avoid getting the GraalVM version at deployment time for AWT #43829

zakkak · 2024-10-11T13:11:13Z

Reduces the times we start a container when using a builder-image

quarkus-bot · 2024-10-11T13:11:33Z

Karm · 2024-10-11T15:47:58Z

I'm sorry for the noise, I've just realized this is a draft. I'm gonna wait how it unfolds first.

Btw, testing with runtime container too, which is not a part of the regular GHA here, is a good thing to try with changes:

(locally Java 17)
$ ./mvnw verify -f integration-tests/pom.xml -pl awt -Ddocker -Dnative 
  -Dquarkus.native.container-build=true -Dquarkus.container-image.build=true 
  -Dquarkus.native.builder-image=quay.io/quarkus/ubi-quarkus-mandrel-builder-image:jdk-21 
  -Dquarkus.native.container-runtime=podman 2>&1 | tee log.log

gsmet · 2024-10-16T13:08:29Z

extensions/awt/runtime/src/main/java/io/quarkus/awt/runtime/graal/AwtFeature.java

+        if (JavaVersionUtil.isJava19OrHigher()) {
+            try {
+                RuntimeJNIAccess.register(Class.forName("sun.font.FontUtilities"));
+            } catch (ClassNotFoundException e) {
+                throw new RuntimeException(e);
+            }
+        }
+    }


Question: we are still supporting GraalVM/Mandrel for Java 17 in Quarkus 3.17+?

I thought we had all sorts of issues with it?

We don't support it (as in we will probably not go to great extends to fix something not working there), but we also don't forbid Quarkus to use it, see #39866 and #43455. As long as it's not getting in the way we prefer to stay somewhat compatible with it for the time being.

OK. FWIW I'm in favor of dropping it whenever you feel comfortable.

gsmet · 2024-10-17T08:21:46Z

@Karm any chance you could have a look at this one before next Tuesday? I'd let to get all these relatively closely related patches into 3.16 if possible.

Karm · 2024-10-17T08:58:29Z

@gsmet Yes, I'll do that. Bumping the priority.

KarmBot · 2024-10-17T09:03:52Z

Adjusting priority for my human worker @Karm renice -n --16

Karm

Hello @zakkak @gsmet,

False Negatives

I took a look and it seems that it's hard to verify any AWT related changes as the test have been disabled for some time. At least since this I guess?

One would assume that -DskipITs might control the failsafe, but it's not the case. Both in-container and hosted tests are skipped. The harness builds the native image but doesn't run any tests with it. In case of in-container tests, the harness builds the naitive-image, builds a container image and copies the executable in it, starts the container, but doesn't do any testing with it.

I had to manually change:

diff --git a/integration-tests/awt/pom.xml b/integration-tests/awt/pom.xml
index f71302da2c2..eb290a5e037 100644
--- a/integration-tests/awt/pom.xml
+++ b/integration-tests/awt/pom.xml
@@ -164,7 +164,7 @@
             <plugin>
                 <artifactId>maven-failsafe-plugin</artifactId>
                 <configuration>
-                    <skip>true</skip>
+                    <skip>false</skip>
                 </configuration>
             </plugin>
         </plugins>

This PR branch fails 🔴

To obtain results.
In case of this PR branch, HEAD 942c089, and host JDK 17.0.13 and Builder image version: Mandrel-23.1.4.0-Final (build 21.0.4+7-LTS) I got a failure that indicates that different images than expected were generated. Usual failure is that fonts weren't loaded so that's why pixels differ where text was supposed to be. I am yet to dump those images to take a look.

[ERROR] Failures: 
[ERROR]   ImageGeometryFontsIT>ImageGeometryFontsTest.testGeometryAndFonts:89 TIFF: Wrong pixel at 80x56. Expected: [46,14,32,255] Actual: [194,59,132,255] ==> expected: <true> but was: <false>
[ERROR]   ImageGeometryFontsIT>ImageGeometryFontsTest.testGeometryAndFonts:89 GIF: Wrong pixel at 80x56. Expected: [2,0,0,0] Actual: [156,0,0,0] ==> expected: <true> but was: <false>
[ERROR]   ImageGeometryFontsIT>ImageGeometryFontsTest.testGeometryAndFonts:89 PNG: Wrong pixel at 80x56. Expected: [46,14,32,255] Actual: [194,59,132,255] ==> expected: <true> but was: <false>
[ERROR]   ImageGeometryFontsIT>ImageGeometryFontsTest.testGeometryAndFonts:89 JPG: Wrong pixel at 80x56. Expected: [73,0,44,0] Actual: [201,53,137,0] ==> expected: <true> but was: <false>
[ERROR]   ImageGeometryFontsIT>ImageGeometryFontsTest.testGeometryAndFonts:89 BMP: Wrong pixel at 80x56. Expected: [46,14,32,0] Actual: [194,59,132,0] ==> expected: <true> but was: <false>
[INFO] 
[ERROR] Tests run: 41, Failures: 5, Errors: 0, Skipped: 0
[INFO]

The failure is the same when I had the test run locally, with Mandrel installed locally (Mandrel-23.1.4.0-Final (build 21.0.4+7-LTS)).

Q 3.15.1 Passes 🟢

When I built & ran Quarkus 3.15.1
with the same change:

-                    <skip>true</skip>
+                    <skip>false</skip>

the test passes though. Mandrel-23.1.4.0-Final (build 21.0.4+7-LTS) both locally and as a builder-image. So I suspect it's really fonts and related to this change.

Karm · 2024-10-11T15:28:12Z

extensions/awt/deployment/src/main/java/io/quarkus/awt/deployment/AwtProcessor.java

-
-        // Added for JDK 19+ due to: https://github.com/openjdk/jdk20/commit/9bc023220 calling FontUtilities
-        if (v.jdkVersionGreaterOrEqualTo("19")) {
-            classes.add("sun.font.FontUtilities");


@zakkak So this is meant like a cleanup for JDK 21+ only , right?

No, that's moved to the new AwtFeature (https://github.com/quarkusio/quarkus/pull/43829/files#diff-086ee2e4b2043822961920521dd5be21c10c6805629f87f5b79d5836eb978e28R14).

The idea is to avoid version checks at Quarkus deployment phase (were we are still not sure what GraalVM version will be used).

zakkak · 2024-10-21T10:29:42Z

extensions/awt/deployment/src/main/java/io/quarkus/awt/deployment/AwtProcessor.java

-
-        // Added for JDK 19+ due to: https://github.com/openjdk/jdk20/commit/9bc023220 calling FontUtilities
-        if (v.jdkVersionGreaterOrEqualTo("19")) {
-            classes.add("sun.font.FontUtilities");


No, that's moved to the new AwtFeature (https://github.com/quarkusio/quarkus/pull/43829/files#diff-086ee2e4b2043822961920521dd5be21c10c6805629f87f5b79d5836eb978e28R14).

The idea is to avoid version checks at Quarkus deployment phase (were we are still not sure what GraalVM version will be used).

zakkak · 2024-10-21T10:30:13Z

extensions/awt/deployment/src/main/java/io/quarkus/awt/deployment/AwtProcessor.java

-        if (v.javaVersion.feature() != 19) {
-            classes.add("java.awt.GraphicsEnvironment");
-            classes.add("sun.awt.X11GraphicsConfig");
-            classes.add("sun.awt.X11GraphicsDevice");
-            classes.add("sun.java2d.SunGraphicsEnvironment");
-            classes.add("sun.java2d.xr.XRSurfaceData");
-        }


@Karm these are cleanup, as we no longer support JDK 19, so I always include these classes

Karm · 2024-10-21T11:32:21Z

fddb6d2 still fails for me.

zakkak · 2024-10-21T11:47:17Z

fddb6d2 still fails for me.

I managed to reproduce (and fix) the issue. PTAL when you can.

Karm · 2024-10-21T14:38:57Z

fddb6d2 still fails for me.

I managed to reproduce (and fix) the issue. PTAL when you can.

WIP...

Karm

So, with the change at b95789b I can say that the test passes with:

JDK 17.0.13 and native-image in builder image Mandrel-23.1.4.0-Final
JDK 17.0.13 and native-image locally Mandrel-23.1.5.0-Final (build 21.0.5+11-LTS)
both local and in-container testing is ok
it's also fine to have JDK 21 on both ends
JDK 17.0.13 and native-image Mandrel-24.1.1.0-Final (build 23.0.1+11) is ok.

What fails is the very latest early access JDK 24 and Mandrel master on fonts dir lookup:

Caused by: java.io.IOException: Problem reading font data.
    at java.desktop@24-beta/java.awt.Font.createFont0(Font.java:1205)
    at java.desktop@24-beta/java.awt.Font.createFont(Font.java:1076)
    at io.quarkus.awt.it.Application.init(Application.java:63)

it is unrelated to this PR though, it fails with 3.15.1 too. I need to port the substitution to the latest JDK.

THX @zakkak 🙏

quarkus-bot · 2024-10-21T21:05:12Z

Status for workflow `Quarkus CI`

This is the status report for running Quarkus CI on commit b95789b.

Failing Jobs

Status	Name	Step	Failures	Logs	Raw logs	Build scan
✖	JVM Tests - JDK 17	`Build`	⚠️ Check →	Logs	Raw logs	🚧
✔️	JVM Tests - JDK 21			Logs	Raw logs	🚧

You can consult the Develocity build scans.

zakkak · 2024-10-22T07:52:11Z

The mongodb failures seem unrelated.

quarkus-bot bot added area/core area/graphics labels Oct 11, 2024

zakkak force-pushed the 2024-10-11-awt-processor-refactor branch 4 times, most recently from 2d575aa to 942c089 Compare October 14, 2024 11:57

zakkak marked this pull request as ready for review October 14, 2024 20:40

zakkak requested a review from Karm October 14, 2024 20:40

This comment has been minimized.

Sign in to view

quarkus-bot bot added the triage/flaky-test label Oct 15, 2024

gsmet reviewed Oct 16, 2024

View reviewed changes

Karm requested changes Oct 21, 2024

View reviewed changes

zakkak commented Oct 21, 2024

View reviewed changes

zakkak force-pushed the 2024-10-11-awt-processor-refactor branch 2 times, most recently from 804d230 to fddb6d2 Compare October 21, 2024 10:46

Avoid getting the GraalVM version at deployment time for AWT

b95789b

zakkak force-pushed the 2024-10-11-awt-processor-refactor branch from fddb6d2 to b95789b Compare October 21, 2024 11:46

Karm self-requested a review October 21, 2024 14:54

Karm approved these changes Oct 21, 2024

View reviewed changes

Karm mentioned this pull request Oct 21, 2024

Some Native tests don't run due to failsafe <skip>true</skip> #43997

Closed

zakkak added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Oct 21, 2024

This comment has been minimized.

Sign in to view

zakkak merged commit 031d580 into quarkusio:main Oct 22, 2024
52 of 53 checks passed

quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Oct 22, 2024

zakkak deleted the 2024-10-11-awt-processor-refactor branch October 22, 2024 07:52

quarkus-bot bot added this to the 3.17 - main milestone Oct 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid getting the GraalVM version at deployment time for AWT #43829

Avoid getting the GraalVM version at deployment time for AWT #43829

zakkak commented Oct 11, 2024

quarkus-bot bot commented Oct 11, 2024

Karm commented Oct 11, 2024 •

edited

Loading

This comment has been minimized.

gsmet Oct 16, 2024

zakkak Oct 16, 2024 •

edited

Loading

gsmet Oct 17, 2024

gsmet commented Oct 17, 2024

Karm commented Oct 17, 2024

KarmBot commented Oct 17, 2024

Karm left a comment •

edited

Loading

Karm Oct 11, 2024

zakkak Oct 21, 2024

zakkak Oct 21, 2024

zakkak Oct 21, 2024

Karm commented Oct 21, 2024

zakkak commented Oct 21, 2024

Karm commented Oct 21, 2024

Karm left a comment

This comment has been minimized.

quarkus-bot bot commented Oct 21, 2024 •

edited by github-actions bot

Loading

zakkak commented Oct 22, 2024

Avoid getting the GraalVM version at deployment time for AWT #43829

Avoid getting the GraalVM version at deployment time for AWT #43829

Conversation

zakkak commented Oct 11, 2024

quarkus-bot bot commented Oct 11, 2024

Karm commented Oct 11, 2024 • edited Loading

This comment has been minimized.

gsmet Oct 16, 2024

Choose a reason for hiding this comment

zakkak Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

gsmet Oct 17, 2024

Choose a reason for hiding this comment

gsmet commented Oct 17, 2024

Karm commented Oct 17, 2024

KarmBot commented Oct 17, 2024

Karm left a comment • edited Loading

Choose a reason for hiding this comment

False Negatives

This PR branch fails 🔴

Q 3.15.1 Passes 🟢

Karm Oct 11, 2024

Choose a reason for hiding this comment

zakkak Oct 21, 2024

Choose a reason for hiding this comment

zakkak Oct 21, 2024

Choose a reason for hiding this comment

zakkak Oct 21, 2024

Choose a reason for hiding this comment

Karm commented Oct 21, 2024

zakkak commented Oct 21, 2024

Karm commented Oct 21, 2024

Karm left a comment

Choose a reason for hiding this comment

This comment has been minimized.

quarkus-bot bot commented Oct 21, 2024 • edited by github-actions bot Loading

Status for workflow Quarkus CI

Failing Jobs

zakkak commented Oct 22, 2024

Karm commented Oct 11, 2024 •

edited

Loading

zakkak Oct 16, 2024 •

edited

Loading

Karm left a comment •

edited

Loading

quarkus-bot bot commented Oct 21, 2024 •

edited by github-actions bot

Loading

Status for workflow `Quarkus CI`