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

Enable override attribute on Netty dependencies #51

Merged
merged 6 commits into from
Sep 19, 2022

Conversation

sdeleuze
Copy link
Collaborator

@gradinac Could you please review this PR? We should also probably update the repository version to 0.2.0-SNAPSHOT in order to express that the format has changed.

sdeleuze added a commit to sdeleuze/native-build-tools that referenced this pull request Sep 12, 2022
This commit adds support for overriding reachability
metadata when the override flag is set to true, designed
to be used with oracle/graalvm-reachability-metadata#51.
It is implemented by adding the required --exclude-config
build arguments.

Closes graalvmgh-268
sdeleuze added a commit to sdeleuze/native-build-tools that referenced this pull request Sep 12, 2022
This commit adds support for overriding reachability
metadata when the override flag is set to true, designed
to be used with oracle/graalvm-reachability-metadata#51.
It is implemented by adding the required --exclude-config
build arguments.

Closes graalvm#268
@sdeleuze
Copy link
Collaborator Author

I had to include empty metadata to override Netty configurations, should I add empty tests or do we have a way to exclude those for the checks?

@lazar-mitrovic
Copy link
Contributor

I would personally prefer if we had explicit tests for each part of Netty (especially since we are now excluding existing config, and we should check for regressions).

Alternatively we could link it to the existing netty-transport test or some dummy test, but I'm afraid that would set a bad precedent.

@lazar-mitrovic
Copy link
Contributor

  • Prior to merging this we need to implement override support for TCK in order to be able to properly test metadata.
    This shouldn't be hard, fix shouldn't be more than couple of lines long. I'll take a look at that this week.

sdeleuze added a commit to sdeleuze/native-build-tools that referenced this pull request Sep 13, 2022
This commit adds support for overriding reachability
metadata when the override flag is set to true, designed
to be used with oracle/graalvm-reachability-metadata#51.
It is implemented by adding the required --exclude-config
build arguments.

Closes graalvm#268
@sdeleuze
Copy link
Collaborator Author

sdeleuze commented Sep 13, 2022

To add some tests, don't we need NBT 0.9.14 to be released first? Isn't it a kind of chicken and egg problem? If so we can maybe more forward with empty tests for now and add proper testing as a second step after NBT 0.9.14 and metadata repo 0.2.0 releases.

@lazar-mitrovic
Copy link
Contributor

To add some tests, don't we need NBT 0.9.14 to be released first? Isn't it a kind of chicken and egg problem? If so we can maybe more forward with empty tests for now and add proper testing as a second step after NBT 0.9.14 and metadata repo 0.2.0 releases.

Well, we already have a different code path for metadata location detection here. We aren't using metadata support artifact here since it isn't what we are testing - we are just using standard primitives and relying on TCK to inject the right values for us, and thus we don't rely on NBT version at all.

@sdeleuze
Copy link
Collaborator Author

sdeleuze commented Sep 14, 2022

Ok please let me know when you have done the required update and if you can add the tests.

melix pushed a commit to graalvm/native-build-tools that referenced this pull request Sep 18, 2022
* Fix FileUtilsTest

* Remove unused imports in FileUtils

* Add support for override flag

This commit adds support for overriding reachability
metadata when the override flag is set to true, designed
to be used with oracle/graalvm-reachability-metadata#51.
It is implemented by adding the required --exclude-config
build arguments.

Closes #268

* Fix NativeImagePlugin.addExcludeConfigArg and related documentation

* Remove unused import

* Fix functional tests
@sdeleuze
Copy link
Collaborator Author

Rebased on top of the commit mentioned in this comment and after various tries, I decided to skip tests for empty metadata.

@lazar-mitrovic lazar-mitrovic merged commit 275d58c into oracle:master Sep 19, 2022
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.

2 participants