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

fix: add compatibility for GraalVM 22.2.0 #1025

Merged
merged 19 commits into from
Nov 17, 2022
Merged

Conversation

shubha-rajan
Copy link
Contributor

@shubha-rajan shubha-rajan commented Oct 28, 2022

Change Description

  • Remove references to svm dependency
  • Upgrade Native Image Maven plugin
  • Change scope of drivers to compile for native image builds

Fixes #998 , #1027

@conventional-commit-lint-gcf
Copy link

🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use automerge label. Good luck human!

-- conventional-commit-lint bot
https://conventionalcommits.org/

@shubha-rajan shubha-rajan force-pushed the fix-graalvm-build branch 7 times, most recently from 384ebab to 053cac8 Compare November 1, 2022 19:41
@shubha-rajan shubha-rajan changed the title fix: add exports for native image compilation fix: add compatibility for GraalVM 22.3.0 Nov 1, 2022
@shubha-rajan shubha-rajan marked this pull request as ready for review November 1, 2022 20:11
@shubha-rajan shubha-rajan requested review from a team and suztomo November 1, 2022 20:11
@@ -164,6 +164,16 @@
</dependency>
</dependencies>
</profile>
<profile>
<id>native</id>
Copy link
Member

Choose a reason for hiding this comment

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

Should these deps be marked as "provided" so the user can set them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Marking them as provided was what was causing one of the errors, so here we're overriding the provided scope with a compile scope. This was the error:

Failures (2):
  JUnit Vintage:JdbcPostgresIamAuthIntegrationTests:pooledConnectionTest
    MethodSource [className = 'com.google.cloud.sql.postgres.JdbcPostgresIamAuthIntegrationTests', methodName = 'pooledConnectionTest', methodParameterTypes = '']
    => org.opentest4j.MultipleFailuresError: Multiple Failures (2 failures)
	java.lang.RuntimeException: Failed to get driver instance for jdbcUrl=jdbc:postgresql:///proxy_testing
	java.lang.NullPointerException: <no message>
       org.junit.vintage.engine.execution.TestRun.getStoredResultOrSuccessful(TestRun.java:200)
       org.junit.vintage.engine.execution.RunListenerAdapter.fireExecutionFinished(RunListenerAdapter.java:248)
       org.junit.vintage.engine.execution.RunListenerAdapter.testFinished(RunListenerAdapter.java:214)
       org.junit.vintage.engine.execution.RunListenerAdapter.testFinished(RunListenerAdapter.java:88)
       org.junit.runner.notification.SynchronizedRunListener.testFinished(SynchronizedRunListener.java:87)
       [...]
       Suppressed: java.lang.RuntimeException: Failed to get driver instance for jdbcUrl=jdbc:postgresql:///proxy_testing
         com.zaxxer.hikari.util.DriverDataSource.<init>(DriverDataSource.java:114)
         com.zaxxer.hikari.pool.PoolBase.initializeDataSource(PoolBase.java:331)
         com.zaxxer.hikari.pool.PoolBase.<init>(PoolBase.java:114)
         com.zaxxer.hikari.pool.HikariPool.<init>(HikariPool.java:108)
         com.zaxxer.hikari.HikariDataSource.<init>(HikariDataSource.java:81)
         [...]
       Caused by: java.sql.SQLException: No suitable driver
         java.sql.DriverManager.getDriver(DriverManager.java:298)
         com.zaxxer.hikari.util.DriverDataSource.<init>(DriverDataSource.java:106)
         [...]
       Suppressed: java.lang.NullPointerException
         com.google.cloud.sql.postgres.JdbcPostgresIamAuthIntegrationTests.dropTableIfPresent(JdbcPostgresIamAuthIntegrationTests.java:111)
         java.lang.reflect.Method.invoke(Method.java:566)
         org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:59)
         org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
         org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:56)
         [...]
  JUnit Vintage:JdbcPostgresIntegrationTests:pooledConnectionTest
    MethodSource [className = 'com.google.cloud.sql.postgres.JdbcPostgresIntegrationTests', methodName = 'pooledConnectionTest', methodParameterTypes = '']
    => org.opentest4j.MultipleFailuresError: Multiple Failures (2 failures)
	java.lang.RuntimeException: Failed to get driver instance for jdbcUrl=jdbc:postgresql:///proxy_testing
	java.lang.NullPointerException: <no message>
       org.junit.vintage.engine.execution.TestRun.getStoredResultOrSuccessful(TestRun.java:200)
       org.junit.vintage.engine.execution.RunListenerAdapter.fireExecutionFinished(RunListenerAdapter.java:248)
       org.junit.vintage.engine.execution.RunListenerAdapter.testFinished(RunListenerAdapter.java:214)
       org.junit.vintage.engine.execution.RunListenerAdapter.testFinished(RunListenerAdapter.java:88)
       org.junit.runner.notification.SynchronizedRunListener.testFinished(SynchronizedRunListener.java:87)
       [...]
       Suppressed: java.lang.RuntimeException: Failed to get driver instance for jdbcUrl=jdbc:postgresql:///proxy_testing
         com.zaxxer.hikari.util.DriverDataSource.<init>(DriverDataSource.java:114)
         com.zaxxer.hikari.pool.PoolBase.initializeDataSource(PoolBase.java:331)
         com.zaxxer.hikari.pool.PoolBase.<init>(PoolBase.java:114)
         com.zaxxer.hikari.pool.HikariPool.<init>(HikariPool.java:108)
         com.zaxxer.hikari.HikariDataSource.<init>(HikariDataSource.java:81)
         [...]
       Caused by: java.sql.SQLException: No suitable driver
         java.sql.DriverManager.getDriver(DriverManager.java:298)
         com.zaxxer.hikari.util.DriverDataSource.<init>(DriverDataSource.java:106)
         [...]
       Suppressed: java.lang.NullPointerException
         com.google.cloud.sql.postgres.JdbcPostgresIntegrationTests.dropTableIfPresent(JdbcPostgresIntegrationTests.java:104)
         java.lang.reflect.Method.invoke(Method.java:566)
         org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:59)
         org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
         org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:56)
         [...]

