Skip to content

Conversation

ppkarwasz
Copy link
Contributor

@ppkarwasz ppkarwasz commented Nov 30, 2024

This adds a log4j.plugin.enableBndAnnotations option to the PluginProcessor. Its default value is inferred from the compiler classpath.

We also rename the pluginPackage option to a more coherent log4j.plugin.package option.

Fixes #3251

This adds a `log4j.plugin.enableBndAnnotations` option to the `PluginProcessor`. Its default value is inferred from the compiler classpath.

We also rename the `pluginPackage` option to a more coherent `log4j.plugin.package` option.

Closes #3251
@ppkarwasz ppkarwasz self-assigned this Dec 4, 2024
@vy
Copy link
Member

vy commented Mar 2, 2025

@ppkarwasz, why do we need a toggle property instead of disabling it, always? This very much smells like leaking an internal detail through configuration properties.

@ppkarwasz
Copy link
Contributor Author

@ppkarwasz, why do we need a toggle property instead of disabling it, always? This very much smells like leaking an internal detail through configuration properties.

It is an option that controls the output of the PluginProcessor, so I would not call it an internal detail. Should we disable BND annotations by default (for users) and keep the option for our own builds (the BND annotations are useful to generate the necessary META-INF/services files and similar)?

@vy
Copy link
Member

vy commented Mar 5, 2025

Should we disable BND annotations by default (for users) and keep the option for our own builds (the BND annotations are useful to generate the necessary META-INF/services files and similar)?

Could you elaborate on how we can implement this? Does this also mean, there will be no need for a toggle property?

@ppkarwasz
Copy link
Contributor Author

ppkarwasz commented Mar 5, 2025

Should we disable BND annotations by default (for users) and keep the option for our own builds (the BND annotations are useful to generate the necessary META-INF/services files and similar)?

Could you elaborate on how we can implement this? Does this also mean, there will be no need for a toggle property?

What I mean is, I just replace:

private static boolean isServiceConsumerClassPresent() {
try {
Class.forName("aQute.bnd.annotation.spi.ServiceConsumer");
return true;
} catch (ClassNotFoundException e) {
return false;
}
}

with return false and we add -Alog4j.plugin.enabledBndAnnotations=true to our builds. BND runs after compile and it registers the PluginService in the correct files, due to the presence of the @ServiceProvider annotation.

The processor lives in the annotation processor path, so `Class.forName` does **not** check for the presence of the BND annotation on the classpath.
Copy link

Job Requested goals Build Tool Version Build Outcome Build Scan®
build-macos-latest clean install 3.9.8 Build Scan PUBLISHED
build-ubuntu-latest clean install 3.9.8 Build Scan PUBLISHED
build-windows-latest clean install 3.9.8 Build Scan PUBLISHED
Generated by gradle/develocity-actions

@ppkarwasz ppkarwasz merged commit 44a9421 into main Apr 16, 2025
11 checks passed
@ppkarwasz ppkarwasz deleted the fix/main/3151_plugin_processor_bnd_annotations branch April 16, 2025 20:31
@github-project-automation github-project-automation bot moved this from To triage to Done in Log4j bug tracker Apr 16, 2025
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.

2 participants