Fix native-image arguments generation for native-sources package type #21809

zakkak · 2021-11-30T08:44:32Z

Ensures the NativeImageSecurityProvidersBuildItem (introduced in Add NativeImageSecurityProviderBuildItem #17774) is taken into account when using -Dquarkus.package.type=native-sources
Gets the GraalVM version so that it can be taken in account when generating the arguments (e.g. to avoid adding parameters that are not present in some older version)

quarkus-bot · 2021-11-30T13:04:45Z

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building `ffd1287`

Status	Name	Step	Failures	Logs	Raw logs
✖	Maven Tests - JDK 11 Windows	`Build`	Failures	Logs	Raw logs

Full information is available in the Build summary check run.

Failures

⚙️ Maven Tests - JDK 11 Windows #

- Failing: integration-tests/maven

📦 integration-tests/maven

✖ io.quarkus.maven.it.PackageIT.testNativeSourcesPackage line 274 - More details - Source on GitHub

org.opentest4j.AssertionFailedError: 

expected: 0

✖ io.quarkus.maven.it.PackageIT.testNativeSourcesPackage line 274 - More details - Source on GitHub

org.opentest4j.AssertionFailedError: 

expected: 0

gsmet · 2021-11-30T14:00:18Z

@zakkak CI is not happy.

sberyozkin

@zakkak Thanks for requesting my review, LGTM, but Guillaume would indeed be a better reviewer for this PR :-)

zakkak · 2021-11-30T20:26:18Z

@gsmet the failure is due to GraalVM not being available when running the test:

Error:  Failed to execute goal io.quarkus:quarkus-maven-plugin:999-SNAPSHOT:build (default) on project acme: Failed to build quarkus application: io.quarkus.builder.BuildException: Build failure: Build failed due to errors
Error: r]: Build step io.quarkus.deployment.pkg.steps.NativeImageBuildStep#nativeSourcesResult threw an exception: java.lang.RuntimeException: Cannot find the `native-image.cmd` in the GRAALVM_HOME, JAVA_HOME and System PATH. Install it using `gu install native-image`

According to #15208 this is a requirement, so apparently my patch is breaking this functionality. The issue with not having GraalVM or at least the version you plan to use when generating the "native-sources" is that Quarkus won't know which flags it should include and which not, e.g. in:

quarkus/core/deployment/src/main/java/io/quarkus/deployment/pkg/steps/NativeImageBuildStep.java

Lines 671 to 674 in 6276695

    
           if (graalVMVersion.isOlderThan(GraalVM.Version.VERSION_21_3)) { 
        
               // Enabled by default in GraalVM >= 21.3 
        
               nativeImageArgs.add("-H:+InlineBeforeAnalysis"); 
        
           }

and

quarkus/core/deployment/src/main/java/io/quarkus/deployment/pkg/steps/NativeImageBuildStep.java

Lines 733 to 735 in 6276695

    
           if (graalVMVersion.is(GraalVM.Version.VERSION_21_3_0) && graalVMVersion.isJava17()) { 
        
               nativeImageArgs.add("-J--add-exports=java.management/sun.management=ALL-UNNAMED"); 
        
           }

So my suggestion is to explicitly set the GraalVM version to GraalVM.Version.CURRENT when using the package type native-sources. That means users of native-sources package type will only be safe if they use the GraalVM.Version.CURRENT GraalVM/Mandrel version.

Another unknown is whether a locally installed GraalVM or a builder image will be used to generate the native executable. This seems to affect the flags passed to the linker, see:

quarkus/core/deployment/src/main/java/io/quarkus/deployment/pkg/steps/NativeImageBuildStep.java

Lines 157 to 159 in 6276695

    
           if (!isContainerBuild && SystemUtils.IS_OS_LINUX) { 
        
               noPIE = detectNoPIE(); 
        
           }

and

quarkus/core/deployment/src/main/java/io/quarkus/deployment/pkg/steps/NativeImageBuildStep.java

Lines 678 to 680 in 6276695

    
           if (!noPIE.isEmpty()) { 
        
               nativeImageArgs.add("-H:NativeLinkerOption=" + noPIE); 
        
           }

As the preferred way of building native-images is using the builder image I suggest we don't set noPie for native-sources package type.

Note, we could allow the user to set the GraalVM version along with whether the end result will be built using a container or not, but that would probably unnecessarily complicate things.

WDYT?

zakkak · 2021-12-06T10:45:26Z

@gsmet can you please advise on #21809 (comment) so that we can get this in 2.2.4?

gsmet · 2021-12-08T19:48:13Z

Sorry I was busy with the Quarkiverse move. I will have a look.

gsmet · 2021-12-08T20:28:04Z

@zakkak I would go with something like:

if it is a container build, just use CURRENT and don't do any detection
if it isn't, try detecting the version
if no version detected, use CURRENT

You're saying the fact that it is a container build is an unknown, is it really true?

Anyway, I think you can simplify and remove noPIE for now and if it becomes a problem, we can revisit.

BTW, make sure your patch applies cleanly to 2.2 or do a specific backport in a separate PR once this one is in as I won't have the time to fiddle with this myself.

Thanks.

zakkak · 2021-12-09T07:57:30Z

@zakkak I would go with something like:

* if it is a container build, just use CURRENT and don't do any detection
* if it isn't, try detecting the version
* if no version detected, use CURRENT

In the scenario that native-sources covers, Quarkus is only available in the first step where we don't know the GraalVM version that will be used in the second step. In the second step we only have access to the packages Quarkus app, the native-image.args file, and an installation of GraalVM.

The steps/algorithm you propose will only be able to be taken in the second step of the process where we are supposed to just execute native-image with the arguments provided in the generated native-image.args file. This step is expected to be executed by some automated pipeline and is out of Quarkus' control. As a result, there is nothing we can do about it and that's why I suggested defaulting to CURRENT.

You're saying the fact that it is a container build is an unknown, is it really true?

Yes, there is no way to (programmatically) know in the 1st step whether the 2nd step of the process will use a locally installed GraalVM or a builder image.

Anyway, I think you can simplify and remove noPIE for now and if it becomes a problem, we can revisit.

BTW, make sure your patch applies cleanly to 2.2 or do a specific backport in a separate PR once this one is in as I won't have the time to fiddle with this myself.

On it.

Thanks.

Thanks :)

* Ensures the NativeImageSecurityProvidersBuildItem is taken into account to add -H:AdditionalSecurityProviders as needed * Set the GraalVM version to `CURRENT`, since we can't know which version will eventually be used to build the native image. This enables us to at least make sure that the generated arguments will work properly with the currently supported GraalVM.

quarkus-bot · 2021-12-09T12:58:20Z

Failing Jobs - Building `53b8498`

Status	Name	Step	Failures	Logs	Raw logs
✔️	JVM Tests - JDK 11
✖	JVM Tests - JDK 11 Windows	`Build`	Failures	Logs	Raw logs
✔️	JVM Tests - JDK 17

Full information is available in the Build summary check run.

Failures

⚙️ JVM Tests - JDK 11 Windows #

- Failing: extensions/opentelemetry/opentelemetry/deployment 
! Skipped: extensions/opentelemetry/opentelemetry-exporter-jaeger/deployment extensions/opentelemetry/opentelemetry-exporter-otlp/deployment integration-tests/opentelemetry and 2 more

📦 extensions/opentelemetry/opentelemetry/deployment

✖ io.quarkus.opentelemetry.deployment.RestClientOpenTelemetryTest.client line 61 - More details - Source on GitHub

org.awaitility.core.ConditionTimeoutException: Assertion condition defined as a lambda expression in io.quarkus.opentelemetry.deployment.TestSpanExporter expected: <2> but was: <1> within 30 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:164)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:119)

✖ io.quarkus.opentelemetry.deployment.RestClientOpenTelemetryTest.urlWithoutAuthentication line 108 - More details - Source on GitHub

