Skip to content

Conversation

ppkarwasz
Copy link
Contributor

@ppkarwasz ppkarwasz commented Jun 21, 2025

Fixes #3771

This PR makes the -Alog4j.graalvm.groupId and -Alog4j.graalvm.artifactId arguments optional.

  • If no arguments are provided, metadata is stored in:

    META-INF/native-image/log4j-generated/<content-derived-value>
    

    Previously an error was thrown.

  • If arguments are provided, files go to:

    META-INF/native-image/log4j-generated/<groupId>/<artifactId>
    

    Previously META-INF/native-image/<groupId>/<artifactId> was used. The new path prevents collisions with user-provided metadata.

Fixes #3771

This PR makes the `-Alog4j.graalvm.groupId` and `-Alog4j.graalvm.artifactId` arguments optional.

* If **no arguments** are provided, metadata is stored in:

  ```
  META-INF/native-image/log4j-generated
  ```
  Previously an error was thrown.

* If **arguments are provided**, files go to:

  ```
  META-INF/native-image/log4j-generated/<groupId>/<artifactId>
  ```
  Previously `META-INF/native-image/<groupId>/<artifactId>` was used. The new path prevents collisions with user-provided metadata.
@ppkarwasz ppkarwasz moved this from To triage to In review in Log4j bug tracker Jun 21, 2025
Modifies the `GraalVmProcessor` to use the hashcode of the descriptor as folder name in the case the user does not provide the recommended compiler options.
@ppkarwasz ppkarwasz requested review from Copilot and vy June 25, 2025 21:58
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR makes the GraalVM metadata processor arguments optional, updates the default storage path to include a log4j-generated prefix, and refactors how reflect-config JSON is generated and emitted.

  • Changed GraalVmProcessor to compute reflect-config once into a byte array and derive a fallback folder name via a hash.
  • Updated getReachabilityMetadataPath to return a warning (not error) and use the new META-INF/native-image/log4j-generated hierarchy.
  • Expanded tests to cover new path logic, warning emission, and metadata contents.

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

File Description
src/changelog/.2.x.x/3771_graalvm-params.xml Added changelog entry for optional GraalVM args
GraalVmProcessor.java Made processor arguments optional, added byte-buffered metadata, new path logic, and warning for missing options
FakePlugin.java / FakeAnnotations.java Added test fixtures for plugins and annotation visitors
GraalVmProcessorTest.java Refactored test helper, added parameterized path tests and warning check
Comments suppressed due to low confidence (2)

log4j-core/src/main/java/org/apache/logging/log4j/core/config/plugins/processor/GraalVmProcessor.java:229

  • [nitpick] Consider adding a JavaDoc comment for this overloaded getReachabilityMetadataPath method to explain the fallbackFolderName parameter and how the fallback hash is computed, improving maintainability for future readers.
    String getReachabilityMetadataPath(

log4j-core/src/main/java/org/apache/logging/log4j/core/config/plugins/processor/GraalVmProcessor.java:230

  • The use of the @Nullable annotation requires an import (e.g., org.jspecify.annotations.Nullable or javax.annotation.Nullable), but no such import is present. Add the correct import to avoid a compile error.
            @Nullable String groupId, @Nullable String artifactId, String fallbackFolderName) {

@ppkarwasz ppkarwasz merged commit f93b6b1 into 2.x Jun 28, 2025
11 checks passed
@ppkarwasz ppkarwasz deleted the fix/graalvm-params branch June 28, 2025 07:23
@github-project-automation github-project-automation bot moved this from In review to Done in Log4j bug tracker Jun 28, 2025
ppkarwasz added a commit that referenced this pull request Jul 5, 2025
Fixes #3771

This PR makes the `-Alog4j.graalvm.groupId` and `-Alog4j.graalvm.artifactId` arguments optional.

* If **no arguments** are provided, metadata is stored in:

  ```
  META-INF/native-image/log4j-generated/<content-derived-value>
  ```
  Previously an error was thrown.

* If **arguments are provided**, files go to:

  ```
  META-INF/native-image/log4j-generated/<groupId>/<artifactId>
  ```
  Previously `META-INF/native-image/<groupId>/<artifactId>` was used. The new path prevents collisions with user-provided metadata.

Co-authored-by: Copilot <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Make GraalVmProcessor Arguments Optional
2 participants