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

feat: Add required reflection/resource metadata for native image generation #1612

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

utkuaydn
Copy link

I hope the title is self-explanatory. Providing reflection metadata to Graal's native image generation would be a nice addition to the library. The Akka counterparts of the artifacts already had these in place, so the contents were trivial.

@raboof
Copy link
Member

raboof commented Dec 18, 2024

The Akka counterparts of the artifacts already had these in place, so the contents were trivial.

Does that mean you derived these artifacts from the Akka artifacts? What license were those Akka artifacts under? If they were BSL-licensed we cannot accept them in Pekko.

I'm a little concerned how we're going to keep those maintained as the library evolves. I remember there was an initiative to maintain such configurations outside of the main Akka tree, https://github.com/vmencik/akka-graal-config . If we could adapt such an approach, the Graal definitions could evolve independently from Pekko 'itself'. Would that be a feasible approach?

@pjfanning
Copy link
Contributor

I guess the idea is that Graal users want these files included in our jars. And an outside group can still repackage the Pekko jars and add these files independently.

I haven't done a full review (currently using a mobile phone) but I didn't notice anything to continuously test these config s so that our CI could police changes.

And to reiterate @raboof 's point that we only accept PRs when the work is the submitter's original work. We will not accept code that is taken from Akka unless the submitter to Pekko is the original submitter to Akka. The code writer retains the copyright on the code and can choose to submit it Pekko.

@utkuaydn
Copy link
Author

utkuaydn commented Dec 18, 2024

About the content of the changes, they are simply JSON files that were generated via the native image agent running on the tests, that are in a schema that the native image executable accepts. Correct me if I'm wrong, but I am not sure if this could be considered copyright material or "original work" as it merely contains class paths, field types, etc. which reference the classes in the Pekko org. TBH, I did not double check if all the classes mentioned in the JSON files exist in the artifacts. But we can do due diligence if it is still a concern.

I didn't include the full workflow to keep these configurations updated because firstly, I wanted to gauge what everyone thinks about it, and secondly, it would be a lot of work and I thought it could be done in parts.

I would also be fine with having these in a separate project, but I think having them in Pekko itself can help with the release process. What do you think?

@pjfanning
Copy link
Contributor

Fair enough about adding regression tests in a 2nd pass.

Could you write up how to generate these JSON files? Ideally someone else would be able to generate them too. We may even need to regenerate if we make certain changes in Pekko. If we know how the generate them that removes any lingering worries that these files were copied directly from Akka.

@utkuaydn utkuaydn force-pushed the native-image-metadata branch from 3ad3545 to 448a3ed Compare December 18, 2024 15:06
@@ -0,0 +1,194 @@
[
Copy link
Contributor

Choose a reason for hiding this comment

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

Why would Pekko need this? Shouldn't this be in logback-classic jar?

If it doesn't matter what, these json files go in then, we could just have standalone jars that have these files instead of having the Pekko jars contain then.

Copy link
Contributor

Choose a reason for hiding this comment

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

logback-classic is an optional dependency - with slf4j, you should be able to add your own compliant logger jar

@@ -0,0 +1,234 @@
[
Copy link
Contributor

Choose a reason for hiding this comment

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

pekko-serialization-jackson is configurable and users can choose to add extra Jackson modules - what happens in that case (in terms of the native image)? Will those users have to create their own reflect-config.json.

@@ -0,0 +1 @@
Args = --enable-monitoring=jfr
Copy link
Contributor

Choose a reason for hiding this comment

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

is it a good idea to always enable JFR monitoring?

@He-Pin
Copy link
Member

He-Pin commented Dec 18, 2024

the file is generated with tools, but it's hard to cover every code paths, i think this better be a plug in for automatic generating instead of a static file

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.

4 participants