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.

Sign up for GitHub

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

Jump to bottom

Workaround graal#3003 by passing IncludeResources via AutomaticFeature #13473

Merged
merged 1 commit into from
Dec 2, 2020
Merged

Workaround graal#3003 by passing IncludeResources via AutomaticFeature #13473

merged 1 commit into from
Dec 2, 2020

Conversation

ppalaga
Copy link
Contributor

@ppalaga ppalaga commented Nov 25, 2020 •
edited
Loading

instead of command line argument

Fix #13472

@ghost
Copy link

ghost commented Nov 25, 2020

Thanks for your pull request!

The title of your pull request does not follow our editorial rules. Could you have a look?

This message is automatically generated by a bot.

@ghost ghost added the area/core label Nov 25, 2020
@ppalaga
Copy link
Contributor Author

ppalaga commented Nov 26, 2020

I have improved the comments in f671833. Could you please re-review, @mkouba ?

mkouba
mkouba approved these changes Nov 26, 2020
View reviewed changes
Copy link
Contributor

@mkouba mkouba left a comment

Choose a reason for hiding this comment

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

LGTM but it would be good if someone more specialized on native image generation could review this PR as well. Maybe @machi1990 or @gwenneg?

@machi1990
Copy link
Member

LGTM but it would be good if someone more specialized on native image generation could review this PR as well. Maybe @machi1990 or @gwenneg?

Me neither, my knowledge on this (especially usage of automatic feature) is basic. Perhaps we could get one of @dmlloyd @zakkak @galderz to review this?

@ppalaga
Copy link
Contributor Author

ppalaga commented Nov 27, 2020

Any gizmo expert could please review this?

gsmet
gsmet requested changes Nov 30, 2020
View reviewed changes
Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

Added a small comment.

Shouldn't we have a test for that?

Because we are generating code that we don't test? Or is there an existing test for that?

@ppalaga
Copy link
Contributor Author

ppalaga commented Nov 30, 2020

houldn't we have a test for that?

Because we are generating code that we don't test? Or is there an existing test for that?

Yes, there is io.quarkus.it.main.ResourcesTestCase. It is passing on GraalVM 20.2.0 and failing on GraalVM 20.3.0 before this fix and passing again after this fix.

zakkak
zakkak approved these changes Dec 1, 2020
View reviewed changes
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.

I don't feel qualified to comment on the code, but the changes look good to me.
I have also tested the changes with mandrel 20.3.0.0.Beta2 and they do fix the issue with ResourcesITCase

@ppalaga
Copy link
Contributor Author

ppalaga commented Dec 1, 2020

@zakkak our of my curiosity: are there more tests failing on GraalVM/Mandrel 20.3.0?

@zakkak
Copy link
Contributor

zakkak commented Dec 1, 2020

@zakkak our of my curiosity: are there more tests failing on GraalVM/Mandrel 20.3.0?

Yes, Redis Client (#12434) and from the Main suite io.quarkus.it.main.ImageIOITCase are failing (probably #13567 related)

@zakkak zakkak mentioned this pull request Dec 1, 2020
Merged
@ppalaga
Copy link
Contributor Author

ppalaga commented Dec 2, 2020

Is this still waiting for a Gizmo expert review? (Sorry @gsmet, I don't know if you count as one.)

@gsmet gsmet merged commit 9cc3d42 into quarkusio:master Dec 2, 2020
@ghost ghost added this to the 1.11 - master milestone Dec 2, 2020
@gsmet
Copy link
Member

gsmet commented Dec 2, 2020

I don't really count as one but it's simple enough for me to check :).

@zakkak zakkak mentioned this pull request Dec 3, 2020
Closed
This was referenced Dec 17, 2020
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mkouba mkouba mkouba approved these changes

@gsmet gsmet gsmet approved these changes

@zakkak zakkak zakkak approved these changes

Assignees
No one assigned
Labels
area/core
Projects
None yet
Milestone
1.11.0.Beta1
Development

Successfully merging this pull request may close these issues.

Workaround graal#3003 by passing IncludeResources via AutomaticFeature instead of command line argument
5 participants
@ppalaga @machi1990 @zakkak @gsmet @mkouba