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

Add log4j.plugin.enableBndAnnotations option to PluginProcessor #3258

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

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
Comment on lines +319 to +321
try {
Class.forName("aQute.bnd.annotation.spi.ServiceConsumer");
return true;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This looks for ServiceConsumer on the plugin processor's classpath, while it should look for it on the application classpath.

@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.

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