Switch from AutomaticFeature to Feature and pass in via --feature #25976

zakkak · 2022-06-06T13:13:55Z

While the Feature interface is API, the annotation AutomaticFeature is
not in the API. That is intentional: if a feature is necessary, then it
should be added via the --features=... option in a
native-image.properties file.

Source: oracle/graal#4616

Implements 1st part of #25943

Marking as Draft till tested completes on my fork.

gastaldi · 2022-06-06T13:42:40Z

@zakkak there are other Feature classes implemented using @AutomaticFeature in the codebase (extensions like Liquibase, Hibernate, to name a few).

~~Should they be migrated to this strategy too?~~

UPDATE: Just saw it's part of #25943

zakkak · 2022-06-06T16:53:35Z

@zakkak there are other Feature classes implemented using @AutomaticFeature in the codebase (extensions like Liquibase, Hibernate, to name a few).

~~Should they be migrated to this strategy too?~~

UPDATE: Just saw it's part of #25943

Yup I forgot about those. We will need to do the migration for them as well and see how to only include them when it makes sense (i.e. based on reachability).

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

core/deployment/src/main/java/io/quarkus/deployment/builditem/GraalVmFeatureBuildItem.java

gastaldi · 2022-06-08T03:31:45Z

Looks good to me, added a minor suggestion for better type check purposes

gastaldi

We use GraalVM in other parts, so better follow the pattern :)

core/deployment/src/main/java/io/quarkus/deployment/builditem/GraalVmFeatureBuildItem.java

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

sberyozkin · 2022-06-08T09:52:08Z

@stuartwdouglas Can you also have a look please ?

gastaldi

LGTM

...ions/security/deployment/src/main/java/io/quarkus/security/deployment/SecurityProcessor.java

gsmet

It looks good to me except for one small interrogation regarding nested classes and escaping.

gsmet · 2022-06-09T09:50:13Z

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

+                for (NativeImageFeatureBuildItem nativeImageFeature : nativeImageFeatures) {
+                    featuresList.add(nativeImageFeature.getQualifiedName());
+                }
+                nativeImageArgs.add("--features=" + String.join(",", featuresList));


Will this work properly if one of the feature is a nested class? I'm worried about $ escaping.

That's an interesting question, I'll give it a try.

PS: Why would one define a feature as a nested class though?

PS: Why would one define a feature as a nested class though?

People are weird :).

That's a good question, and possible :)

According to my tests the $ is being properly escaped but we still can't use nested classes for defining a Feature even when using @AutomaticFeature (i.e. without this PR).

native-image fails with:

Error: Error instantiating Feature class io.quarkus.runtime.graal.DisableLoggingFeature$NestedDisableLoggingFeature. Ensure the class is not abstract and has a no-argument constructor. com.oracle.svm.core.util.UserError$UserException: Error instantiating Feature class io.quarkus.runtime.graal.DisableLoggingFeature$NestedDisableLoggingFeature. Ensure the class is not abstract and has a no-argument constructor. at org.graalvm.nativeimage.builder/com.oracle.svm.core.util.UserError.abort(UserError.java:84) at org.graalvm.nativeimage.builder/com.oracle.svm.hosted.FeatureHandler.registerFeature(FeatureHandler.java:180) at org.graalvm.nativeimage.builder/com.oracle.svm.hosted.FeatureHandler.registerFeatures(FeatureHandler.java:128) at org.graalvm.nativeimage.builder/com.oracle.svm.hosted.NativeImageGenerator.setupNativeImage(NativeImageGenerator.java:836) at org.graalvm.nativeimage.builder/com.oracle.svm.hosted.NativeImageGenerator.doRun(NativeImageGenerator.java:559) at org.graalvm.nativeimage.builder/com.oracle.svm.hosted.NativeImageGenerator.run(NativeImageGenerator.java:519) at org.graalvm.nativeimage.builder/com.oracle.svm.hosted.NativeImageGeneratorRunner.buildImage(NativeImageGeneratorRunner.java:407) at org.graalvm.nativeimage.builder/com.oracle.svm.hosted.NativeImageGeneratorRunner.build(NativeImageGeneratorRunner.java:585) at org.graalvm.nativeimage.builder/com.oracle.svm.hosted.NativeImageGeneratorRunner.main(NativeImageGeneratorRunner.java:128) Caused by: java.lang.NoSuchMethodException: io.quarkus.runtime.graal.DisableLoggingFeature$NestedDisableLoggingFeature.<init>() at java.base/java.lang.Class.getConstructor0(Class.java:3349) at java.base/java.lang.Class.getDeclaredConstructor(Class.java:2553) at org.graalvm.nativeimage.base/com.oracle.svm.util.ReflectionUtil.lookupConstructor(ReflectionUtil.java:81) at org.graalvm.nativeimage.base/com.oracle.svm.util.ReflectionUtil.lookupConstructor(ReflectionUtil.java:76) at org.graalvm.nativeimage.base/com.oracle.svm.util.ReflectionUtil.newInstance(ReflectionUtil.java:95) at org.graalvm.nativeimage.builder/com.oracle.svm.hosted.FeatureHandler.registerFeature(FeatureHandler.java:178) ... 7 more

