-
Notifications
You must be signed in to change notification settings - Fork 2k
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
add a module for Spring Boot 3 AOT #2457
add a module for Spring Boot 3 AOT #2457
Conversation
return (generationContext, beanFactoryInitializationCode) -> { | ||
RuntimeHints hints = generationContext.getRuntimeHints(); | ||
String[] classNames = new String[]{ | ||
"com.google.gson.JsonElement",// |
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.
these class names need to be secured by unit-test, in case the linked classes are renamed/moved in the future. we can do this in a follow-up.
spring-aot/pom.xml
Outdated
<!-- | ||
todo : we don't need this after 24 november, 2022 | ||
--> | ||
<repositories> |
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.
nit:how about we lift/centralize these repository declarations to the parent pom?
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.
they won't be required after the 24th. no need to add them at all, afaict
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.
Let's wait until after the 24th and get these removed.
spring-aot/pom.xml
Outdated
<dependency> | ||
<groupId>org.reflections</groupId> | ||
<artifactId>reflections</artifactId> | ||
<version>0.10.2</version> |
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.
nit: move the versioned dependency to the parent pom and let the children modules inherit.
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 idea
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.
can you run the formatter so that Verify Source Format
job can pass?
@joshlong the new module will block the compilation of test jobs running on platforms older than java17, i think we need to explicitly exclude |
what formatter? |
ive addressed most of the issues, i think. i hope this works :) I think you mean |
.github/workflows/maven.yml
Outdated
@@ -22,12 +22,34 @@ jobs: | |||
java-version: 8.0.x | |||
- name: Verify Format and License | |||
run: mvn spotless:check | |||
legacy-build: |
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'd really rather not maintain two different build targets, it's a recipe for drift and cut/paste errors.
Is there a way that we can combine them together with a conditional depending on the java version, that should be possible.
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.
pom.xml
Outdated
@@ -561,5 +562,21 @@ limitations under the License. | |||
<module>fluent-gen</module> | |||
</modules> | |||
</profile> | |||
<profile> |
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 don't know if it is possible in a pom, but is there a way to inherit this? I'd really prefer to avoid duplication if we can avoid 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.
i'm not sure about that, now it's already declared in the root-level pom. as far as i know, it's not inheriting from any other project/places.
spring-aot/pom.xml
Outdated
<dependency> | ||
<groupId>org.springframework.boot</groupId> | ||
<artifactId>spring-boot-dependencies</artifactId> | ||
<version>3.0.0-SNAPSHOT</version> |
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.
Can we wait until this is released, I'd rather not depend on a snapshot version.
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.
Absolutely ;) that'll be on Thursday this week. Happy thanksgiving if you celebrate! I am certainly grateful for everything y'all are doing in this great project
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.
let's switch to non-snapshot release after that
<plugin> | ||
<groupId>org.jacoco</groupId> | ||
<artifactId>jacoco-maven-plugin</artifactId> | ||
<version>0.8.8</version> |
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.
pull this version up to the parent pom (this may be a problem in all of our poms...)
@joshlong exactly, looks like all the tests passed now |
Hi - i just updated the PR to use Spring Boot 3 GA! Happy day! The PR removes all mentions of snapshots and snapshot repositories. I also avoid redundantly declaring the Spring Boot BOM in the child module, overriding the Also, I am not sure if this was intentional or if I missed something in my earlier PR, but I noticed that the |
b859f45
to
985765a
Compare
Signed-off-by: yue9944882 <291271447@qq.com>
985765a
to
3d710de
Compare
mvn clean test -q -B --define=org.slf4j.simpleLogger.log.org.apache.maven.cli.transfer.Slf4jMavenTransferListener=warn | ||
if [ $(grep -E '^(8|11)\.' <<< '${{ matrix.java }}') ]; then | ||
# some module doesn't compile on java platform lower than 17, need to skip them by specifying a profile | ||
MODS_OVERRIDES='-pl !spring-aot' |
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.
excluding spring-aot
for java8 and java11 build ^^^
@brendandburns @joshlong i appended another commit to this thread so that the test workflow can pass after we bump java version to 17. the java 8/11 compatibility is meanwhile assured by #2457 (comment) |
whats the next step? |
@joshlong it looks like https://github.com/kubernetes-client/java/pull/2457/files#r1029896501 hasn't been resolved yet, can you move the plugin declaration to the root? |
Hi - what version? The and the rest of the XML in that |
i think @brendandburns is talking about the version of the jacoco plugin, but yeah there're some other |
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.
/lgtm
/approve
/hold
ready to merge it, before that holding this pull a few days if @brendandburns has additional review comments
/lgtm We can do the pom cleanup in a different PR. @joshlong it would be great if we could get your example controller merged into the examples directory. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: brendandburns, joshlong, yue9944882 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
I'll get the example controller cleaned up and send a separate PR. It was a lot of fun working with y'all so far. Thanks for allowing me the opportunity to contribute, even if ever so slightly, to your much-appreciated efforts. |
this supercedes my last PR. in this example I've preserved Spring Framework 5 / Spring Boot 2 / Java 8 support in the
client-java-spring-integration
module, adding only one text file for Spring Boot 3 support.I've also created a new module,
client-java-spring-aot-integration
, that contributes GraalVm hints using Spring Boot 3's AOT engine. This module depends on Spring Framework 6 / Spring Boot 3 / Java 17 / the third party Reflections library.I've built a trivial controller here https://github.com/kubernetes-native-java/controllers-101 and linked it against the
-aot
module in this PR, and it works :)