Skip to content
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

Fail the build if CDS_TRAINING_JAVA_TOOL_OPTIONS is set with BP_SPRING_AOT_ENABLED=true #494

Merged
merged 1 commit into from
Jul 16, 2024

Conversation

anthonydahanne
Copy link
Member

@anthonydahanne anthonydahanne commented Jul 10, 2024

* it could break apps that had AOT'ed some migration library like flyway during build, which would need some extra code (java) to not automatically use them at runtime (and training run...) see: spring-projects/spring-boot#41348
* after confirmation we won't ever re introduce AOT support along with CDS in Spring Boot buildpack, we'll deprecate the feature

If the application is AOT instrumented (presence of META-INF/native-image folder) AND BP_SPRING_AOT_ENABLED is set to true AND CDS_TRAINING_JAVA_TOOL_OPTIONS is set
* fail the build with "build failed because of invalid user configuration" - the reason being is that the AOT classes used during training run won't be compatible with a different set of JAVA_TOOL_OPTIONS at runtime
* the Spring team explains this issue in detail here: spring-projects/spring-boot#41348

Checklist

  • I have viewed, signed, and submitted the Contributor License Agreement.
  • I have linked issue(s) that this PR should close using keywords or the Github UI (See docs)
  • I have added an integration test, if necessary.
  • I have reviewed the styleguide for guidance on my code quality.
  • I'm happy with the commit history on this PR (I have rebased/squashed as needed).

@anthonydahanne anthonydahanne requested a review from a team as a code owner July 10, 2024 19:16
@anthonydahanne anthonydahanne added type:documentation A documentation update semver:patch A change requiring a patch version bump labels Jul 10, 2024
@sdeleuze
Copy link

+1 for this change as the flag gives a false sense that AOT and CDS can be combined with Buildpacks, which is not true for some use cases as demonstrated in the issue linked above (this is a by design problem, not something we can fix easily with current Spring Boot and Buildpacks capabilities).

I also add here that this change should not block anybody since people not impacted by the issue described above can still manually set -Dspring.aot.enabled=true in JAVA_TOOL_OPTIONS and CDS_TRAINING_JAVA_TOOL_OPTIONS.

@anthonydahanne anthonydahanne changed the title undocument AOT support Fail the build if CDS_TRAINING_JAVA_TOOL_OPTIONS is set with BP_SPRING_AOT_ENABLED=true Jul 16, 2024
@anthonydahanne anthonydahanne added semver:minor A change requiring a minor version bump type:enhancement A general enhancement and removed type:documentation A documentation update semver:patch A change requiring a patch version bump labels Jul 16, 2024
@anthonydahanne
Copy link
Member Author

Hello @dmikusa , @pivotal-david-osullivan @sdeleuze 👋

Please read the updated title and description and changeset for this PR.

Then, know that you can test it out with this: anthonydahanne/petclinic-efficient-container@62d6873

If you do, you will experience this, because it's now forbidden to have CDS_TRAINING_JAVA_TOOL_OPTIONS set when BP_SPRING_AOT_ENABLED is true:

./mvnw spring-boot:build-image -DskipTests
[...]
[INFO]     [creator]     ===> DETECTING
[INFO]     [creator]     target distro name/version labels not found, reading /etc/os-release file
[INFO]     [creator]     paketo-buildpacks/ca-certificates   3.8.2
[INFO]     [creator]     paketo-buildpacks/bellsoft-liberica 10.8.1
[INFO]     [creator]     paketo-buildpacks/syft              1.47.1
[INFO]     [creator]     paketo-buildpacks/executable-jar    6.10.1
[INFO]     [creator]     paketo-buildpacks/dist-zip          5.8.1
[INFO]     [creator]     paketo-buildpacks/spring-boot       {{.version}}
[...]
[INFO]     [creator]     Paketo Buildpack for Spring Boot {{.version}}
[INFO]     [creator]       https://github.com/paketo-buildpacks/spring-boot
[INFO]     [creator]       Build Configuration:
[INFO]     [creator]         $BPL_JVM_CDS_ENABLED                 false  whether to enable CDS optimizations at runtime
[INFO]     [creator]         $BPL_SPRING_AOT_ENABLED              false  whether to enable Spring AOT at runtime
[INFO]     [creator]         $BP_JVM_CDS_ENABLED                  true   whether to enable CDS & perform JVM training run
[INFO]     [creator]         $BP_SPRING_AOT_ENABLED               true   whether to enable Spring AOT
[INFO]     [creator]         $BP_SPRING_CLOUD_BINDINGS_DISABLED   false  whether to contribute Spring Boot cloud bindings support
[INFO]     [creator]         $BP_SPRING_CLOUD_BINDINGS_VERSION    1      default version of Spring Cloud Bindings library to contribute
[INFO]     [creator]       Launch Configuration:
[INFO]     [creator]         $BPL_SPRING_CLOUD_BINDINGS_DISABLED  false  whether to auto-configure Spring Boot environment properties from bindings
[INFO]     [creator]         $BPL_SPRING_CLOUD_BINDINGS_ENABLED   true   Deprecated - whether to auto-configure Spring Boot environment properties from bindings
[INFO]     [creator]     ERROR: CDS_TRAINING_JAVA_TOOL_OPTIONS is not compatible with BP_SPRING_AOT_ENABLED - as the AOT classes used during training run won't be compatible with a different set of JAVA_TOOL_OPTIONS at runtime 
[INFO]     [creator]     The Spring team explains this issue in detail here: https://github.com/spring-projects/spring-boot/issues/41348 
[INFO]     [creator]     If you need to provide CDS_TRAINING_JAVA_TOOL_OPTIONS (to disable a connection to a remote service for example), you need to disable BP_SPRING_AOT_ENABLED 
[INFO]     [creator]     
[INFO]     [creator]     Paketo Buildpack for Spring Boot {{.version}}
[INFO]     [creator]       build failed because of invalid user configuration
[INFO]     [creator]     ERROR: failed to build: exit status 1
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE

Please let us know what you think! Thanks!

@sdeleuze
Copy link

sdeleuze commented Jul 16, 2024

Thanks @anthonydahanne, I think this is a great tradeoff as it still allows to use the AOT flag, even potentially with CDS but not with the problematic custom training run Java options.

That brings clarity, the options supported are the one we are able to fully support, and the documentation provides pointers for users wanting to dig deeper.

All good from my POV 👍🏼

Copy link
Contributor

@dmikusa dmikusa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All sounds good to me

@dmikusa dmikusa added type:enhancement A general enhancement and removed type:enhancement A general enhancement labels Jul 16, 2024
@anthonydahanne anthonydahanne merged commit 802d50d into main Jul 16, 2024
15 of 17 checks passed
@anthonydahanne anthonydahanne deleted the undocument-aot branch July 16, 2024 12:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver:minor A change requiring a minor version bump type:enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants