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

Added GraalVM native support to Apache FreeMarker #121

Open
wants to merge 3 commits into
base: 2.3-gae
Choose a base branch
from

Conversation

fugerit79
Copy link

@fugerit79 fugerit79 commented Jan 17, 2025

Native support

In this pull request :

GraalVM native support

  1. Added native-image.properties to initialize at runtime relevant classes
  2. Added resource-config.json to track resources to include in the native executable
  3. Added boolean IS_GRAALVM_NATIVE = System.getProperty( "org.graalvm.nativeimage.imagecode" ) != null in logger to avoid runtime initialization error
  4. Added dependency to GraalVM SDK

Test module

Added a test module to build as a GraalVM native image.

  1. Build the main Apache FreeMarker proejct :
./gradlew build -Pfreemarker.allowUnsignedReleaseBuild=true
  1. Build the test module native image with GraalVM :
./gradlew :freemarker-test-graalvm-native:nativeCompile 
  1. Run the project :
./freemarker-test-graalvm-native/build/native/nativeCompile/freemarker-test-graalvm-native 

CI

Updated CI to test the GraalVM native support by building and running the test module as a native executable.

@fugerit79 fugerit79 force-pushed the 1-add-graalvm-support-to-apache-freemarker branch 4 times, most recently from 54a345f to 6fc56e1 Compare January 17, 2025 23:31
@fugerit79
Copy link
Author

fugerit79 commented Jan 17, 2025

@ddekany

I misunderstood at the beginning.

I’ve pushed some modifications, and hopefully, it should be fine now.

I’ve also done some additional testing and made the following changes:

  • Improved the native test by adding relevant reflection configurations.
  • Added some comments.
  • Fixed a couple of typos.
  • Substitution (Log4jOverSLF4JTesterSubstitute) is no more needed after graalvm native check in logger

Additionally I tested the pull request on my fork : 

https://github.com/fugerit-org/freemarker/actions/workflows/ci.yml

I really hope now everything is ok.

See you

@fugerit79 fugerit79 force-pushed the 1-add-graalvm-support-to-apache-freemarker branch from 6fc56e1 to 0afb662 Compare January 18, 2025 01:21
@@ -428,7 +433,8 @@ private static LoggerFactory createLoggerFactory(int libraryEnum) throws ClassNo
if (libraryEnum == LIBRARY_AUTO) {
for (int libraryEnumToTry = MAX_LIBRARY_ENUM; libraryEnumToTry >= MIN_LIBRARY_ENUM; libraryEnumToTry--) {
if (!isAutoDetected(libraryEnumToTry)) continue;
if (libraryEnumToTry == LIBRARY_LOG4J && hasLog4LibraryThatDelegatesToWorkingSLF4J()) {
// skip hasLog4LibraryThatDelegatesToWorkingSLF4J when running in GraalVM native image
if (!IS_GRAALVM_NATIVE && libraryEnumToTry == LIBRARY_LOG4J && hasLog4LibraryThatDelegatesToWorkingSLF4J()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

But that still prefers Log4J API instead of SLF4J API when we have log4j-over-slf4j, under GrallVM Native only, and it should prefer SLF4J in that case. I know, this Logger class is a mess. Anyway, I looked into more, and the easiest way to achieve what I said is just changing isAutoDetected so that if IS_GRAALVM_NATIVE is true, then we return true for LIBRARY_SLF4J. Actually, even better long term, let's return true for LIBRARY_COMMONS too, and that way for GraalVM Native we just brought ahead what's planned for 2.4 anyway.

Copy link
Author

Choose a reason for hiding this comment

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

I'm having a deeper look at it.

Copy link
Author

@fugerit79 fugerit79 Jan 18, 2025

Choose a reason for hiding this comment

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

@ddekany let's see if this is ok for you.

I preferred to create separate methods so maybe it's easier to understand than complex conditional statements, do you agree?

    // legacy auto-detection (until FreeMarker 2.3.X)
    private static boolean isAutoDetectedLegacy( int libraryEnum ) {
        return !(libraryEnum == LIBRARY_AUTO || libraryEnum == LIBRARY_NONE
                || libraryEnum == LIBRARY_SLF4J || libraryEnum == LIBRARY_COMMONS);
    }

    // next generation auto-detection (FreeMarker 2.4.X and on)
    private static boolean isAutoDetectedNG( int libraryEnum ) {
        return !(libraryEnum == LIBRARY_AUTO || libraryEnum == LIBRARY_NONE);
    }

    private static boolean isAutoDetected(int libraryEnum) {
        // 2.4: Remove libraryEnum == LIBRARY_SLF4J || libraryEnum == LIBRARY_COMMONS (use only isAutoDetectedNG()=
        return IS_GRAALVM_NATIVE ? isAutoDetectedNG(libraryEnum) : isAutoDetectedLegacy(libraryEnum);
    }

At beginning didn't fully grasped the meaning of all logger detection logic. I hope that now it is ok.

{
"pattern": "\\Qfreemarker/ext/beans/DefaultMemberAccessPolicy-rules\\E",
"condition": {
"typeReachable": "freemarker.ext.beans.DefaultMemberAccessPolicy"
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we also need to add freemarker/ext/beans/unsafeMethods.properties accessed from freemarker.ext.beans.LegacyDefaultMemberAccessPolicy here?

Copy link
Author

@fugerit79 fugerit79 Jan 18, 2025

Choose a reason for hiding this comment

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

Maybe 'freemarker/ext/beans/unsafeMethods.properties' when freemarker.ext.beans.LegacyDefaultMemberAccessPolicy is reachable? (just commited it this way)

do you think is it better to squash commits?

*/
public void sayHello() throws Exception {
try (StringWriter buffer = new StringWriter()) {
Version version = Configuration.getVersion(); // using latest version
Copy link
Contributor

Choose a reason for hiding this comment

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

new Version(Configuration.getVersion().toString()), because, recycling the current version like that is in principle not allowed (current only logs a WARN, eventually will throw exception). (Because, people almost always do this by mistake, not understanding what that setting does. So in the rare cases where you really mean to do it, there's some gymnastic involved.)

Copy link
Author

Choose a reason for hiding this comment

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

Ok, updated this one too. (in production projects I never resuse it).

rat-excludes Outdated
# ignore for freemarker-test-graalvm-native module (only used for GraalVM native support compliance)
freemarker-test-graalvm-native/build/**
freemarker-test-graalvm-native/README.md
freemarker-test-graalvm-native/src/main/resources/freemarker-templates/**
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add the licence header to the template instead (inside <#-- ... -->)

Copy link
Author

Choose a reason for hiding this comment

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

I think this is done too. Sorry If i missed it

@fugerit79 fugerit79 force-pushed the 1-add-graalvm-support-to-apache-freemarker branch from 35e569b to 9d13f81 Compare January 21, 2025 21:55
@fugerit79
Copy link
Author

@ddekanyI have squashed everything into two commits :

  • GraalVM native support implementation
  • Tests and CI

I hope all your requests have been fulfilled.

Regards

native-image.properties (to handle initialized at runtime)
resource-config.json (to load configuration files in resources/)
isAutoDetected() in logger does not skip LIBRARY_SLF4J and LIBRARY_COMMONS when IS_GRAALVM_NATIVE = true (anticipate FreeMarker 2.4)

Added GraalVM ready badge
See freemarker-test-native/README.md
@fugerit79 fugerit79 force-pushed the 1-add-graalvm-support-to-apache-freemarker branch from 9d13f81 to c545b0a Compare January 22, 2025 13:36
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.

2 participants