-
Notifications
You must be signed in to change notification settings - Fork 187
Support Native Image for Edge #2216
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
Conversation
HeikoKlare
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the contribution! This looks like a proper refactoring to me and if that helps to use Edge with GraalVM, that's of course fine.
@tnikolai2 can please properly format the lines of code that you have touched? In particular, the added lines use whitespaces for indentation while the surrounding code uses tabs. And it would be great if you could use a more expressive headline in your commit than "Update Edge.java".
You also refer to the necessity to add a jni-config.json. Is that something that needs to be done locally in an application executed on GraalVM? In that case, it might make sense that we add this specific knowledge about usage with GraalVM to the FAQ: https://github.com/eclipse-platform/eclipse.platform/blob/master/docs/FAQ/FAQ_How_do_I_use_Edge-IE_as_the_Browser's_underlying_renderer.md
|
I fix whitespaces. If complile with GraalVM with To fix These metadata files can be auto-generated by running the application with the native image agent: However, due to limitations in GraalVM, inline lambdas cannot be referenced in reachability metadata. But compiling with GraalVM and creating reachability metadata it is a big separate task. |
|
Here is an example for Windows: target/SequrFly-1.0-SNAPSHOT-jar-with-dependencies.jar - full jar with updated Edge.java or can generate with Set env variables Use GraalVM for windows 64, java 24 Select the Desktop development with C++ checkbox in the main installation window. On the right side under Installation Details, make sure that the two requirements, Windows 11 SDK and MSVC (…) C++ x64/x86 build tools, are selected. Gen
build native image
run sequrfly.exe |
|
If use original org.eclipse.swt.win32.win32.x86_64 3.129.0 need also add native-config\reflect-config.json But anyway will appear error: This cannot be fixed without changing Edge.java. |
HeikoKlare
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the explanations!
From my side, this looks fine now.
@sratz do you have any comments here?
sratz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me as well.
Would it make sense to add a small javadoc comment to the classes reminding that they should
- not be converted to lambdas
- ideally not be renamed to maximize compatibility
?
Contributes to supporting native images (GraalVM) for Edge.
That definitely makes sense. I have just added comments to the two classes (also including references to this PR) and updated the PR accordingly. |
Lambda expressions are replaced by classes because GraalVM Native Image does not support the use of lambda expressions in jni callback functions.
Also need add to jni-config.json: