hub4j / github-api

Java API for GitHub
https://github-api.kohsuke.org/
MIT License
1.13k stars 723 forks source link

Enable github-api to support GraalVM native images #1908

Closed klopfdreh closed 1 week ago

klopfdreh commented 4 weeks ago

Describe the bug Due to some reflections you encounter errors during the runtime when github-api is used in a native image.

Example

at java.base@22.0.1/java.lang.invoke.LambdaForm$DMH/sa346b79c.invokeStaticInit(LambdaForm$DMH)\nCaused by: com.fasterxml.jackson.databind.exc.InvalidDefinitionException: Cannot construct instance of `org.kohsuke.github.GHRepository`: cannot deserialize from Object value (no delegate- or property-based Creator): this appears to be a native image, in which case you may need to configure reflection for the class that is to be deserialized\n at [Source: REDACTED (`StreamReadFeature.INCLUDE_SOURCE_IN_LOCATION` disabled); line: 1, column: 2]

To Reproduce Steps to reproduce the behavior:

  1. Build an application with Spring Boot Native and github-api
  2. Perform a native-image build
  3. Run the native application

Expected behavior github-api should be used in a native image without any issues

Desktop (please complete the following information): N/A

Additional context You could add META-INF/native-image/<groupid>/<artifactid>/reflect-config.json and describe the reflection usage:

Example:

[
  {
    "name": "org.kohsuke.github.GHRepository",
    <settings for reflections>
  }
]
klopfdreh commented 4 weeks ago

I saw a lot of other issues which are caused by the heavy usage of reflections. When I add all classes which are instantiated with reflections, like explained in the issue, I received the following error

java.lang.NullPointerException: The root GitHub reference for this instance is null. Probably caused by deserializing this class without using a GitHub instance. If you must do this, use the MappingObjectReader from GitHub.getMappingObjectReader().

Edit: I created a demo project for showcase: https://github.com/klopfdreh/github-api-native-test

Here you can run the GitHubApiNativeTestApplication in Java and everything is working fine.

When you perform a native image build (like explained in the README.md) the error above occurs.

All images are arm based - if you want to try it on x86_64 - you have to change the docker image and the liberica NIK which is downloaded - see Dockerfile.

bitwiseman commented 3 weeks ago

@klopfdreh I don't have a lot of time to commit to this project currently.

I've created a PR to create a v2.x That removes Java 8 and most of the reflection from this project. But it is already out of date and needs more attention to complete.

PRs welcome, but sounds like the changes you're asking if I would be breaking changes. So we might want to include the 2.x effort.

klopfdreh commented 3 weeks ago

Hey @bitwiseman - yes it seems to be better in 2.x then.

I think it all narrows down to the way objects are read and the root value is passed into the newly created objects with InjectableValues.

Sadly I just came across this issue as we are moving all of our projects to Spring Boot Native and one of it is using this GitHub-API.

I don’t know if I have the time to fix this as we have a lot of other projects being ported.

I just created a demo project which can be used to test native image.

When there will be a near final version of 2.x then let me know - I could test it for you.

gsmet commented 3 weeks ago

FWIW, we are heavily using this API in Quarkus projects and you might find some inspiration here: https://github.com/quarkiverse/quarkus-github-api/blob/main/deployment/src/main/java/io/quarkiverse/githubapi/deployment/GithubApiProcessor.java#L38-L52 and here: https://github.com/quarkiverse/quarkus-github-api/blob/main/deployment/src/main/java/io/quarkiverse/githubapi/deployment/GitHubApiDotNames.java (yeah sorry for all the constants, it was a bad idea, I need to drop them and just initialize the lists)

You could probably use this work to generate a native-image reflection config file.

In any case, given Jackson is heavily reflection-based (as all the general purpose JSON library out there), you will end up with some reflection. What could be a good idea for 2 is to have a marker annotation for JSON objects and use a small annotation processor to generate the native-image file.

klopfdreh commented 3 weeks ago

FWIW, we are heavily using this API in Quarkus projects and you might find some inspiration here: https://github.com/quarkiverse/quarkus-github-api/blob/main/deployment/src/main/java/io/quarkiverse/githubapi/deployment/GithubApiProcessor.java#L38-L52 and here: https://github.com/quarkiverse/quarkus-github-api/blob/main/deployment/src/main/java/io/quarkiverse/githubapi/deployment/GitHubApiDotNames.java (yeah sorry for all the constants, it was a bad idea, I need to drop them and just initialize the lists)

You could probably use this work to generate a native-image reflection config file.

In any case, given Jackson is heavily reflection-based (as all the general purpose JSON library out there), you will end up with some reflection. What could be a good idea for 2 is to have a marker annotation for JSON objects and use a small annotation processor to generate the native-image file.

Hey - thanks a lot for the heads up! Is there anyway to translate this into something which can be used only by GraalVM and does not rely ony Quarkus / JBoss API? If yes we could provide it here to enable the project for native images in a more generic way.

gsmet commented 3 weeks ago

Basically, you need to provide a reflection config file in JSON that lists all the reflection that is needed.

That's what Quarkus does from the classes I pointed out.

You could probably script something that generates most of the JSON file from what I pointed out. Again, with an annotation and an annotation processor, things would be a lot easier.

I adjust this code every time there is a release to make sure everything is listed (I'm a couple of micros late).

klopfdreh commented 3 weeks ago

Thank you for the explanation. I got it working with a RuntimeHintsRegistrar for Spring Boot Native right now. 👍

See last commit: https://github.com/klopfdreh/github-api-native-test/commit/8c50f6c60af7f2a745800cc7354aafa9cb496c4e

@bitwiseman what we could do to make this work for Spring Boot Native:

  1. Add a file META-INF/spring/aot-factories with the content
    org.springframework.aot.hint.RuntimeHintsRegistrar=org.kohsuke.github.aot.GitHubRuntimeHints
  1. Add the GitHubRuntimeHints to org.kohsuke.github.aot with the following content
    
    package org.kohsuke.github.aot;

import org.springframework.aot.hint.MemberCategory; import org.springframework.aot.hint.RuntimeHints; import org.springframework.aot.hint.RuntimeHintsRegistrar; import org.springframework.aot.hint.TypeReference;

import java.io.File; import java.io.FileInputStream; import java.io.IOException; import java.util.Arrays; import java.util.jar.JarEntry; import java.util.jar.JarInputStream; import java.util.logging.Level; import java.util.logging.Logger;

public class GitHubRuntimeHints implements RuntimeHintsRegistrar {

private static final Logger LOGGER = Logger.getLogger(GitHubRuntimeHints.class.getName());

@Override
public void registerHints(RuntimeHints hints, ClassLoader classLoader) {
    Arrays.stream(System.getProperty("java.class.path").split(File.pathSeparator)).forEach(classpathEntry -> {
        // If the classpathEntry is no jar skip it
        if (!classpathEntry.endsWith(".jar")) {
            return;
        }

        try (JarInputStream is = new JarInputStream(new FileInputStream(classpathEntry))) {
            JarEntry entry;
            while ((entry = is.getNextJarEntry()) != null) {
                if (entry.getName().endsWith(".class") && entry.getName().startsWith("org/kohsuke/github")) {
                    String githubApiClassName = entry.getName().replace("/", ".");
                    String githubApiClassNameWithoutClass = githubApiClassName.substring(0, githubApiClassName.length() - 6);
                    LOGGER.log(Level.INFO, "Registered class " + githubApiClassNameWithoutClass + " for reflections and serialization.");
                    hints.reflection().registerType(TypeReference.of(githubApiClassNameWithoutClass), MemberCategory.values());
                    hints.serialization().registerType(TypeReference.of(githubApiClassNameWithoutClass));
                }
            }
        } catch (IOException e) {
            LOGGER.log(Level.INFO, "Error while reading jars", e);
        }
    });
}

}



When ever GitHub-API is used as jar in a Spring Boot Native project it should be caught up automatically all serialization and reflection hints during the aot-processing. I think the spring dependency could be introduced as optional as this `RuntimeHintsRegistrar` is only executed when used within a Spring project - for logging I used `java.util.logging`.

See like it works in a Spring Project (In this case in Spring Batch): 

https://github.com/spring-projects/spring-batch/blob/main/spring-batch-core/src/main/java/org/springframework/batch/core/aot/CoreRuntimeHints.java

https://github.com/spring-projects/spring-batch/blob/main/spring-batch-core/src/main/resources/META-INF/spring/aot.factories

What I did to make this future proof is to iterate through all classes and get all in the `org/kohsuke/github` package. So everything in there is handled like in Java in which you can access all with reflections.
klopfdreh commented 3 weeks ago

@bitwiseman - if I should I create a PR let me know. 👍

gsmet commented 3 weeks ago

I think it would need to be a separate artifact. Having a dependency on Spring in such a low level library looks odd.

klopfdreh commented 3 weeks ago

I think it would need to be a separate artifact. Having a dependency on Spring in such a low level library looks odd.

So you suggest to add a new repository to hub4j called „github-api-spring-boot“ and if you want to build native applications you need to add both of them? (I let the native information out of the name as this dependency may also provide other Spring related integrations)

Would be fine for me, too. 👍

gsmet commented 3 weeks ago

It’s really for @bitwiseman to decide :)

