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

[GR-30205] Extend the agent to collect the locale and classname of resource bundles. #3557

Conversation

graalvmbot
Copy link
Collaborator

@graalvmbot graalvmbot commented Jul 7, 2021

This PR extends the resource-config.json configuration file structure and adds two new fields locales and classNames for each bundle. By using these entries one can narrow down which bundles should be included, the default being all selected locales.

Specifying bundles using classNames has the advantage that they don't have to be looked up and instantiated at build time, which can be dangerous.

NativeImageAgent was updated to provide more detailed configuration using classNames.

@graalvmbot graalvmbot force-pushed the github/d-kozak/GR-30205/update-agent-to-support-localization-better branch from 5811da0 to 38c2154 Compare July 16, 2021 13:37
@graalvmbot graalvmbot force-pushed the github/d-kozak/GR-30205/update-agent-to-support-localization-better branch from 8b50a6d to 8eb8a5a Compare July 26, 2021 16:57
@graalvmbot graalvmbot force-pushed the github/d-kozak/GR-30205/update-agent-to-support-localization-better branch 4 times, most recently from 8778ff7 to 967cf2d Compare August 18, 2021 11:52
@graalvmbot graalvmbot force-pushed the github/d-kozak/GR-30205/update-agent-to-support-localization-better branch 3 times, most recently from 811a9fb to b0cdc3e Compare August 26, 2021 08:22
@graalvmbot graalvmbot force-pushed the github/d-kozak/GR-30205/update-agent-to-support-localization-better branch from 23adb58 to 3155f86 Compare September 14, 2021 11:05
@graalvmbot graalvmbot force-pushed the github/d-kozak/GR-30205/update-agent-to-support-localization-better branch from a398adf to 6fe6f2f Compare September 25, 2021 09:45
@peter-hofer peter-hofer self-requested a review October 11, 2021 14:08
@graalvmbot graalvmbot force-pushed the github/d-kozak/GR-30205/update-agent-to-support-localization-better branch 2 times, most recently from 39a892f to d516367 Compare October 22, 2021 11:11
@graalvmbot graalvmbot force-pushed the github/d-kozak/GR-30205/update-agent-to-support-localization-better branch from d516367 to 860f582 Compare October 22, 2021 11:31
}

@Override
public void addResourceBundles(ConfigurationCondition condition, String basename, Collection<Locale> locales) {
Copy link
Member

Choose a reason for hiding this comment

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

This class is used not only in the image build, but also in the agent and the native-image-configure images at runtime. Could using Locale objects here be an issue since these images might not include the locales in question? If so, it would be preferable to use plain strings instead and do any checks during the image build.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good question. Fortunately, it should not be a problem - it is possible to create Locale objects even for non-existent locales such as aa-bb-ccccc. The method parseLocaleFromTag uses the locale tag parsing from JDK, which only cares about the syntatic structure of the tag, again aa-bb-ccccc works here. And there is also a fallback for handling locales with invalid tags (driven from a test case in the JCK). So creating the Locale objects on their own should not be an issue, even in an image that does not have corresponding locale data included.

Comment on lines +92 to +93
bundleNameField.set(bundle, basename);
bundleLocaleField.set(bundle, locale);
Copy link
Member

Choose a reason for hiding this comment

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

Could setting these fields reflectively break ResourceBundle implementations?

Copy link
Contributor

@d-kozak d-kozak Nov 1, 2021

Choose a reason for hiding this comment

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

As far as I know, it should not. There is no special logic in their getters and setters and in the ResourceBundle class, they are used only during the lookup process which is completely substituted in the optimized mode. There is no suspicious use outside the class either. I am setting them reflectively because they are also set during the lookup, see ResourceBundle.loadLookup. Without setting them, the class bundles in the optimized mode would have no locale and basename associated, which would be inconsistent with the JDK. The whole idea of not instantiating bundles and only preparing their classes for reflection, unfortunately, does not work with the older optimized mode, in which everything is stored in the map at build time, so I have to instantiate them in that scenario anyway.

@graalvmbot graalvmbot force-pushed the github/d-kozak/GR-30205/update-agent-to-support-localization-better branch from a17caba to f516400 Compare November 1, 2021 11:08
@graalvmbot graalvmbot merged commit a53da53 into master Nov 2, 2021
KengoTODA added a commit to KengoTODA/graalvm-native-image-plugin that referenced this pull request Jan 28, 2022
KengoTODA added a commit to KengoTODA/graalvm-native-image-plugin that referenced this pull request Jan 29, 2022
* ci: upgrade GraalVM versions

* ci: fix workflows

* fix: add necessary metadata to new versions

* fix: support Java 17

* fix: add options to the gu command

* fix: support the latest format of ResourceBundleUsage

refs oracle/graal#3557

* chore: reformat

* fix: stop using Java9 feature for Java8 compatibility

Arrays.compare() is not ready to use for Java8

* fix: handle nullness of lists properly

* fix: handle nullness of lists properly
@graalvmbot graalvmbot deleted the github/d-kozak/GR-30205/update-agent-to-support-localization-better branch February 8, 2022 01:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants