-
Notifications
You must be signed in to change notification settings - Fork 65
chore: centralize graal 23 downstream updates #3674
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you test in local that the GraalVM tests in downstream libraries still pass without the native-image.properties
in their repos?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a list of local test results in the description. I ended up further adjusting the configuration, so thanks!
This reverts commit d0af847.
|
|
org.junit.experimental.categories.Category,\ | ||
org.junit.experimental.categories.CategoryValidator,\ | ||
org.junit.Ignore,\ | ||
org.junit.runner.RunWith,\ | ||
org.junit.runners.model.FrameworkField,\ | ||
org.junit.validator.AnnotationValidator,\ | ||
org.junit.vintage.engine.discovery.FilterableIgnoringRunnerDecorator |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
q,, Is this only adding the junit configs because junit contains the common config between all the deps?
It's been a while since my release, but I recall storage-nio required some custom configurations: https://github.com/googleapis/java-storage-nio/blob/97c3624ac7e375621d78c7eea10b00359d7971fd/google-cloud-nio/src/main/resources/META-INF/native-image/com/google/cloud/google-cloud-nio/native-image.properties#L7-L12
I think one missing from there is guava's com.google.common.collect.RegularImmutableList
, but not sure if adding that would mess with other downstream repos
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the catch. I was not aware that changes in storage-nio were necessary as well.
It should not break other repos since declaring the classes twice produces no problem.
I'm currently testing storage-nio after moving the configs to gax.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the relevant config and raise a PR to be merged once this one is released: googleapis/java-storage-nio#1559
Can you create an issue for this if we don't already have one? It sounds that we could potentially move to test configs instead. |
Good idea, thanks. I created #3707. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Follow up from #3674 Context: Some local changes were not pushed to that PR before it was merged. Main changes: - include `src/test/.../native-image.properties` in testlib jar - include java-storage-nio related classes for initialization at build time
Fixes #3638
Moves all added
src/test/native-image
configs done in HW repos. See:Further investigation: some JUnit classes already existed in the production native image folder and some other JUnit classes had to be added in those configs. We need to find out why JUnit configs need to be in production native configs.
Local tests
java-spanner
googleapis/java-spanner#3687
java-logging
googleapis/java-logging#1778
java-bigtable
googleapis/java-bigtable#2521
java-pubsub
googleapis/java-pubsub#2368
java-firestore
googleapis/java-firestore#2041
java-spanner-jdbc
googleapis/java-spanner-jdbc#1954
Fails locally (may need further investigation on local) both using the in-repo config and the config in this PR via gax, after initializing the image, meaning that the configuration works but the failures are due to a separate issue.
java-storage-nio
googleapis/java-storage-nio#1559