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

Add SVM metadata to netty modules #8963

Merged
merged 1 commit into from Apr 29, 2019

Conversation

@pmlopes
Copy link
Contributor

commented Mar 21, 2019

Motivation:

GraalVM native images are a new way to deliver java applications. Netty is one of the most popular libraries however there are a few limitations that make it impossible to use with native images out of the box. Adding a few metadata (in specific modules will allow the compilation to success and produce working binaries)

Modification:

Added properties files in META-INF and substitutions classes (under internal.svm) will solve the compilation issues. The substitutions classes are not visible and do not have a public constructor so they are not visible to end users.

Result:

Fixes #8959

This fix is very conservative as it applies the minimum config required to build:

  • pure netty servers
  • vert.x applications
  • grpc applications

The build is having trouble due to checkstyle which does not seem to be able to find the copyright notice on property files.

@netty-bot

This comment has been minimized.

Copy link

commented Mar 21, 2019

Can one of the admins verify this patch?

@normanmaurer

This comment has been minimized.

Copy link
Member

commented Mar 22, 2019

@netty-bot test this please

@normanmaurer

This comment has been minimized.

Copy link
Member

commented Mar 22, 2019

@pmlopes I need to adjust the Checkstyle rule... will let you know once done and once you can rebase. I the meantime is there any way to test that this works as expected ? Like make it part of the build to ensure we not break it again ?

@pmlopes

This comment has been minimized.

Copy link
Contributor Author

commented Mar 22, 2019

I could add a new module like testsuite-native-image what builds a few examples but it would require a graalvm (community edition is fine) to be installed in the CI environment. Basically this can only be tested as a "integration test" as most of the changes are json/property files and the code that was added is just annotated classes without behaviour making unit testing really not possible.

@normanmaurer

This comment has been minimized.

Copy link
Member

commented Mar 22, 2019

@pmlopes I think that would be fine... Maybe just skip the test suite by default and then we can add a CI job which has everything setup to run these. WDYT ?

@pmlopes

This comment has been minimized.

Copy link
Contributor Author

commented Mar 22, 2019

Sure, I'll work on it.

normanmaurer added a commit that referenced this pull request Mar 22, 2019

Update to new netty-build version to be able to correctly detect copy…
…right header in property files.

Motivation:

#8963 adds property files which contains a netty copyright header but our old checkstyle regex did not correct detect these.

Modifications:

Update to new netty-build which contains an updated regex.

Result:

Be able to correctly detect copyright headers in property files.

normanmaurer added a commit that referenced this pull request Mar 22, 2019

