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

Add GraalVM native-image configuration #1959

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

mhalbritter
Copy link

@mhalbritter mhalbritter commented Jul 26, 2022

This allows HikariCP to be used in a GraalVM native image without additional configuration.

The JSON configuration files have been generated by running 'run-tests-with-agent.sh', which executes the test suite with the native-image-agent attached and then generates the configuration metadata located in src/main/resources.

References:

This allows HikariCP to be used in a native image without additional configuration.

The JSON configuration files have been generated by running 'run-tests-with-agent', which executes the test suite with the native-image-agent attached and then generates the configuration metadata located in src/main/resources.
@mhalbritter mhalbritter changed the title Add native-image configuration Add GraalVM native-image configuration Jul 26, 2022
@codecov
Copy link

codecov bot commented Jul 26, 2022

Codecov Report

Merging #1959 (cff775c) into dev (ef99317) will decrease coverage by 0.04%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##                dev    #1959      +/-   ##
============================================
- Coverage     70.37%   70.33%   -0.05%     
+ Complexity      572      571       -1     
============================================
  Files            26       26              
  Lines          2164     2164              
  Branches        311      311              
============================================
- Hits           1523     1522       -1     
  Misses          492      492              
- Partials        149      150       +1     
Impacted Files Coverage Δ
...ain/java/com/zaxxer/hikari/util/ConcurrentBag.java 75.53% <0.00%> (-1.07%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ef99317...cff775c. Read the comment docs.

@mhalbritter
Copy link
Author

Not sure why the codecov is red, i didn't touch any code.

@mhalbritter
Copy link
Author

Hello! Any chance that this will be merged soon?

@lfbayer
Copy link
Collaborator

lfbayer commented Aug 1, 2022

Forgive my ignorance, but is it normal for the library providers to have to make changes to support GraalVM?

@mhalbritter
Copy link
Author

mhalbritter commented Aug 1, 2022

Thanks for asking! There are 3 possibilities for libraries:

  1. ignore GraalVM and let users deal with a workaround
  2. don't include GraalVM metadata and let users contribute to the shared GraalVM reachability metadata repository. Once the metadata files are in there, all users can benefit from them
  3. include the metadata files in their own source tree

It's up to you to decide how you want to handle this. GraalVM proponents would prefer option 3, but option 2. would work too. I've already contributed hints here. I prefer the hints closer to the code which does the proxying / reflection etc. and that's why I have created the PR. But don't feel bad declining the PR if you don't want to have this metadata in your code.

@lfbayer
Copy link
Collaborator

lfbayer commented Sep 27, 2022

@mhalbritter, as a best practice generated code should generally not be committed. Is it standard practice for projects to commit all of these files directly? Is there not some maven plugin that can generate these automatically at build time?

It sounds like these files would need to be regenerated with any future code changes and it is unlikely that the maintainers would know what kind of changes would impact GraalVM metadata. I suspect this would mean someone will eventually and unwittingly cause the GraalVM metadata to be wrong.

I would prefer a solution similar to how the apache felix maven plugin generates the OSGi manifest. Felix is just a simple plugin in the pom.xml and everything is automatic.

If there is not a simpler way to support this, I would recommend your "option 2" until such a time as the GraalVM ecosystem can support a less invasive method.

@mhalbritter
Copy link
Author

These files have been originally created by running the test suite with the GraalVM agent attached. These files are only correct if the test suite has 100% coverage. The idea behind putting the GraalVM hints into the codebase is that the authors can update the hints files when changing the Java code (for example if more reflection is done). You can't easily auto-generate these GraalVM hints like you can with, for example, an OSGi manifest.

Comment on lines +671 to +679
<agent>
<options>
<option>experimental-conditional-config-part</option>
</options>
<options name="test">
<option>access-filter-file=${project.basedir}/native/access-filter.json</option>
<option>caller-filter-file=${project.basedir}/native/caller-filter.json</option>
</options>
</agent>
Copy link

Choose a reason for hiding this comment

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

  • As a reminder, you have not configured native/user-code-filter.json

Choose a reason for hiding this comment

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

  • Sorry, I noticed it in the shell script

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants