-
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
Update documentation about using GraalVM configuration files #36491
Merged
gsmet
merged 1 commit into
quarkusio:main
from
zakkak:2023-10-16-update-resources-reflect-config
Oct 16, 2023
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Is this option entirely gone or it's just that you need to stick to conventions and let GraalVM detect them?
Same question for the other option below.
Asking because IIRC, using these files could be useful/more flexible but maybe that's not the case anymore?
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.
The latter. There is no plan to remove these options in the foreseeable future, but users should not be encouraged to use them as they can change or get removed anytime.
For resources, this files are going to move to "glob patterns" any way, meaning that even if someone relies on the added flexibility at some point they won't be able to use them.
Regarding reflection I believe that
io.quarkus.runtime.annotations.RegisterForReflection
covers most cases, and if there are some not covered we could work in extending it instead of asking users to create their own config files.As a more defensive approach we could keep the docs and add a warning that the way the config files are used is planned to change and users should refrain from using them.
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.
My proposal would have been to keep the content, remove the mention of the option and enforce the file name.
But I can see your point too. So I let you decide what you prefer. If you prefer your PR as is, let me know and I'll merge 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.
That's not going to work, at least not without changing the way it works now. Currently Quarkus copies all json config files in the jars it generates, but only uses them in the native-image if the user explicitly defines the said options. My understanding is that this is done to prevent unintended bloat.
I have no strong opinion. The options I see are:
This PR, were we drop the support of custom configs entirely.
Leave the documentation as is and adding a note mentioning that this is an "on your own option" and whoever uses it is responsible to make sure the configuration files are compatible with the Mandrel or GraalVM version they use, as well as for making sure they pass any additional flags to avoid warnings and build errors.
Allow for custom configurations that we integrate in the json configuration files that Quarkus generates anyway, this will remove the need for using
ResourceConfigurationFiles
andReflectionConfigurationFiles
but will complicate the code handling the corresponding annotations and the generation of the json config files.I think I prefer 2. Let me work on it and submit another PR.