Update to new netty-build version to be able to correctly detect copy…
…right header in property files. (#8967)

Motivation:

#8963 adds property files which contains a netty copyright header but our old checkstyle regex did not correct detect these.

Modifications:

Update to new netty-build which contains an updated regex.

Result:

Be able to correctly detect copyright headers in property files.
@normanmaurer

This comment has been minimized.

Copy link
Member

commented Mar 22, 2019

@pmlopes please rebase... We should now be able to correctly detect the copyright headers in the property files: #8967

@normanmaurer normanmaurer added this to the 4.1.35.Final milestone Mar 22, 2019

@pmlopes pmlopes force-pushed the pmlopes:svm-metadata branch from 8ce1ade to 5fa6514 Mar 22, 2019

@normanmaurer

This comment has been minimized.

Copy link
Member

commented Mar 22, 2019

@netty-bot test this please

normanmaurer added a commit that referenced this pull request Mar 22, 2019

Update to new netty-build version to be able to correctly detect copy…
…right header in property files. (#8967)

Motivation:

#8963 adds property files which contains a netty copyright header but our old checkstyle regex did not correct detect these.

Modifications:

Update to new netty-build which contains an updated regex.

Result:

Be able to correctly detect copyright headers in property files.
@normanmaurer

This comment has been minimized.

Copy link
Member

commented Mar 22, 2019

@pmlopes let me know once you have the testing stuff added and I will see how to do the CI bit

@pmlopes

This comment has been minimized.

Copy link
Contributor Author

commented Mar 25, 2019

@normanmaurer I've created the simplest white smoke test (which helped detecting a bug in the PR). The test suite is a hello world http server, that will fail to compile if the meta data is missing (the property files) or will fail to run (segfault) if the substitution classes are missing. Currently I added it as a sub module to the top top but I think that you need to help me fix as this should only run on a linux/mac with graalvm installed (not for everyone).

@pmlopes

This comment has been minimized.

Copy link
Contributor Author

commented Mar 25, 2019

@olpaw could you have a look now that all the basic pieces are in place?

@normanmaurer

This comment has been minimized.

Copy link
Member

commented Mar 25, 2019

@pmlopes ok cool... I think what I would do is basically add an extra property which allows us to skip the test and only set it to true if we run it on the correct CI.

Something like:

https://github.com/netty/netty/blob/netty-4.1.34.Final/testsuite-autobahn/pom.xml#L67
https://github.com/netty/netty/blob/netty-4.1.34.Final/testsuite-autobahn/pom.xml#L85

@pmlopes

This comment has been minimized.

Copy link
Contributor Author

commented Mar 25, 2019

This PR is now on hold until graal rc15 is out where the plugin will support skip configuration.

Depends: oracle/graal#1113

Show resolved Hide resolved testsuite-native-image/pom.xml Outdated
Show resolved Hide resolved common/pom.xml Outdated
@normanmaurer

This comment has been minimized.

Copy link
Member

commented Mar 28, 2019

@pmlopes just to be prepared can you let know what exactly should be installed to run the tests ?

Show resolved Hide resolved pom.xml Outdated
@pmlopes

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2019

@trustin I've fixed all the indentation issues and white space. There are just 2 open questions, if I remove the copyright header on the properties files the checkstyle validation will fail. The copyright header in the testsuite is there just to keep it consistent with the remaning testsuites.

Please advise on how to proceed.

<skip>${skipNativeImageTestsuite}</skip>
<imageName>${project.artifactId}</imageName>
<mainClass>io.netty.testsuite.svm.HttpNativeServer</mainClass>
<buildArgs>--report-unsupported-elements-at-runtime --allow-incomplete-classpath</buildArgs>

This comment has been minimized.

Copy link
@olpaw

olpaw Apr 15, 2019

Since RC15 you can have multiline buildArgs

<buildArgs>
  --report-unsupported-elements-at-runtime
  --allow-incomplete-classpath
</buildArgs>

(if you like)

This comment has been minimized.

Copy link
@pmlopes

pmlopes Apr 15, 2019

Author Contributor

👍 I think that for this example it should be ok, but for longer it is a nice addition!

Channel channel = b.bind(0).sync().channel();
System.err.println("Server started, will shutdown now.");
channel.close().sync();
System.exit(0);

This comment has been minimized.

Copy link
@carl-mastrangelo

carl-mastrangelo Apr 16, 2019

Member

System.exit skips finally IIRC.

This comment has been minimized.

Copy link
@pmlopes

pmlopes Apr 16, 2019

Author Contributor

@carl-mastrangelo I've updated the test to flag the sucess of the start and shutdown of the server and from the finally block to return the right status code.

@pmlopes pmlopes force-pushed the pmlopes:svm-metadata branch from ab797e1 to b11d3f8 Apr 16, 2019

@johnou

johnou approved these changes Apr 16, 2019

Copy link
Contributor

left a comment

One last nit and lgtm.

@trustin
Copy link
Member

left a comment

Thanks, @pmlopes.

@pmlopes

This comment has been minimized.

Copy link
Contributor Author

commented Apr 17, 2019

@johnou I've moved the System.exit outside the finally block as suggested!

@johnou

johnou approved these changes Apr 17, 2019

@johnou

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2019

@pmlopes great work!

@normanmaurer

This comment has been minimized.

Copy link
Member

commented Apr 17, 2019

@pmlopes please sign our ICLA and let me know once done + squash everything.

After this is done I can merge. Great work!

@pmlopes pmlopes force-pushed the pmlopes:svm-metadata branch from 9f4c08c to 8f1d770 Apr 17, 2019

@pmlopes

This comment has been minimized.

Copy link
Contributor Author

commented Apr 17, 2019

@normanmaurer done, squashed and signed the ICLA.

@normanmaurer

This comment has been minimized.

Copy link
Member

commented Apr 18, 2019

@pmlopes perfect... one last thing (sorry).

Can you just apply this diff to your patch and squash again:

diff --git a/pom.xml b/pom.xml
index 51b7f16dca..05f7e59c47 100644
--- a/pom.xml
+++ b/pom.xml
@@ -68,6 +68,19 @@
   </developers>

   <profiles>
+    <!-- Detect if we use GraalVM and if so enable the native image testsuite -->
+    <profile>
+      <id>graal</id>
+      <activation>
+        <file>
+          <!-- GraalVM Component Updater should exists when using GraalVM-->
+          <exists>${java.home}/bin/gu</exists>
+        </file>
+      </activation>
+      <properties>
+        <skipNativeImageTestsuite>false</skipNativeImageTestsuite>
+      </properties>
+    </profile>
     <!-- JDK13 -->
     <profile>
       <id>java13</id>
diff --git a/testsuite-native-image/pom.xml b/testsuite-native-image/pom.xml
index 5df8844c32..439f3b0209 100644
--- a/testsuite-native-image/pom.xml
+++ b/testsuite-native-image/pom.xml
@@ -21,7 +21,7 @@
   <parent>
     <groupId>io.netty</groupId>
     <artifactId>netty-parent</artifactId>
-    <version>4.1.35.Final-SNAPSHOT</version>
+    <version>4.1.36.Final-SNAPSHOT</version>
   </parent>

   <artifactId>netty-testsuite-native-image</artifactId>

This way the native testsuite will be run automatically if the CI uses graalvm to build :) Also I adjusted the version number as we released 4.1.35.Final in the meantime.

@olpaw

This comment has been minimized.

Copy link

commented Apr 18, 2019

@normanmaurer wouldn't it make more sense to check for the existence of ${java.home}/bin/native-image to detect a GraalVM (especially in this context) ?

@normanmaurer

This comment has been minimized.

Copy link
Member

commented Apr 18, 2019

@pmlopes pmlopes force-pushed the pmlopes:svm-metadata branch from 8f1d770 to 7522e7a Apr 18, 2019

@pmlopes

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2019

@normanmaurer @olpaw patch is applied and using native-image as check

@normanmaurer

This comment has been minimized.

Copy link
Member

commented Apr 18, 2019

@netty-bot test this please

@normanmaurer normanmaurer merged commit f1495e1 into netty:4.1 Apr 29, 2019

2 of 3 checks passed

pull request validation (centos6-java12) Build finished.
Details
pull request validation (centos6-java11) Build finished.
Details
pull request validation (centos6-java8) Build finished.
Details
@normanmaurer

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

@pmlopes thanks a lot again!

normanmaurer added a commit that referenced this pull request Apr 29, 2019

Add SVM metadata and minimal substitutions to build graalvm native im…
…age applications. (#8963)

Motivation:

GraalVM native images are a new way to deliver java applications. Netty is one of the most popular libraries however there are a few limitations that make it impossible to use with native images out of the box. Adding a few metadata (in specific modules will allow the compilation to success and produce working binaries)

Modification:

Added properties files in `META-INF` and substitutions classes (under `internal.svm`) will solve the compilation issues. The substitutions classes are not visible and do not have a public constructor so they are not visible to end users.

Result:

Fixes #8959

This fix is very conservative as it applies the minimum config required to build:

* pure netty servers
* vert.x applications
* grpc applications

The build is having trouble due to checkstyle which does not seem to be able to find the copyright notice on property files.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.