@enocom enocom self-requested a review November 2, 2022 21:11
Copy link
Member

@enocom enocom left a comment

Choose a reason for hiding this comment

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

LGTM.

@shubha-rajan shubha-rajan changed the title fix: add compatibility for GraalVM 22.3.0 fix: add compatibility for GraalVM 22.2.0 Nov 8, 2022
Copy link
Contributor

@mpeddada1 mpeddada1 left a comment

Choose a reason for hiding this comment

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

@shubha-rajan the use of the svm dependency has been debated for some time and there were some discussions to stop releasing it. For stability in the future releases, it might be better to remove references to it. We have a couple of references here:

import com.oracle.svm.core.annotate.AutomaticFeature;
import com.oracle.svm.core.configure.ResourcesRegistry;
.

  1. Here is an example of how AutomaticFeature was replaced in gax: googleapis/gax-java@2229bfe
  2. The use of the ResourceRegistration class can be replaced with using resource-config.json. For example: https://github.com/googleapis/gax-java/pull/1674/files

@shubha-rajan
Copy link
Contributor Author

shubha-rajan commented Nov 16, 2022

@shubha-rajan the use of the svm dependency has been debated for some time and there were some discussions to stop releasing it.

I followed the example to remove the svm references here. PTAL

Comment on lines +170 to +167
<dependency>
<groupId>mysql</groupId>
<artifactId>mysql-connector-java</artifactId>
<version>8.0.17</version>
</dependency>
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah quick question, what error do we run into if we don't include this dependency? Is it the one you mentioned above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No it's a different one. "No suitable driver found." I started seeing it after upgrading the maven plugin.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

       Suppressed: java.lang.RuntimeException: Failed to get driver instance for jdbcUrl=jdbc:postgresql:///proxy_testing
         com.zaxxer.hikari.util.DriverDataSource.<init>(DriverDataSource.java:114)
         com.zaxxer.hikari.pool.PoolBase.initializeDataSource(PoolBase.java:331)
         com.zaxxer.hikari.pool.PoolBase.<init>(PoolBase.java:114)
         com.zaxxer.hikari.pool.HikariPool.<init>(HikariPool.java:108)
         com.zaxxer.hikari.HikariDataSource.<init>(HikariDataSource.java:81)
         [...]
       Caused by: java.sql.SQLException: No suitable driver
         java.sql.DriverManager.getDriver(DriverManager.java:298)
         com.zaxxer.hikari.util.DriverDataSource.<init>(DriverDataSource.java:106)
         [...]
       Suppressed: java.lang.NullPointerException
         com.google.cloud.sql.postgres.JdbcPostgresIamAuthIntegrationTests.dropTableIfPresent(JdbcPostgresIamAuthIntegrationTests.java:111)
         java.lang.reflect.Method.invoke(Method.java:566)
         org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:59)
         org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
         org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:56)
         [...]

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting, thanks for the stacktrace! I wonder if this dependency is not being included in the classpath generated by the native maven plugin. Optional, but it might also be worth checking if this is a regression introduced by the plugin. There was an issue when we upgraded to 0.9.12 where test-jar dependencies weren't getting recognized: https://github.com/mpeddada1/0.9.12-test-jar so this could be similar.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the issues are related. I'll put together a quick repro so we can track this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

unfortunately we still have to upgrade because we get UnsupportedFeatureExceptions otherwise

@@ -0,0 +1,11 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: how about we name this directoryMETA-INF/native-image/com.google.cloud.sql/jdbc-socket-factory-parent/native-image.properties? For more info: https://www.graalvm.org/22.1/reference-manual/native-image/BuildConfiguration/#embedding-a-configuration-file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

pom.xml Outdated
@@ -72,9 +72,8 @@
<properties>
<project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
<javac.version>9+181-r4173-1</javac.version>
<native-image.version>0.9.11</native-image.version>
<native-image.version>0.9.16</native-image.version>
<graal-sdk.version>21.3.4</graal-sdk.version>
Copy link
Contributor

Choose a reason for hiding this comment

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

This might also need to be updated to 22.2.0.

Comment on lines +170 to +167
<dependency>
<groupId>mysql</groupId>
<artifactId>mysql-connector-java</artifactId>
<version>8.0.17</version>
</dependency>
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting, thanks for the stacktrace! I wonder if this dependency is not being included in the classpath generated by the native maven plugin. Optional, but it might also be worth checking if this is a regression introduced by the plugin. There was an issue when we upgraded to 0.9.12 where test-jar dependencies weren't getting recognized: https://github.com/mpeddada1/0.9.12-test-jar so this could be similar.

Copy link
Contributor

@mpeddada1 mpeddada1 left a comment

Choose a reason for hiding this comment

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

LGTM

@shubha-rajan shubha-rajan merged commit bd153cd into main Nov 17, 2022
@shubha-rajan shubha-rajan deleted the fix-graalvm-build branch November 17, 2022 20:50
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.

IllegalAccessError when trying to build Java 17 Native image
3 participants