Explicitly defining the default public constructor doesn't help either.

This is essentially happening because inner classes don't actually have "a no-argument constructor" since the seemingly no-argument default constructor of inner classes takes as argument an instance of the outer class. However, GraalVM doesn't handle this case so it doesn't support defining nested Features.

static nested features should work though, no?

Good point! In that case everything works as expected ($ gets escaped and the feature gets registered).

E.g.:

... [INFO] [io.quarkus.deployment.pkg.steps.NativeImageBuildRunner] /home/zakkak/code/graal/sdk/latest_graalvm_home/bin/native-image -J-Djava.util.logging.manager=org.jboss.logmanager.LogManager -J-Dsun.nio.ch.maxUpdateArraySize=100 -J-Dio.netty.leakDetection.level=DISABLED -J-Dio.netty.allocator.maxOrder=3 -J-Dvertx.logger-delegate-factory-class-name=io.quarkus.vertx.core.runtime.VertxLogDelegateFactory -J-Dvertx.disableDnsResolver=true -J-Duser.language=en -J-Duser.country=IE -J-Dfile.encoding=UTF-8 --features=io.quarkus.runner.Feature,io.quarkus.runtime.graal.ResourcesFeature,io.quarkus.runtime.graal.DisableLoggingFeature\$NestedDisableLoggingFeature -H:-ParseOnce -J--add-exports=java.security.jgss/sun.security.krb5=ALL-UNNAMED -J--add-opens=java.base/java.text=ALL-UNNAMED -H:InitialCollectionPolicy=com.oracle.svm.core.genscavenge.CollectionPolicy\$BySpaceAndTime -H:+JNI -H:+AllowFoldMethods -J-Djava.awt.headless=true -H:FallbackThreshold=0 --link-at-build-time -H:+ReportExceptionStackTraces -g -H:DebugInfoSourceSearchPath=app-sources -H:-AddAllCharsets -H:EnableURLProtocols=http -H:NativeLinkerOption=-no-pie -H:-UseServiceLoaderFeature -H:+StackTrace -J--add-exports=org.graalvm.nativeimage.builder/com.oracle.svm.core.jni=ALL-UNNAMED -J--add-exports=org.graalvm.nativeimage.builder/com.oracle.svm.core.jdk=ALL-UNNAMED -J--add-exports=org.graalvm.nativeimage.builder/com.oracle.svm.core.jdk.localization=ALL-UNNAMED -J--add-exports=org.graalvm.nativeimage.base/com.oracle.svm.util=ALL-UNNAMED quarkus-integration-test-awt-999-SNAPSHOT-runner -jar quarkus-integration-test-awt-999-SNAPSHOT-runner.jar ... [1/7] Initializing... (17.0s @ 0.16GB) Version info: 'GraalVM 22.2.0-dev Java 11 CE' Java version info: '11.0.16+5-jvmci-22.2-b03' C compiler: gcc (redhat, x86_64, 12.1.1) Garbage collector: Serial GC 5 user-specific feature(s) - io.quarkus.awt.runtime.graal.AwtFeature - io.quarkus.runner.Feature - io.quarkus.runtime.graal.DisableLoggingFeature$NestedDisableLoggingFeature - io.quarkus.runtime.graal.ResourcesFeature - org.graalvm.home.HomeFinderFeature: Finds GraalVM paths and its version number

