feat: GraalVM support

[klopfdreh]

Description

This PR will enable the github-api to be supported by GraalVM. There have been two options discussed.

1. Use Spring Boot RuntimeHintsRegistrar / Quarkus @buildstep to generate the reflect-config.json / serialization-config.json in separate artifacts.
2. Add the low level GraalVM metadata directly to the project.

@bitwiseman preferred option 2 as it reduces the dependencies and lower the costs.

The See #1908 for more information

Before submitting a PR:

• Changes must not break binary backwards compatibility. If you are unclear on how to make the change you think is needed while maintaining backward compatibility, CONTRIBUTING.md for details.
• Add JavaDocs and other comments explaining the behavior.
• When adding or updating methods that fetch entities, add @link JavaDoc entries to the relevant documentation on https://docs.github.com/en/rest .
• Add tests that cover any added or changed code. This generally requires capturing snapshot test data. See CONTRIBUTING.md for details.
• Run mvn -D enable-ci clean install site locally. If this command doesn't succeed, your change will not pass CI.
• Push your changes to a branch other than main. You will create your PR from that branch.

When creating a PR:

• Fill in the "Description" above with clear summary of the changes. This includes:
  • If this PR fixes one or more issues, include "Fixes #" lines for each issue.
  • Provide links to relevant documentation on https://docs.github.com/en/rest where possible. If not including links, explain why not.
• All lines of new code should be covered by tests as reported by code coverage. Any lines that are not covered must have PR comments explaining why they cannot be covered. For example, "Reaching this particular exception is hard and is not a particular common scenario."
• Enable "Allow edits from maintainers".

[klopfdreh]

What I did:

1. I ran my application with the execution aot-process of spring-boot-maven-plugin with 1.323 of github-api as dependency.
2. Because of the RuntimeHintsRegistrar mentioned here Enable github-api to support GraalVM native images #1908 (comment) all reflection-config and serialization-config hints are created at ./target/spring-aot/main/resources/META-INF/groupid/artifactid/*.json
3. I copied all information which belongs to org.kohsuke.github packages into those configs provided by this PR.

Hey @gsmet - it would be very nice if you could check if I missed something in the configs. 😃

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.36%. Comparing base (338de9a) to head (dd53d7c).
Report is 4 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##               main    #1914   +/-   ##
=========================================
  Coverage     81.36%   81.36%           
  Complexity     2464     2464           
=========================================
  Files           239      239           
  Lines          7471     7471           
  Branches        401      401           
=========================================
  Hits           6079     6079           
  Misses         1146     1146           
  Partials        246      246           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[klopfdreh]

Important: If there are any new classes that requires reflections those need to be added to the configs as well as they are not auto generated via Spring / Quarkus

[klopfdreh]

I had to force push as I forgot the native-image folder inside of META-INF

[bitwiseman]

@klopfdreh
Thanks for giving this a try. This PR avoids the dependency, but it introduces a manual step for any time a class is added or removed. I'm not opposed this outright, but if you're going to go this route, there needs to be a test that checks that all expected classes are present in the file.

[bitwiseman]

@klopfdreh
So, we need test to ensure the file stay up-to-date.

Also, it looks like you're registering all the classes in org.kohsuke.github for reflection, which is not accurate. Many classes in there are not reflected on. I'm guessing that including them has consequences for performance?

The difficulty here is that we don't have a way of indicating what classes should and shouldn't be in the file.

Added some notes to #1908 just for reference...

Here's my thought on how to proceed:

• Add a file called graalvm-no-reflect to the test/resources.
• List classes in there that are under org.kohsuke.github do not use reflection (start with a few just as proof of concept)
• Add a test based on your aot-process
  ** You could even use the spring-boot dependency if you like (as a test dependency). But I'm not sure you need it.
  ** The test would load the current reflect-config.json and the graalvm-no-reflect and then check the contents of reflect-config.json is correct.
  ** If the contents are not correct, the test will fail with meanful instructions saying, "Add class z.x.y to one the two files: graalvm-no-reflect or reflect-config.json."
• Same for serialize-config as there are plenty of classes that don't need special serialization behavior either. Are they the same ones? Not sure.

This will ensure the files are maintained but also have low cost to maintain them.

How does this sound to you?

[klopfdreh]

For me this sounds a bit odd that you want to handle both classes with/without reflections, as you already know from the reflect-config.json which classes use them, meaning all others don't use reflection.

You can't get a list with classes that require reflection as this is something you need to know and define via spring-aot.

I agree that it is much nicer to find out if you missed a class in the reflect-config.json during build time, but the only way to see if it is working is to start a native image in your pipeline (which is expensive).

