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

Rename reflection-config.json into reflect-config.json and resources-config.json into resource-config.json #38118

Merged
merged 1 commit into from
Feb 2, 2024

Conversation

dliubars
Copy link
Contributor

Updating config file names to be consistent with those specified here: https://www.graalvm.org/latest/reference-manual/native-image/metadata/

Copy link

github-actions bot commented Jan 10, 2024

🙈 The PR is closed and the preview is expired.

-H:ResourceConfigurationFiles=resources-config.json,\
-H:ResourceConfigurationFiles=resource-config.json,\
Copy link
Member

Choose a reason for hiding this comment

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

I will let @zakkak chime in but do we even need to register the file if we are following the convention?

Copy link
Contributor

@zakkak zakkak left a comment

Choose a reason for hiding this comment

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

Thanks for the update @dliubars.

Please note that after the renaming of the files, passing ResourceConfigurationFiles and ReflectionConfigurationFiles is no longer required.

As stated in https://github.com/oracle/graal/blob/master/docs/reference-manual/native-image/AutomaticMetadataCollection.md

The generated configuration files can be supplied to the native-image tool by placing them in a META-INF/native-image/ directory on the class path. This directory (or any of its subdirectories) is searched for files with the names jni-config.json, reflect-config.json, proxy-config.json, resource-config.json, predefined-classes-config.json, serialization-config.json which are then automatically included in the build process. Not all of those files must be present. When multiple files with the same name are found, all of them are considered.

If the user/dev wants to place the files in a different directory (i.e. one not containing META-INF/native-image/ in its path or a directory not included in the classpath) then -H:ConfigurationFileDirectories should be used instead:

A directory containing configuration files that is not part of the class path can be specified to native-image via -H:ConfigurationFileDirectories=/path/to/config-dir/. This directory must directly contain all files: jni-config.json, reflect-config.json, proxy-config.json and resource-config.json. A directory with the same metadata files that is on the class path, but not in META-INF/native-image/, can be provided via -H:ConfigurationResourceRoots=path/to/resources/. Both -H:ConfigurationFileDirectories and -H:ConfigurationResourceRoots can also take a comma-separated list of directories.

@dliubars
Copy link
Contributor Author

Hi @zakkak, thank you for the review!

Please note that after the renaming of the files, passing ResourceConfigurationFiles and ReflectionConfigurationFiles is no longer required.

As I understand it, the documentation suggests placing config files under src/main/resources/, not src/main/resources/META-INF/native-image/, it notes:
Please also note that the resource-config.json file will be overwritten by Quarkus if placed directly under src/main/resources/META-INF/native-image/ as Quarkus generates its own configuration file in this directory.

In my understanding, specifying ResourceConfigurationFiles remains necessary in this case because the config files are not located under META-INF/native-image/, where they would be automatically picked up by native-image. This page mentions ResourceConfigurationFiles.

If the user/dev wants to place the files in a different directory (i.e. one not containing META-INF/native-image/ in its path or a directory not included in the classpath) then -H:ConfigurationFileDirectories should be used instead:

As far as I understand, ConfigurationFileDirectories can also be used. However, in this case, one must follow a strict naming convention, which seems less flexible than using ResourceConfigurationFiles.

I apologize for any confusion. The intention of this PR was not to alter the proposed configuration method, but rather to use file names consistent with Graal (whether these files are automatically picked up or specified explicitly). I should have provided some context: when I was adapting langchain4j to be ready for use in native, I have used several guides (including this one) and spent some time figuring out why my configuration in resources-config.json was ignored by Graal. It was only later when I realized that resources-config.json != resource-config.json and that Graal expects resource-config.json. Hence, I updated this page to use naming which will work in both cases (implicit/convention and explicit configuration), to prevent others without Graal experience (like me) from going through a similar frustrating experience.

Please feel free to modify or reject this PR, or let me know what should be changed.
Thank you.

@zakkak
Copy link
Contributor

zakkak commented Jan 11, 2024

In my understanding, specifying ResourceConfigurationFiles remains necessary in this case because the config files are not located under META-INF/native-image/, where they would be automatically picked up by native-image

Indeed, in this case the extra flags are necessary. Perhaps we should change the docs and ask devs to place the config files under src/main/resources/META-INF/native-image/their/domain/ to make things simpler. No strong opinions here.

Please also note that the resource-config.json file will be overwritten by Quarkus if placed directly under src/main/resources/META-INF/native-image/ as Quarkus generates its own configuration file in this directory.

Correct, but this doesn't prevent us from adding them under a different directory in the src/main/resources/META-INF/native-image/ tree, e.g. under src/main/resources/META-INF/native-image/their/domain/.

This page mentions ResourceConfigurationFiles.

Interesting, we need to fix this in upstream GraalVM documentation, since ResourceConfigurationFiles is considered "experimental" and requires passing -H:+UnlockExperimentalVMOptions as well.

As far as I understand, ConfigurationFileDirectories can also be used. However, in this case, one must follow a strict naming convention, which seems less flexible than using ResourceConfigurationFiles.

I see what you mean, but that's the only non-experimental option I am aware of.

I apologize for any confusion. The intention of this PR was not to alter the proposed configuration method, but rather to use file names consistent with Graal (whether these files are automatically picked up or specified explicitly). I should have provided some context: when I was adapting langchain4j to be ready for use in native, I have used several guides (including this one) and spent some time figuring out why my configuration in resources-config.json was ignored by Graal. It was only later when I realized that resources-config.json != resource-config.json and that Graal expects resource-config.json. Hence, I updated this page to use naming which will work in both cases (implicit/convention and explicit configuration), to prevent others without Graal experience (like me) from going through a similar frustrating experience.

No need for apologies, I totally understand the motivation behind this PR, I am just trying to make sure we provide the best possible instructions to save people from further confusion in the future :)

Please feel free to modify or reject this PR, or let me know what should be changed.

  1. Please avoid mentioning any experimental options (like ResourceConfigurationFiles) unless strictly necessary.
  2. I suggest instructing users to place the configuration files under src/main/resources/META-INF/native-image/their/domain/, e.g. src/main/resources/META-INF/native-image/net/zakkak/, to avoid the need for additional flags in the common case scenario.
  3. Place a note about the ability to place the config files in any directory/ies and point native-image to it using ConfigurationFileDirectories.

Note that only 1 is a blocker, 2 and 3 are just my suggestions and are up for discussion.
Thank you

@zakkak
Copy link
Contributor

zakkak commented Jan 22, 2024

Hi @dliubars, do you have any updates on that?

Note that in the meantime #38186 was created, which seems to tackle my second point.

Copy link
Member

@galderz galderz left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. These changes still promote using experimental flags that produce warning messages. I propose this PR to be closed and instead integrate #38186.

@dliubars
Copy link
Contributor Author

Hi, pardon for the late reply! Sure, feel free to close this PR.

Copy link
Contributor

@zakkak zakkak left a comment

Choose a reason for hiding this comment

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

@galderz it looks like @dliubars updated the PR and it now only includes the following changes (i.e. it no longer suggests using experimental options):

  1. replace references to resources-config.json with resource-config.json
  2. replace references to reflection-config.json with reflect-config.json

I suggest you either:

  1. merge this PR and rebase Update native configuration file best practices #38186
    or
  2. cherry-pick @dliubars commit in from this PR in Update native configuration file best practices #38186

@galderz galderz merged commit aec0d69 into quarkusio:main Feb 2, 2024
5 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.9 - main milestone Feb 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

4 participants