I know that for Quarkus, we are dealing with this in extensions and we avoid cluttering the upstream projects.

I would have a similar approach here.

gsmet commented 3 weeks ago

Note that on the opposite side, if we go the low level GraalVM metadata route, it could make sense to include them upstream.

klopfdreh commented 3 weeks ago

It’s really for @bitwiseman to decide :)

I know that for Quarkus, we are dealing with this in extensions and we avoid cluttering the upstream projects.

I would have a similar approach here.

Yes that’s true, but it is very nice that you share your experiences here to find the right way to go. 😃

klopfdreh commented 3 weeks ago

Note that on the opposite side, if we go the low level GraalVM metadata route, it could make sense to include them upstream.

I think this depends on how often new classes which require reflections are introduced.

As there will be a 2.x soon it might be more future proof to introduce projects for Spring / Quarkus - but let’s see how @bitwiseman decides. 👍

bitwiseman commented 3 weeks ago

I currently have very little time to devote to this project. I would rather not fork to a separate repository - but creating a separate release branch that generates a separate artifact is relatively low cost.

See https://github.com/hub4j/github-api/tree/release/v1.x-unbridged

If there's a way to do this without adding a SpringBoot dependency that would be better.

klopfdreh commented 3 weeks ago

@bitwiseman in this case there is no need for a separate release branch. I created a PR which covers all reflection and serialization hints generated by the RuntimeHintsRegistrar I posted above.

I also double checked if there is no new class requires reflection hints in the main branch compared to the latest release.

bitwiseman commented 2 weeks ago

For reference/thought:

As noted by @gsmet, Quarkus goes the route of manually registering classes for reflection. https://quarkus.io/guides/writing-native-applications-tips#registering-for-reflection

The GraalVM docs discuss how to generate these files based on tracing: https://www.graalvm.org/latest/reference-manual/native-image/metadata/AutomaticMetadataCollection/

Since we enforce code coverage on our data classes, it might be possible to generate these files based on execution of all the tests.

But I think I've made reasonable suggestion for how to make #1914 a workable solution without either of the above.

klopfdreh commented 1 week ago

In the PR a suggestion has been implemented. It is without generate reachable meta data, but provides a way to add GraalVM without Spring / Quarkus classes during runtime.