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

Avoid using internal API from package org.graalvm.nativeimage.impl #28093

Merged
merged 3 commits into from Sep 29, 2022

Conversation

zakkak
Copy link
Contributor

@zakkak zakkak commented Sep 20, 2022

As discussed in #27728 (comment)

@quarkus-bot
Copy link

quarkus-bot bot commented Sep 20, 2022

/cc @Karm, @galderz

@zakkak
Copy link
Contributor Author

zakkak commented Sep 27, 2022

The removal of the remaining usages requires upstream changes, see oracle/graal#5013.

@quarkus-bot

This comment has been minimized.

@zakkak zakkak marked this pull request as draft September 27, 2022 17:36
@zakkak zakkak force-pushed the runtimeinitializationsupport-public-api branch from ceddfa0 to 930d0ab Compare September 27, 2022 19:42
@zakkak zakkak marked this pull request as ready for review September 27, 2022 19:42
@quarkus-bot

This comment has been minimized.

@zakkak zakkak requested a review from galderz September 28, 2022 06:27
@zakkak
Copy link
Contributor Author

zakkak commented Sep 28, 2022

@jerboaa can you please review this?

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.

LGTM but @jerboaa should have a second look since he was involved in the original discussions.

Copy link
Contributor

@jerboaa jerboaa left a comment

Choose a reason for hiding this comment

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

Looks mostly good. I think one unintentional change from runtime->buildtime sneaked in, though.

Stops using the internal methods from the `org.graalvm.nativeimage.impl`
package.
Since `RuntimeResourceAccess#addResourceBundle` requires a module to be
passed to it we pass the unnamed module by default and extend
`NativeImageResourceBundleBuildItem` to include a module name for
specifying one when needed, e.g., for JDK internal resources.
@zakkak zakkak force-pushed the runtimeinitializationsupport-public-api branch from 930d0ab to a5c2014 Compare September 28, 2022 13:23
@zakkak zakkak requested a review from jerboaa September 28, 2022 13:23
Copy link
Contributor

@jerboaa jerboaa left a comment

Choose a reason for hiding this comment

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

LGTM

@quarkus-bot
Copy link

quarkus-bot bot commented Sep 28, 2022

Failing Jobs - Building a5c2014

Status Name Step Failures Logs Raw logs
✔️ JVM Tests - JDK 11
✔️ JVM Tests - JDK 17
JVM Tests - JDK 17 MacOS M1 ⚠️ Check → Logs Raw logs
✔️ JVM Tests - JDK 18

@zakkak zakkak merged commit 18aa6ab into quarkusio:main Sep 29, 2022
@quarkus-bot quarkus-bot bot added this to the 2.14 - main milestone Sep 29, 2022
@zakkak zakkak deleted the runtimeinitializationsupport-public-api branch September 29, 2022 06:14
gsmet pushed a commit that referenced this pull request Sep 30, 2022
@gsmet gsmet modified the milestones: 2.14 - main, 2.13.1.Final Oct 3, 2022
gsmet pushed a commit to gsmet/quarkus that referenced this pull request Oct 3, 2022
Fixes issues introduced in quarkusio#28093

(cherry picked from commit ea67abe)
igorregis pushed a commit to igorregis/quarkus that referenced this pull request Oct 16, 2022
igorregis pushed a commit to igorregis/quarkus that referenced this pull request Oct 16, 2022
igorregis pushed a commit to igorregis/quarkus that referenced this pull request Oct 16, 2022
igorregis pushed a commit to igorregis/quarkus that referenced this pull request Oct 17, 2022
tmihalac pushed a commit to tmihalac/quarkus that referenced this pull request Oct 27, 2022
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.

None yet

4 participants