Note: It is not that big of a deal to add all classes to reflect and serialization as it is like in the JVM. The performance impact is also very low (I tested it already).

What I suggest would be to add a test that uses spring-aot and checks via Registrar if all classes are mentioned in the reflect-config.json.

To use reachable meta data for generating the reflect-config.json is risky as you have to have a 100% test coverage all the time.

[gsmet]

In the Quarkus extension, we have a test that approximates things a bit, meaning we have a list of excluded classes and any time a class gets added, we have to decide if we exclude it or add it: https://github.com/quarkiverse/quarkus-github-api/blob/main/deployment/src/test/java/io/quarkiverse/githubapi/deployment/GitHubApiDotNamesTest.java#L28 .

It has worked well enough for us even if not ideal.

[gsmet]

BTW, I still think that having an annotation and an annotation processor would make things a bit easier.

People wouldn't have to adjust the JSON file, which can be error prone, they would just have to add an annotation.

[bitwiseman]

@klopfdreh

What I suggest would be to add a test that uses spring-aot and checks via Registrar if all classes are mentioned in the reflect-config.json

I agree to this suggestion. It is a bit on the bare minimum side, but I would accept this.

Can the test also update the json file (and then report failure)? That would address @gsmet's concern about maintaining the json file.

In the Quarkus extension, we have a test that approximates things a bit, meaning we have a list of excluded classes and any time a class gets added, we have to decide if we exclude it or add it: https://github.com/quarkiverse/quarkus-github-api/blob/main/deployment/src/test/java/io/quarkiverse/githubapi/deployment/GitHubApiDotNamesTest.java#L28 .

This is basically lines up with what I was thinking around not registering classes for reflection which don't need it.

What this test shows is that having an exclude list is not expensive and it lets us check "all classes except a specific list are mentioned in the reflect-config.json". If you're willing to do this @klopfdreh, that would be even better but not absolute required.

[klopfdreh]

@bitwiseman - I am going to add the test, tomorrow.

Even if this is a manual effort to add the entry into the reflect-config.json I would not let a test change the source code. (It is still only a test)

[bitwiseman]

Then If possible make the output when the test fails clear instructions about what to change.

[klopfdreh]

Hey @bitwiseman - I added the test now.

Things which are important:

1. The test build requires java 17 for Spring Boot 3.x so I removed java 11 and added java 21 in matrix build
2. ensure-java-1.8-class-library can't be used for animal-sniffer-maven-plugin anymore as Spring Boot uses Java 17
3. The test has to be run with mvn because the process-test-aot is used of spring-boot-maven-plugin to generate all stuff into ./target/spring-aot/test/....
4. I had to add junit-vintage-engine so that junit 4 and junit 5 tests are executed through maven-surefire-plugin
5. I was not able to get the test Java 8 action back to work as the logback dependency is required with a higher version from Spring Boot right now. So I shifted some tests require logback to slow-or-flaky-tests.txt
6. I had to change the codeql-analysis-yml as it was running with java 8 and requires java 17, now.
7. Github does not refresh the status report for matrix builds so there is still java 11 mentioned at the end of this PR.

I have Java > 17 installed locally and already tested that if you run mvn clean verify and not add a class to reflect-config.json / serialization-config.json or no-reflect-and-serialization-list the following error occurs

The class "org.kohsuke.github.AbstractBuilder" needs to be configured or excluded for reflection / serialization and was not mentioned in one of the following resources:

src/main/resources/META-INF/reflect-config.json - example:

  {
    "name": "org.kohsuke.github.AbstractBuilder",
    "allPublicFields": true,
    "allDeclaredFields": true,
    "queryAllPublicConstructors": true,
    "queryAllDeclaredConstructors": true,
    "allPublicConstructors": true,
    "allDeclaredConstructors": true,
    "queryAllPublicMethods": true,
    "queryAllDeclaredMethods": true,
    "allPublicMethods": true,
    "allDeclaredMethods": true,
    "allPublicClasses": true,
    "allDeclaredClasses": true
  }

src/main/resources/META-INF/serialization.json - example:

  {
    "name": "org.kohsuke.github.AbstractBuilder"
  }

src/test/resources/no-reflect-and-serialization-list - example:

  org.kohsuke.github.AbstractBuilder

Please add it to either no-reflect-and-serialization-list or to serialization.json and / or reflect-config.json

[bitwiseman]

I'm still not sure this list is correct, but issues here can be addressed as bug fixes.

[bitwiseman]

@klopfdreh
Great work!

@gsmet
This looks good to me. Please review and approve if you agree.

[bitwiseman]

I'm going to merge this. @gsmet if you have any changes, please file an issue and tag @klopfdreh .