org.awaitility.core.ConditionTimeoutException: Assertion condition defined as a lambda expression in io.quarkus.opentelemetry.deployment.TestSpanExporter expected: <2> but was: <3> within 30 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:164)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:119)

gsmet · 2021-12-09T13:38:47Z

Thanks!

zakkak · 2021-12-09T15:02:58Z

BTW, make sure your patch applies cleanly to 2.2 or do a specific backport in a separate PR once this one is in as I won't have the time to fiddle with this myself.

Backport PR created: #22069

zakkak requested a review from sberyozkin November 30, 2021 08:44

quarkus-bot bot added the area/core label Nov 30, 2021

zakkak added area/native-image triage/backport-2.2? kind/bug Something isn't working labels Nov 30, 2021

gsmet approved these changes Nov 30, 2021

View reviewed changes

gsmet added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Nov 30, 2021

sberyozkin reviewed Nov 30, 2021

View reviewed changes

geoand removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Dec 1, 2021

zakkak force-pushed the fix-native-sources branch from ffd1287 to 53b8498 Compare December 9, 2021 08:33

zakkak requested a review from gsmet December 9, 2021 08:57

gsmet approved these changes Dec 9, 2021

View reviewed changes

gsmet merged commit a690b0c into quarkusio:main Dec 9, 2021

quarkus-bot bot added this to the 2.7 - main milestone Dec 9, 2021

gsmet added the triage/backport? label Dec 9, 2021

zakkak deleted the fix-native-sources branch December 9, 2021 14:43

zakkak mentioned this pull request Dec 9, 2021

Fix native-image arguments generation for native-sources package type #22069

Merged

gsmet removed this from the 2.7 - main milestone Dec 9, 2021

gsmet added this to the 2.5.2.Final milestone Dec 9, 2021

gsmet removed triage/backport-2.5? labels Dec 9, 2021

geoand mentioned this pull request Jan 4, 2022

Reflection registration when building with native-sources #22590

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix native-image arguments generation for native-sources package type #21809

Fix native-image arguments generation for native-sources package type #21809

zakkak commented Nov 30, 2021 •

edited

Loading

quarkus-bot bot commented Nov 30, 2021 •

edited

Loading

Failing Jobs - Building `ffd1287`

Failures

⚙️ Maven Tests - JDK 11 Windows #

📦 integration-tests/maven

gsmet commented Nov 30, 2021

sberyozkin left a comment

zakkak commented Nov 30, 2021 •

edited

Loading

zakkak commented Dec 6, 2021

gsmet commented Dec 8, 2021

gsmet commented Dec 8, 2021

zakkak commented Dec 9, 2021

quarkus-bot bot commented Dec 9, 2021

gsmet commented Dec 9, 2021

zakkak commented Dec 9, 2021

Fix native-image arguments generation for native-sources package type #21809

Fix native-image arguments generation for native-sources package type #21809

Conversation

zakkak commented Nov 30, 2021 • edited Loading

quarkus-bot bot commented Nov 30, 2021 • edited Loading

Failing Jobs - Building ffd1287

Failures

⚙️ Maven Tests - JDK 11 Windows #

📦 integration-tests/maven

gsmet commented Nov 30, 2021

sberyozkin left a comment

Choose a reason for hiding this comment

zakkak commented Nov 30, 2021 • edited Loading

zakkak commented Dec 6, 2021

gsmet commented Dec 8, 2021

gsmet commented Dec 8, 2021

zakkak commented Dec 9, 2021

quarkus-bot bot commented Dec 9, 2021

Failing Jobs - Building 53b8498

Failures

⚙️ JVM Tests - JDK 11 Windows #

📦 extensions/opentelemetry/opentelemetry/deployment

gsmet commented Dec 9, 2021

zakkak commented Dec 9, 2021

zakkak commented Nov 30, 2021 •

edited

Loading

quarkus-bot bot commented Nov 30, 2021 •

edited

Loading

Failing Jobs - Building `ffd1287`

zakkak commented Nov 30, 2021 •

edited

Loading

Failing Jobs - Building `53b8498`