Notice the --features=io.quarkus.runner.Feature,io.quarkus.runtime.graal.ResourcesFeature,io.quarkus.runtime.graal.DisableLoggingFeature\$NestedDisableLoggingFeature parameter.

sberyozkin · 2022-06-09T10:06:23Z

...ions/security/deployment/src/main/java/io/quarkus/security/deployment/SecurityProcessor.java

+        }
+        return null;
+    }
+


I understand the difference now, thanks

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

geoand · 2022-06-16T05:18:31Z

This needs a rebase

zakkak · 2022-06-16T09:33:08Z

I addressed the comments and rebased the branch, it is again ready for review.

gsmet · 2022-06-16T09:42:23Z

@zakkak please run the formatter :)

While the Feature interface is API, the annotation AutomaticFeature is not in the API. That is intentional: if a feature is necessary, then it should be added via the --features=... option in a native-image.properties file. Source: oracle/graal#4616

In quarkusio#25976 we switched from using the @AutomaticFeature annotation to explicitly passing the required features using the --features parameter. Given that `@AutomaticFeature`s (including the `SerializationFeature`) are always registered before non-automatic ones there is no longer the need to have an explicit dependency on `SerializationFeture`.

quarkus-bot bot added the area/core label Jun 6, 2022

zakkak force-pushed the autofeature-to-feature branch from eff876b to 6fc0592 Compare June 6, 2022 19:08

quarkus-bot bot added area/graphics area/hibernate-validator Hibernate Validator area/hibernate-orm Hibernate ORM area/hibernate-search Hibernate Search area/persistence OBSOLETE, DO NOT USE area/security area/websockets labels Jun 6, 2022

gastaldi reviewed Jun 6, 2022

View reviewed changes

core/deployment/src/main/java/io/quarkus/deployment/pkg/steps/NativeImageBuildStep.java Outdated Show resolved Hide resolved

zakkak force-pushed the autofeature-to-feature branch 2 times, most recently from 08a35eb to 2845c08 Compare June 7, 2022 11:13

zakkak marked this pull request as ready for review June 7, 2022 11:13

zakkak requested a review from galderz June 7, 2022 11:13

This comment has been minimized.

Sign in to view

gastaldi approved these changes Jun 8, 2022

View reviewed changes

gastaldi reviewed Jun 8, 2022

View reviewed changes

core/deployment/src/main/java/io/quarkus/deployment/builditem/GraalVmFeatureBuildItem.java Outdated Show resolved Hide resolved

gastaldi requested changes Jun 8, 2022

View reviewed changes

core/deployment/src/main/java/io/quarkus/deployment/builditem/GraalVmFeatureBuildItem.java Outdated Show resolved Hide resolved

zakkak force-pushed the autofeature-to-feature branch from 2845c08 to 2cae1f6 Compare June 8, 2022 05:28

zakkak requested a review from gastaldi June 8, 2022 05:30

This comment has been minimized.

Sign in to view

zakkak force-pushed the autofeature-to-feature branch from 2cae1f6 to 662c980 Compare June 8, 2022 05:43

gastaldi reviewed Jun 8, 2022

View reviewed changes

core/deployment/src/main/java/io/quarkus/deployment/pkg/steps/NativeImageBuildStep.java Outdated Show resolved Hide resolved

zakkak force-pushed the autofeature-to-feature branch from 662c980 to 95a371f Compare June 8, 2022 10:54

gastaldi approved these changes Jun 8, 2022

View reviewed changes

gastaldi reviewed Jun 8, 2022

View reviewed changes

...ions/security/deployment/src/main/java/io/quarkus/security/deployment/SecurityProcessor.java Outdated Show resolved Hide resolved

zakkak force-pushed the autofeature-to-feature branch from 95a371f to 71ec3a8 Compare June 8, 2022 13:09

gastaldi requested a review from stuartwdouglas June 8, 2022 14:22

This comment has been minimized.

Sign in to view

zakkak force-pushed the autofeature-to-feature branch 2 times, most recently from 5561b92 to ba749b7 Compare June 9, 2022 09:12

gsmet reviewed Jun 9, 2022

View reviewed changes

sberyozkin reviewed Jun 9, 2022

View reviewed changes

gastaldi added the triage/needs-rebase This PR needs to be rebased first because it has merge conflicts label Jun 9, 2022

geoand reviewed Jun 10, 2022

View reviewed changes

core/deployment/src/main/java/io/quarkus/deployment/pkg/steps/NativeImageBuildStep.java Outdated Show resolved Hide resolved

zakkak force-pushed the autofeature-to-feature branch from ba749b7 to 8c53bb7 Compare June 16, 2022 09:31

This comment has been minimized.

Sign in to view

zakkak force-pushed the autofeature-to-feature branch from 8c53bb7 to a4724d5 Compare June 16, 2022 09:52

gsmet approved these changes Jun 16, 2022

View reviewed changes

gsmet added triage/waiting-for-ci Ready to merge when CI successfully finishes and removed triage/needs-rebase This PR needs to be rebased first because it has merge conflicts labels Jun 16, 2022

gastaldi merged commit 57fb652 into quarkusio:main Jun 17, 2022

quarkus-bot bot added this to the 2.11 - main milestone Jun 17, 2022

quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jun 17, 2022

zakkak deleted the autofeature-to-feature branch August 12, 2022 05:56

zakkak mentioned this pull request Aug 12, 2022

[GR-40198] Provide public API for feature-based JNI / Resource / Proxy / Serialization registration. oracle/graal#4783

Merged

zakkak mentioned this pull request Aug 12, 2022

Drop getRequiredFeatures method from io.quarkus.runner.Feature #27263

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch from AutomaticFeature to Feature and pass in via --feature #25976

Switch from AutomaticFeature to Feature and pass in via --feature #25976

zakkak commented Jun 6, 2022

gastaldi commented Jun 6, 2022 •

edited

Loading

zakkak commented Jun 6, 2022

This comment has been minimized.

gastaldi commented Jun 8, 2022

gastaldi left a comment

This comment has been minimized.

sberyozkin commented Jun 8, 2022

gastaldi left a comment

This comment has been minimized.

gsmet left a comment

gsmet Jun 9, 2022

zakkak Jun 9, 2022

gsmet Jun 9, 2022

gastaldi Jun 9, 2022

zakkak Jun 16, 2022

zakkak Jun 16, 2022

geoand Jun 16, 2022

zakkak Jun 16, 2022

sberyozkin Jun 9, 2022

geoand commented Jun 16, 2022

zakkak commented Jun 16, 2022

This comment has been minimized.

gsmet commented Jun 16, 2022

Switch from AutomaticFeature to Feature and pass in via --feature #25976

Switch from AutomaticFeature to Feature and pass in via --feature #25976

Conversation

zakkak commented Jun 6, 2022

gastaldi commented Jun 6, 2022 • edited Loading

zakkak commented Jun 6, 2022

This comment has been minimized.

gastaldi commented Jun 8, 2022

gastaldi left a comment

Choose a reason for hiding this comment

This comment has been minimized.

sberyozkin commented Jun 8, 2022

gastaldi left a comment

Choose a reason for hiding this comment

This comment has been minimized.

gsmet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geoand commented Jun 16, 2022

zakkak commented Jun 16, 2022

This comment has been minimized.

gsmet commented Jun 16, 2022

gastaldi commented Jun 6, 2022 •

edited

Loading