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

Place json configuration files next to thin jar when building #5566

Merged
merged 1 commit into from Nov 19, 2019

Conversation

geoand
Copy link
Contributor

@geoand geoand commented Nov 18, 2019

This makes it very easy and predicable to configure
add extra json files to configure GraalVM natives.
Furthermore, this makes these files available to
docker when a container build of the native image
is being used - something which fails without this change.

Fixes: #5390

@geoand
Copy link
Contributor Author

geoand commented Nov 18, 2019

I could add tests for these if you feel it's warranted, but they would of course slow down the build a lot.
I tested by creating a maven and gradle project then adding a json config file for GraalVM and finally building using both regular and docker builds.

Copy link
Contributor

@gastaldi gastaldi left a comment

Choose a reason for hiding this comment

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

Added some comments

Copy link
Contributor

@gastaldi gastaldi left a comment

Choose a reason for hiding this comment

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

LGTM

This makes it very easy and predicable to configure
add extra json files to configure GraalVM natives.
Furthermore, this makes these fails available to
docker when a container build of the native image
is being used.

Fixes: quarkusio#5390
@geoand geoand added the area/user-experience Will make us lose users label Nov 18, 2019
@geoand geoand added this to the 1.1.0 milestone Nov 18, 2019
@geoand
Copy link
Contributor Author

geoand commented Nov 18, 2019

@gsmet are you ok with this one?

@geoand geoand changed the title Place json configuration files next to thin jar Place json configuration files next to thin jar when building Nov 18, 2019
@stuartwdouglas
Copy link
Member

I don't really like this solution. Even though it improves the status quo I still don't think it provides a great user experience.

Maybe the approach should be:

  • Everything in the application (src/main/resources) is included by default (otherwise why would it be in the application if it was not needed?)
  • Additional resources can be included through either an annotation that specifies the paths to include, or a config file in a well known location (quarkus-resources.properties?) that we parse at build time and then automatically handle.
  • Maybe the ability to include resources via application.properties, but I don't know if this is really config

I think we should discuss this, as the user should not have to mess around with JSON files or the substrate command line to include some resources in their image.

@geoand
Copy link
Contributor Author

geoand commented Nov 19, 2019

@stuartwdouglas you have my vote when it comes to adding everything in src/main/resources, I had even opened an issue about it: #2746.

This PR didn't really attempt to solve that problem generally. It was motivated primarily by the fact that currently one can't due a native build using docker and specify the json config files

@stuartwdouglas stuartwdouglas merged commit c42baca into quarkusio:master Nov 19, 2019
@geoand
Copy link
Contributor Author

geoand commented Nov 19, 2019

Even if we do add some dedicated configuration options for resource and reflection handling (plus whatever other json file could be added), we'll still need the copy mechanics of this PR.

I do like the idea of having dedicated quarkus.native.reflection-config-files or something like that to users can just specify what they want to include.

@geoand geoand deleted the #5390 branch November 19, 2019 06:24
@gsmet gsmet modified the milestones: 1.1.0, 1.0.0.Final Nov 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/user-experience Will make us lose users
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adding a reflection configuration file fails when building native using Docker
4 participants