Add AOT/Native sample w/ integration test#120
Conversation
|
Also, if this is what we need, should we now, as I did, write that the build will not succeed until the missing hints problem is fixed? |
2f54286 to
10b8b09
Compare
There was a problem hiding this comment.
Great first pass at this @panic08 !
A have left some specific comments inline. Other than that, here are some general thoughts:
So we can build native images using Buildpacks or w/ GraalVM native build tools.
I think we should support and talk about both.
Buildpacks
With buildpacks we build w/ one of these:
mvn -Pnative spring-boot:build-image
OR
gradle bootBuildImage
Then run in Docker like:
docker run --rm -p 8080:8080 docker.io/library/myproject:0.0.1-SNAPSHOT
Native Build Tools
Requirement here is that we have GraalVM installed on the machine. I use skdman locally and do something like:
$ sdk install java 22.3.r17-nik
$ sdk use java 22.3.r17-nik
I am not sure how we go about handling this in an automated fashion though. I believe we can leverage org.junit.jupiter.api.condition.EnabledInNativeImage to guard the tests in this case.
Note
Let's not worry about the scaffolding or executing it w/ native tools but let's make sure that the steps we end up w/ in the README/help are valid and we can do this locally.
After that the steps to build the native executable are..
mvn -Pnative native:compile
OR
gradle nativeCompile
And to run
target/grpc-server-aot
OR
build/native/nativeCompile/grpc-server-aot
Wdyt?
...rver-aot/src/test/java/org/springframework/grpc/sample/GrpcServerHealthIntegrationTests.java
Outdated
Show resolved
Hide resolved
...rpc-server-aot/src/test/java/org/springframework/grpc/sample/GrpcServerIntegrationTests.java
Outdated
Show resolved
Hide resolved
samples/grpc-server-aot/src/test/resources/application-ssl.properties
Outdated
Show resolved
Hide resolved
samples/grpc-server-aot/src/main/resources/application.properties
Outdated
Show resolved
Hide resolved
...META-INF/native-image/org.springframework.samples/grpc-server-sample/native-image.properties
Outdated
Show resolved
Hide resolved
0fdbeda to
75cef70
Compare
|
Thank you so much for the review @onobc I've fixed the unnecessary things you pointed out and brought the module as close as possible to what is generated in start.spring.io
I actually added the If we run a native build with the However, we will see some warnings in the logs. Here are the warnings that appear if I start building natively with If we build with Build Packs, everything will also start and run fine, these warnings will still appear during the build phase |
|
Do we really need a completely new sample? Wouldn't the existing grpc-server sample be minimal enough, and it already has the native plugin configured in Maven? |
We take the new module as an it and its domain is exactly testing this functionality, as I think the new module will be not bad for this, what do you think? |
|
I don't know what you mean. The old module is just fine IMO (it even works OOTB in Maven - for Gradle I had to add some settings). |
My reasoning on creating a single module is:
I would propose (if we go forward w/ single AOT/native sample) that we remove the AOT/native mentions/configs from the other samples. That being said, my goal is more to have AOT/native in a single sample; whether it be in the existing grpc-server sample or a new one. |
|
I don't really see how this separate module is better than the one we already have (the |
I updated my previous comment w/ the goal of just having the native/AOT in a single place (rather than its own module). Is that a good compromise? |
|
We discussed this during our team meeting today and have agreed to put this AOT/native into the existing Do you mind targeting the relevant changes to the |
I guess we decided not to make a separate module after all, but to have just grpc-server. Then I guess as it sounds, of the changes that will be moved to grpc-server, it's only HELP-gradle.md, HELP-maven.md. Anyway, after I move to grpc-server, I guess the build results will be the same as here #120 (comment). So the build is still successful? Now it is worth to add I guess workflow as dsyer said, which confirms that the build is successful? |
|
Did you test the native image build (maven and gradle)? It didn't work for me in Gradle. |
In my current module native build via gradle works, I've discounted the log above. I'll try to see what's wrong with the |
We want to hold off on the workflows for a bit as there are some that we may be able leverage from other Spring projects. |
75cef70 to
d0d2140
Compare
I have native image builds working successfully with both Build Packs and Native Build Tools (gradle and maven respectively). I run all this on Ubuntu 22.04, also using sdkman with dependency: |
d0d2140 to
4310365
Compare
onobc
left a comment
There was a problem hiding this comment.
Awesome job @panic08 !
I have a minor ask on the README, but other than that this looks great.
|
By the way, what do you think about adding HELP-.md to each sample? Since we gave up a separate module with aot + native image, I think it would be worth supporting native images for each sample then. And right now we have such support (added Then I think we should add HELP-.md to each sample? |
I'm actually in favor of removing native from all other samples as it can become a maintenance headache. I am in favor of doing it in one place and doing it well. Let's just focus on |
4310365 to
726f8f4
Compare
Signed-off-by: Andrey Litvitski <andrey1010102008@gmail.com>
726f8f4 to
31f2bab
Compare
onobc
left a comment
There was a problem hiding this comment.
Looks good @panic08 ! Thanks for another great contribution.
Since I don't have much expertise in this area, I spent some time, but I still managed to combine Spring AOT + native image. I did this by adding the
org.springframework.boot.aotplugin for AOT, and to make it all native, I addedBP_NATIVE_IMAGE=trueto the environment.And everything is as described in the issue, we can't get the project to build. When trying to build, we will see the following log:
Is it just me, or is it just that problem related to missing hints?
fix: #116