-
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
Initial native image agent with JVM mode tests integration #36826
Conversation
.resolve(Path.of("generated-native-config", "resource-config.json")); | ||
if (agentResourceConfigPath.toFile().exists()) { | ||
final Pattern ignoreResources = Pattern.compile( | ||
"^((?!(microprofile|quarkus|smallrye|jboss|junit|slf4j|application.properties|surefire|logging.properties|Test.class|java/lang|java/util)).)*$"); |
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.
what makes something qualified /necessary to have in this list?
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.
Good question.
I created the list based on the output of the native image agent config for the getting started reactive sample JVM mode unit tests, see here.
Something more dynamic would likely be needed here. I guess there could be some general starting point for this list, and then based on the extensions in use, expand it?
* A json format reader. | ||
* It follows the <a href="https://www.json.org/json-en.html>ECMA-404 The JSON Data Interchange Standard.</a>. | ||
*/ | ||
public class JsonReader { |
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 just know that this class is going to be misused to add more json stuff....
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.
That can of worms was open when the Json
class was added to generate build output metrics, which Foivos' then reused to generate the json config output for native image :)
<skipITs>false</skipITs> | ||
</properties> | ||
<build> | ||
<plugins> |
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 is just for tests....but how about 'quarkus run' / mvn quarkus:run
- should that be configured somehow?
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.
Yeah that should be done too. I'll look into 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.
@maxandersen I've looked into add quarkus run integration but I should be deferred to a later stage (we can create a follow up issue). Gradle quarkusRun
does not seem to work at all. It starts Quarkus and then finishes the build, see here. Also, there are no integration tests under gradle
that verify quarkusRun
works at all. Native image agent integration with run targets should be added after that is fixed.
@galderz that's definitely possible, but we would need to explicitly pass these config files to native-image using I wonder if we could add some kind of comment in the build items and output it in the json files, in the future we could augment the proposed Either way would be acceptable I believe. |
Minimal sounds good, as long as the user can include and edit the generated parts in their source code repository. Things might become tricky though when later you do an update and rerun |
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.
Very interesting and less complicated than I expected. Nice work @galderz.
.../deployment/src/main/java/io/quarkus/deployment/steps/NativeImageAgentReflectConfigStep.java
Outdated
Show resolved
Hide resolved
Files.write(path, content.getBytes(StandardCharsets.UTF_8)); | ||
} catch (IOException e) { | ||
throw new MojoExecutionException("Unable to write native image agent access filter file", e); | ||
} |
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.
Ultimately we will need the corresponding code for gradle.
don't we control the exeuction of tests and run enough to run with different agents? feel like i recall @stuartwdouglas or @geoand mention we could register agents? that could avoid need of manipulating pom.xml/build.gradle? |
@maxandersen we can fully control the launch of |
This got me thinking. I think the best way to use this integration would be to run it locally, with GraalVM installed locally, take the generated config and add it to your source tree, as you already suggest above. The main reason for doing this is because we would not be able to execute If the user then wants to add additional configuration manually via JSON, it would work best if the files were differently named to the ones that were added to the source tree as a result of So, going back to your other comment about different files vs reasons, I think different files would work better given the above. |
Thanks all for the feedback. I've had a quick chat with @zakkak to clarify a few things. I'll work on the next iteration of the feature and I'll post an update when ready. |
...deployment/src/main/java/io/quarkus/deployment/steps/NativeImageAgentResourceConfigStep.java
Outdated
Show resolved
Hide resolved
b99a50d
to
6d556e4
Compare
I've just pushed a new prototype that works like this:
I have the following questions, doubts or comments:
|
when you are generating the missing declarations, it would be nice to display it somewhere readable. for instance in the build logs. |
Great work, @galderz. I like the new approach even better :) |
Fair enough. I could add some log messages that specify the folders where the native image agent generated config files and the transformed ones are located. Does that sound good? |
ideally I would give more control to the developer. at least in our env, dev teams do not have access to the CI workspace. they have to go through the CI team to get an extract. |
Ok, I understand the situation now. I think an option to show native image agent configuration files in the console would make sense to cover such scenario, but it would work best if only the post-processed configuration would be shown. So, not what comes out directly out of the native image agent, but after having that filtered out to remove things that Quarkus takes care of. For the most basic of Quarkus applications that has a just returns a greeting message, these are the sizes of files generated by native image agent: $ l ./new-project/target/native-agent-base-config
total 872
drwxr-xr-x 9 galder staff 288B 7 Dec 09:45 .
drwxr-xr-x 18 galder staff 576B 7 Dec 09:46 ..
-rw-r--r-- 1 galder staff 967B 7 Dec 09:45 jni-config.json
-rw-r--r-- 1 galder staff 86B 7 Dec 09:45 proxy-config.json
-rw-r--r-- 1 galder staff 406K 7 Dec 09:45 reflect-config.json
-rw-r--r-- 1 galder staff 8.6K 7 Dec 09:45 resource-config.json
-rw-r--r-- 1 galder staff 2.1K 7 Dec 09:45 serialization-config.json The biggest file there is by quite a big marging
The reflection configuration has dropped to 55K. Technically this could be further brought down but it's not easy for Quarkus to know exactly, because these remaining registrations are all related to |
yes
yes exactly |
if/when your reporting feature is available, I would instruct the teams to review the generated content systematically, as it is smell to detect something. I think this should be logged as a WARN |
I had a follow up chat with @aloubyansky last week and I'm working on a further iteration of this. I'm also including the feedback that @vsevel provided. I will update once I have something to show once again. |
Co-authored-by: Alexey Loubyansky <[email protected]>
Co-authored-by: Alexey Loubyansky <[email protected]>
Co-authored-by: Alexey Loubyansky <[email protected]>
Co-authored-by: Alexey Loubyansky <[email protected]>
This comment has been minimized.
This comment has been minimized.
@aloubyansky Pushed commits to fix your comments. |
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.
Thanks!
Status for workflow
|
Status for workflow
|
awesome @galderz |
just tried out in just released |
Prototype implementation for #36822
Here are the implementation details:
It adds a new profile, called
native-with-agent
to the generated projects where the JVM mode tests run with the native image agent. This profile sets the native image agent parameter necessary to run the JVM mode tests with. This parameter includes references to filters that are produced before running JVM mode tests.Before the JVM mode tests run,
GenerateCodeTestsMojo
has been modified to generated the native image agent filters are produced. This enables filtering out most of the configuration that is quarkus specific, but crucially it doesn't handle resources. One caveat of this early filtering is that we don't have context of the application to do more fine grained filtering. An alternative implementation would skip filtering at this level and instead filter at the build step phase, just like resources are filtered (read on).During JVM mode tests, native image agent generates config files inside
target/generated-native-config
.Then, native image agent specific build steps pick up this configuration, parse it, filtering using regex and produce existing build items, e.g.
ReflectiveClassBuildItem
,NativeImageResourcePatternsBuildItem
...etc.There is a builder class that produces json but there's no class to read json files, so I've gone ahead and created a small, dependency-free, json parser called
JsonReader
for this. The objective of this parser was to get something working quickly and without dependencies, so I'm sure there's a lot of ways in which it can be improved :)As mentioned above, resources cannot be filtered at the native image agent level, so I post-processed the resources configuration file using the
JsonReader
parser, along with regex, to filter out resources that Quarkus takes care of.One important note here is that both the native image configuration that Quarkus produces, and the one extracted from the native image agent ends up in the same configuration files inside the generated bytecode. Achieving this is quite simple because we just reuse the same existing build items. But it might make debugging easier if the files were different so that we can distinguish between those Quarkus produces and those that come from the native image agent more easily. Not sure this is doable, @zakkak? Otherwise, you can always run the app with
-Dnative
and-Dnative-with-agent
and compare the json files (see a demonstration below).I've tested out this in a new project generated from this branch called
new-project
(attached new-project-0.1.tar.gz). This is a small example that requires reflection to be configured forAlice
class lookup in order to respond to the/hello/Alice
endpoint. The native image agent integration works when the native integration test for that endpoint is a success. Comparing the output of the following commands with-Dnative
and-Dnative-with-agent
you can see what impact the native image agent jvm mode test integration has on the configuration:It has added
Alice
class and itssayMyName
method to the reflection configuration, which is required in order for endpoint to respond.There are also differences in the resource configuration but they are not relevant here:
One thing of interest in the resource differences is that it uncovers a bug in the native image integration. It's escaping patterns when it shouldn't, see #36823.
Final note, what to filter for is up for debate. I was quite aggressive in the native image agent filters because there really wasn't need for any of the additional configuration that it generated, but certain filters are debatable. Not sure what is the best approach here. Do we prefer minimal filtering that we now for sure Quarkus takes care of? Or do we prefer aggressive filtering? I think I would probably go for minimal because the aim here is to get something that native image builds and works. The user can trim things later if necessary.