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 BND configuration for log4j-api #1820

Merged
merged 1 commit into from
Oct 2, 2023
Merged

Conversation

ppkarwasz
Copy link
Contributor

@ppkarwasz ppkarwasz commented Sep 26, 2023

This PR performs the necessary changes to switch log4j-api from maven-bundle-plugin and a manually written module-info.java to bnd-maven-plugin.

Before

In version 2.20.0 the OSGi descriptor had these imports/exports:

Import-Package
  org.apache.logging.log4j.message       
  org.apache.logging.log4j.simple        
  org.apache.logging.log4j.status        
  org.apache.logging.log4j.util          
  org.osgi.framework                     {version=[1.8,2)}
  org.osgi.framework.wiring              {version=[1.2,2)}
  sun.reflect                            {resolution:=optional}

The decompiled module descriptor was:

module org.apache.logging.log4j@2.20.0 {
  requires java.base;
  requires static java.sql;
  exports org.apache.logging.log4j;
  exports org.apache.logging.log4j.message;
  exports org.apache.logging.log4j.simple;
  exports org.apache.logging.log4j.spi;
  exports org.apache.logging.log4j.status;
  exports org.apache.logging.log4j.util;
  uses org.apache.logging.log4j.spi.Provider;
  uses org.apache.logging.log4j.util.PropertySource;
  uses org.apache.logging.log4j.message.ThreadDumpMessage$ThreadInfoFactory;
  provides  org.apache.logging.log4j.util.PropertySource with
    org.apache.logging.log4j.util.EnvironmentPropertySource,
    org.apache.logging.log4j.util.SystemPropertiesPropertySource;
}

After

After this PR the OSGi imports are:

Import-Package
  org.apache.logging.log4j.message       
  org.apache.logging.log4j.simple        
  org.apache.logging.log4j.status        
  org.apache.logging.log4j.util          
  org.osgi.framework                     {version=[1.8,2)}
  org.osgi.framework.wiring              {version=[1.2,2)}

and the automatically generated module descriptor is:

module org.apache.logging.log4j@2.21.0.SNAPSHOT {
  requires java.base;
  requires static java.management;
  requires static java.sql;
  requires static org.osgi.core;
  exports org.apache.logging.log4j;
  exports org.apache.logging.log4j.message;
  exports org.apache.logging.log4j.simple;
  exports org.apache.logging.log4j.spi;
  exports org.apache.logging.log4j.status;
  exports org.apache.logging.log4j.util;
  uses org.apache.logging.log4j.message.ThreadDumpMessage$ThreadInfoFactory;
  uses org.apache.logging.log4j.spi.Provider;
  uses org.apache.logging.log4j.util.PropertySource;
  provides  org.apache.logging.log4j.util.PropertySource with
    org.apache.logging.log4j.util.EnvironmentPropertySource,
    org.apache.logging.log4j.util.SystemPropertiesPropertySource;
}

Updates: removed the static transitive requires, which seem to work as simple transitive requires.

@ppkarwasz ppkarwasz mentioned this pull request Sep 26, 2023
29 tasks
@HannesWell
Copy link
Contributor

HannesWell commented Sep 26, 2023

First of all, it is great to see you using more automation to generation better OSGi and JMPS metadata in a simpler way.

Differences in the OSGi descriptor:

  • the import statement on sun.reflect disappears,
  • an import statement on org.apache.logging.log4j.internal appears.

You didn't mention the exports in the initial message, but I assume org.apache.logging.log4j.internal is exported from log4j-api's MANIFEST.MF, isn't it? Is that intentional?

In case you want to disable imports of packages that are exported by a Bundle you can append -noimport:=true to the Export-Header in the bnd-instructions. E.g. Export-Package= foo.bar.impl.*;-noimport:=true, * to prevent self-imports of foo.bar.impl.* packages or Export-Package= *;-noimport:=true for all packages as described here: https://bnd.bndtools.org/heads/export_package.html

@ppkarwasz
Copy link
Contributor Author

ppkarwasz commented Sep 26, 2023

@HannesWell ,

You didn't mention the exports in the initial message, but I assume org.apache.logging.log4j.internal is exported from log4j-api's MANIFEST.MF, isn't it? Is that intentional?

The org.apache.logging.log4j.internal package was not exported in 2.20.0 and is not exported now. It is not mentioned in the POM, because BND detects this package as internal, whereas it can not find the org.apache.logging.log4j.util.internal package (which is in META-INF/versions/9.

In case you want to disable imports of packages that are exported by a Bundle you can append -noimport:=true to the Export-Header in the bnd-instructions. E.g. Export-Package= foo.bar.impl.*;-noimport:=true, * to prevent self-imports of foo.bar.impl.* packages or Export-Package= *;-noimport:=true for all packages as described here: https://bnd.bndtools.org/heads/export_package.html

Should we disable self-imports? As far as I understand, if those imports are in place log4j-api and PAX Logging can use the same classes.

This is the complete OSGi manifest in case you find more bugs:

[MANIFEST]

Bundle-ActivationPolicy                 lazy
Bundle-Activator                        org.apache.logging.log4j.util.Activator
Bundle-Description                      The Apache Log4j API
Bundle-License                          "Apache-2.0";link="https://www.apache.org/licenses/LICENSE-2.0.txt"
Bundle-ManifestVersion                  2
Bundle-Name                             Apache Log4j API
Bundle-SymbolicName                     org.apache.logging.log4j.api
Bundle-Vendor                           The Apache Software Foundation
Bundle-Version                          2.21.0.SNAPSHOT
Export-Package                          org.apache.logging.log4j.message;uses:="org.apache.logging.log4j.util";version="2.21.0"
                                        org.apache.logging.log4j.simple;uses:="org.apache.logging.log4j,org.apache.logging.log4j.message,org.apache.logging.log4j.spi,org.apache.logging.log4j.util";version="2.21.0"
                                        org.apache.logging.log4j.spi;uses:="org.apache.logging.log4j,org.apache.logging.log4j.message,org.apache.logging.log4j.util";version="2.21.0"
                                        org.apache.logging.log4j.status;uses:="org.apache.logging.log4j,org.apache.logging.log4j.message,org.apache.logging.log4j.spi";version="2.21.0"
                                        org.apache.logging.log4j.util;uses:="org.apache.logging.log4j.message,org.apache.logging.log4j.spi,org.osgi.framework";version="2.21.0"
                                        org.apache.logging.log4j;uses:="org.apache.logging.log4j.message,org.apache.logging.log4j.spi,org.apache.logging.log4j.util";version="2.21.0"
Import-Package                          org.apache.logging.log4j.message
                                        org.apache.logging.log4j.simple
                                        org.apache.logging.log4j.status
                                        org.apache.logging.log4j.util
                                        org.osgi.framework.wiring;version="[1.2,2)"
                                        org.osgi.framework;version="[1.8,2)"
Manifest-Version                        1.0
Multi-Release                           true
Private-Package                         org.apache.logging.log4j.internal
Provide-Capability                      osgi.service;objectClass:List<String>="org.apache.logging.log4j.util.PropertySource";effective:=active
                                        osgi.serviceloader;osgi.serviceloader="org.apache.logging.log4j.util.PropertySource";register:="org.apache.logging.log4j.util.EnvironmentPropertySource"
                                        osgi.serviceloader;osgi.serviceloader="org.apache.logging.log4j.util.PropertySource";register:="org.apache.logging.log4j.util.SystemPropertiesPropertySource"
Require-Capability                      osgi.ee;filter:="(&(osgi.ee=JavaSE)(version=1.8))"
                                        osgi.extender;filter:="(&(osgi.extender=osgi.serviceloader.processor)(version>=1.0.0)(!(version>=2.0.0)))";resolution:=optional
                                        osgi.extender;filter:="(&(osgi.extender=osgi.serviceloader.registrar)(version>=1.0.0)(!(version>=2.0.0)))";resolution:=optional
                                        osgi.serviceloader;filter:="(osgi.serviceloader=org.apache.logging.log4j.message.ThreadDumpMessage$ThreadInfoFactory)";osgi.serviceloader="org.apache.logging.log4j.message.ThreadDumpMessage$ThreadInfoFactory";cardinality:=single;resolution:=optional
                                        osgi.serviceloader;filter:="(osgi.serviceloader=org.apache.logging.log4j.spi.Provider)";osgi.serviceloader="org.apache.logging.log4j.spi.Provider";cardinality:=multiple;resolution:=optional
                                        osgi.serviceloader;filter:="(osgi.serviceloader=org.apache.logging.log4j.util.PropertySource)";osgi.serviceloader="org.apache.logging.log4j.util.PropertySource";cardinality:=multiple;resolution:=optional

Updated: 2023-09-28

@HannesWell
Copy link
Contributor

HannesWell commented Sep 26, 2023

You didn't mention the exports in the initial message, but I assume org.apache.logging.log4j.internal is exported from log4j-api's MANIFEST.MF, isn't it? Is that intentional?

The org.apache.logging.log4j.internal package was not exported in 2.20.0 and is not exported now. It is not mentioned in the POM, because BND detects this package as internal, whereas it can not find the org.apache.logging.log4j.util.internal package (which is in META-INF/versions/9.

Thanks for the clarification. That makes sense.
If you want you can also remove headers from the generated MANIFEST.MF using the -removeheaders instruction. In this case just: -removeheaders: Private-Package
For an OSGi runtime this header is irrelevant and thus ignored and I don't know any other purpose than maybe BND-internal, but I usually remove it and have also seen other projects that remove it too, just to keep the MANIFEST a bit less crowded.

Should we disable self-imports? As far as I understand, if those imports are in place log4j-api and PAX Logging can use the same classes.

That's a good question and a topic that's a bit controversial. It actually only makes sense if there there are multiple really different provides (not only different versions of the same api-bundle) to achieve the effect you mentioned for PAX-logging.
I cannot tell if PAX-Logging really needs it, maybe @grgrzybek can tell this?
On the other hand I have also seen self-imports causing troubles because it can effectively widened the version range of an exported package and can cause (in specific scenarios) sever problems for the resolver of a OSGi runtime. If self-imports are done, the should at least have a version range that use the next minor version as exclusive upper-bound.
At the moment they have no version restriction at all:

Import-Package                          org.apache.logging.log4j.message
                                        org.apache.logging.log4j.simple
                                        org.apache.logging.log4j.status
                                        org.apache.logging.log4j.util

This can be problematic, if there a log4j-3 is one day (if I see it correctly you are working on this, aren't you?) and a bundle for example imports org.apache.logging.log4j.util;version="[2.20,3)" and there is org.apache.logging.api in version 2.20 and version 3 in the runtime, then the resolver will resolve the package against version 3 or will fail. I'm not 100% what will happen in that case, but I'm relatively certain one wants to avoid this.

This is the complete OSGi manifest in case you find more bugs:

Thanks for this. That was helpful! What I noticed is that you maybe want to add the directive cardinality:=multiple to all/some of the required service-loader capabilities:

                                        osgi.serviceloader;filter:="(osgi.serviceloader=org.apache.logging.log4j.message.ThreadDumpMessage$ThreadInfoFactory)";osgi.serviceloader="org.apache.logging.log4j.message.ThreadDumpMessage$ThreadInfoFactory";resolution:=optional
                                        osgi.serviceloader;filter:="(osgi.serviceloader=org.apache.logging.log4j.spi.Provider)";osgi.serviceloader="org.apache.logging.log4j.spi.Provider";resolution:=optional
                                        osgi.serviceloader;filter:="(osgi.serviceloader=org.apache.logging.log4j.util.PropertySource)";osgi.serviceloader="org.apache.logging.log4j.util.PropertySource";resolution:=optional

Otherwise in case multiple providers are present in the runtime, OSGi will wire at most one provider to the log4j.api bundle, which will then only see that one and not potential others.
IIRC the @ServiceConsumer annotation has a cardinality field that you can set to multiple where it is desired. I assume you want that at least for log4j.spi.Provider.

@grgrzybek
Copy link
Contributor

I'll have a look soon. Please give me some time to check and comment.

Copy link
Member

@jvz jvz left a comment

Choose a reason for hiding this comment

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

I assume the annotations are source-only or at least optional in general (as annotations tend to be when the interpreter for said annotations isn't available).

@ppkarwasz
Copy link
Contributor Author

ppkarwasz commented Sep 26, 2023

I assume the annotations are source-only or at least optional in general (as annotations tend to be when the interpreter for said annotations isn't available).

They have a retention of CLASS, so are not required at runtime.

log4j-to-slf4j/pom.xml Outdated Show resolved Hide resolved
@ppkarwasz
Copy link
Contributor Author

osgi.serviceloader;filter:="(osgi.serviceloader=org.apache.logging.log4j.message.ThreadDumpMessage$ThreadInfoFactory)";osgi.serviceloader="org.apache.logging.log4j.message.ThreadDumpMessage$ThreadInfoFactory";resolution:=optional
osgi.serviceloader;filter:="(osgi.serviceloader=org.apache.logging.log4j.spi.Provider)";osgi.serviceloader="org.apache.logging.log4j.spi.Provider";resolution:=optional
osgi.serviceloader;filter:="(osgi.serviceloader=org.apache.logging.log4j.util.PropertySource)";osgi.serviceloader="org.apache.logging.log4j.util.PropertySource";resolution:=optional

Otherwise in case multiple providers are present in the runtime, OSGi will wire at most one provider to the log4j.api bundle, which will then only see that one and not potential others. IIRC the @ServiceConsumer annotation has a cardinality field that you can set to multiple where it is desired. I assume you want that at least for log4j.spi.Provider.

@HannesWell, thanks I fixed that.

Differences in the module descriptor:
 * `java.management` is added to the optional requires.

Differences in the OSGi descriptor:
 * the import statement on `sun.reflect` disappears,
 * an import statement on `org.apache.logging.log4j.internal` appears.
@ppkarwasz ppkarwasz merged commit 2554847 into apache:bnd Oct 2, 2023
@ppkarwasz ppkarwasz deleted the bnd-api branch October 2, 2023 13:21
@ppkarwasz
Copy link
Contributor Author

@grgrzybek @HannesWell,

I have merged these changes into the bnd branch (#1819), which offers a more global view of all modules.

However please feel free to add comments to this PR if you find other bugs in the OSGi descriptors. As I already mentioned elsewhere, there are no committer of the Log4j project that current work in OSGi environments and we value the opinion of experts.

@grgrzybek
Copy link
Contributor

Sure sure, I remember - I have this tab opened to comment at some point, but I have very tight schedule recently...

@grgrzybek
Copy link
Contributor

The problem with OSGi headers generated into bundles' META-INF/MANIFEST.MF is that we can't simply verify them by looking at them ;)
In my experience I had a lot of problems with some headers causing problems in connection with other unrelated bundles which together made OSGi resolver confused...

I have two things.

  1. [2.2.x] Upgrade to Log4j2 2.21.0 ops4j/org.ops4j.pax.logging#535 is an update of Pax Logging to Log4j2. I have to shade very small number of classes (the most important one is org/apache/logging/log4j/core/appender/db/jdbc/JdbcDatabaseManager.java to make it more OSGi-friendly), but this is not a big problem. The most important problem I had was duplicate generation of Bundle-Activator - because bnd was scanning Log4j2 classes and detected:
@Header(name = org.osgi.framework.Constants.BUNDLE_ACTIVATOR, value = "${@class}")

on org.apache.logging.log4j.core.osgi.Activator, so pax-logging-log4j2 had TWO activators configured (comma separated). I had to use:

<_bundleannotations />

or (bnd version):

-bundleannotations: false
  1. something more helpful for you ;) Some time ago I wrote a handy (at least for me) too: https://github.com/ops4j/org.ops4j.tools/tree/main/maven/osgi-report-maven-plugin. See its documentation.

I used two tags: rel/2.20.0 and rel/2.21.0 and run:

mvn clean install -DskipTests org.ops4j.tools.maven:osgi-report-maven-plugin:0.1.1:manifest-summary

which generates target/manifest-summary.txt file which tries to generate highly comparable report for all the manifest headers. To ensure it's useful, all the values are sorted, so we can compare different versions and check whether you miss some Import-Package or something similar.

For example comparing 2.21.0 (with this #1820 merged) and 2.20.0 I got this diff:

-= org.apache.logging.log4j:log4j-1.2-api:jar:2.20.0
+= org.apache.logging.log4j:log4j-1.2-api:jar:2.21.0
 
 == General attributes
 
-Automatic-Module-Name: org.apache.log4j
-Build-Jdk-Spec: 1.8
-Created-By: Apache Maven Bundle Plugin 5.1.8
 Fragment-Host: org.apache.logging.log4j.core
-Implementation-Title: Apache Log4j 1.x Compatibility API
-Implementation-Vendor: The Apache Software Foundation
-Implementation-Vendor-Id: org.apache
-Implementation-Version: 2.20.0
-Manifest-Version: 1.0
-Specification-Title: Apache Log4j 1.x Compatibility API
-Specification-Vendor: The Apache Software Foundation
-Specification-Version: 2.20
-Tool: Bnd-6.3.1.202206071316
-X-Compile-Source-JDK: 1.8
-X-Compile-Target-JDK: 1.8
+Manifest-Version: 1.0
+Multi-Release: false
 
 == Bundle attributes
 
 Bundle-Description: The Apache Log4j 1.x Compatibility API
-Bundle-DocURL: https://www.apache.org/
-Bundle-License: https://www.apache.org/licenses/LICENSE-2.0.txt
+Bundle-License: "Apache-2.0";link="https://www.apache.org/licenses/LICENSE-2.0.txt"
 Bundle-ManifestVersion: 2
 Bundle-Name: Apache Log4j 1.x Compatibility API
-Bundle-SymbolicName: org.apache.logging.log4j.1.2-api
+Bundle-SymbolicName: org.apache.logging.log4j.1.2.api
 Bundle-Vendor: The Apache Software Foundation
-Bundle-Version: 2.20.0
+Bundle-Version: 2.21.0
 
 == Service attributes
 
@@ -43,118 +30,56 @@
 
 Export-Package:
     org.apache.log4j
-        version = 2.20.0
+        version = 2.20.1
         uses :=
             org.apache.log4j.helpers
             org.apache.log4j.or
             org.apache.log4j.spi
             org.apache.logging.log4j.message
             org.apache.logging.log4j.spi
-    org.apache.log4j.bridge
-        version = 2.20.0
-        uses :=
-            org.apache.log4j
-            org.apache.log4j.rewrite
-            org.apache.log4j.spi
-            org.apache.logging.log4j.message
-            org.apache.logging.log4j.util
-    org.apache.log4j.builders
...

there are many other differences which are rational because you've switched from maven-bundle-plugin to bnd-maven-plugin like removal of Implementation-* and Specification-* headers or removal of self-imports (I agree that these are not necessary)

2.21.0 also have much more required capabilities, like:

Require-Capability:
    osgi.ee
        filter := (&(osgi.ee=JavaSE)(version=1.8))
    osgi.extender
        filter := (&(osgi.extender=osgi.serviceloader.processor)(version>=1.0.0)(!(version>=2.0.0)))
        resolution := optional
    osgi.extender
        filter := (&(osgi.extender=osgi.serviceloader.registrar)(version>=1.0.0)(!(version>=2.0.0)))
        resolution := optional
    osgi.serviceloader
        osgi.serviceloader = org.apache.logging.log4j.message.ThreadDumpMessage$ThreadInfoFactory
        cardinality := single
        filter := (osgi.serviceloader=org.apache.logging.log4j.message.ThreadDumpMessage$ThreadInfoFactory)
        resolution := optional
    osgi.serviceloader
        osgi.serviceloader = org.apache.logging.log4j.spi.Provider
        cardinality := multiple
        filter := (osgi.serviceloader=org.apache.logging.log4j.spi.Provider)
        resolution := optional
    osgi.serviceloader
        osgi.serviceloader = org.apache.logging.log4j.util.PropertySource
        cardinality := multiple
        filter := (osgi.serviceloader=org.apache.logging.log4j.util.PropertySource)
        resolution := optional

which I also support - these tell OSGi developers that Log4j uses Service Loader and it makes Log4j2 really more OSGi friendly.

I can also see Private-Package headers generated which should be removed with:

-removeheaders: Private-Package`

There are some confusing headers, like org.apache.logging.log4j.core bundle with this import:

Import-Package:
...
    org.apache.logging.log4j.core
        version = [2.20,3)
    org.apache.logging.log4j.core.appender.db
        version = [2.21,3)
...

Why some packages are imported with [2.20,3) and some with [2.21,3)?

Please play with:

mvn clean install -DskipTests org.ops4j.tools.maven:osgi-report-maven-plugin:0.1.1:manifest-summary

and see yourself ;)

As a reminder - Pax Logging is quite aggressive - it re-exports Log4j2 and Logback and provides own manifests. The reason was simple (historically) - there are many logging facades available for Pax Logging:

  • SLF4J
  • JULI
  • JUL (java.util.logging)
  • JCL (Commons Logging)
  • JBoss Logging
  • Log4j1 (API)
  • Log4j2 (API)
  • OSGi
  • Avalon

And initially there was only one backend - Log4j1. Then Logback and then Log4j2 came.
Also Pax Logging should be as least dependent on anything as possible (so it can be used as for example "bundle 2") and all other bundles should be wired to only this one bundle.

So minimal OSGi runtime could use:

  • System bundle
  • Pax Logging API bundle (with all the facades)
  • Pax Logging Log4j2 bundle as implementation

And with the above config you should get a lot of functinality, appenders etc without a need to install any other bundles.

I guess you could do that installing Log4j2 original bundles itself, but I work in my narrow perspective, where Karaf is the OSGi runtime and Pax Logging API + impl provides the logging functionality. Sure - not a purist-OSGi approach, but works for me for almost 10 years ;)

With original Log4j2 bundles installed and other bundles requiring several facades (like Hibernate needing JBoss Logging or Tomcat requiring JULI or many other bundles requiring SLF4J) the runtime config can become more complicated...

I hope my comment can be helpful (even if a bit complex and long) :)

@ppkarwasz
Copy link
Contributor Author

@grgrzybek,

Thank You for your detailed comment.

I have two things.

  1. [2.2.x] Upgrade to Log4j2 2.21.0 ops4j/org.ops4j.pax.logging#535 is an update of Pax Logging to Log4j2. I have to shade very small number of classes (the most important one is org/apache/logging/log4j/core/appender/db/jdbc/JdbcDatabaseManager.java to make it more OSGi-friendly), but this is not a big problem. The most important problem I had was duplicate generation of Bundle-Activator - because bnd was scanning Log4j2 classes and detected:

I am quite curious why so many classes must be shaded, whereas Logback only requires replacing one class. Maybe we could integrate those changes into Log4j Core?

2.21.0 also have much more required capabilities, like:

Require-Capability:
    osgi.ee
        filter := (&(osgi.ee=JavaSE)(version=1.8))
    osgi.extender
        filter := (&(osgi.extender=osgi.serviceloader.processor)(version>=1.0.0)(!(version>=2.0.0)))
        resolution := optional
    osgi.extender
        filter := (&(osgi.extender=osgi.serviceloader.registrar)(version>=1.0.0)(!(version>=2.0.0)))
        resolution := optional
    osgi.serviceloader
        osgi.serviceloader = org.apache.logging.log4j.message.ThreadDumpMessage$ThreadInfoFactory
        cardinality := single
        filter := (osgi.serviceloader=org.apache.logging.log4j.message.ThreadDumpMessage$ThreadInfoFactory)
        resolution := optional
    osgi.serviceloader
        osgi.serviceloader = org.apache.logging.log4j.spi.Provider
        cardinality := multiple
        filter := (osgi.serviceloader=org.apache.logging.log4j.spi.Provider)
        resolution := optional
    osgi.serviceloader
        osgi.serviceloader = org.apache.logging.log4j.util.PropertySource
        cardinality := multiple
        filter := (osgi.serviceloader=org.apache.logging.log4j.util.PropertySource)
        resolution := optional

which I also support - these tell OSGi developers that Log4j uses Service Loader and it makes Log4j2 really more OSGi friendly.

These requirements were added by @ServiceProvider BND annotation. We need it for the provides statements in the JPMS module descriptor. Regarding OSGi we have some custom solutions like OsgiServiceLocator or the activator you mentioned before, that should allow Log4j API and Log4j Core to function properly without a Service Loader Mediator. They are not well tested, though.

If Service Loader Mediator is a standard component in OSGi environments and it supports the way we use ServiceLoader (which is through MethodHandles.Lookup, cf. ServiceLoaderUtil), we should probably get over the "Not invented here" syndrome and remove our custom code.

There are some confusing headers, like org.apache.logging.log4j.core bundle with this import:

Import-Package:
...
    org.apache.logging.log4j.core
        version = [2.20,3)
    org.apache.logging.log4j.core.appender.db
        version = [2.21,3)
...

Why some packages are imported with [2.20,3) and some with [2.21,3)?

I started adding @Version annotations to all our packages, with the minimal version dumps suggested by bnd-baseline-maven-plugin. So Log4j 2.21.0 is the first release where some packages have a 2.20.x version and other 2.21.x.

The idea behind this is: we rarely add new methods to the Log4j API. So we don't want to scare implementors by announcing "Log4j API 2.42.x is out, you need to upgrade your implementations".

So minimal OSGi runtime could use:

* System bundle

* Pax Logging API bundle (with all the facades)

* Pax Logging Log4j2 bundle as implementation

I guess that these do not require Service Loader Mediator.

So if I understand correctly, Pax Logging covers the needs of embedded application developers and the fact that Log4j API does not require any additional OSGi bundles (except an implementation) is not a real advantage.

We should probably simplify our OSGi code and require users to use Apache SPI Fly. We can document Pax Logging as an alternative for developers that require a minimal environment.

I hope my comment can be helpful (even if a bit complex and long) :)

It is very helpful, thanks.

@grgrzybek
Copy link
Contributor

@grgrzybek,

Thank You for your detailed comment.

I have two things.

  1. [2.2.x] Upgrade to Log4j2 2.21.0 ops4j/org.ops4j.pax.logging#535 is an update of Pax Logging to Log4j2. I have to shade very small number of classes (the most important one is org/apache/logging/log4j/core/appender/db/jdbc/JdbcDatabaseManager.java to make it more OSGi-friendly), but this is not a big problem. The most important problem I had was duplicate generation of Bundle-Activator - because bnd was scanning Log4j2 classes and detected:

I am quite curious why so many classes must be shaded, whereas Logback only requires replacing one class. Maybe we could integrate those changes into Log4j Core?

I can justify all shaded classes:

  • org.apache.logging.log4j.status.StatusLogger - because under OSGi, I want special "simple logger" that's not just printing to stdout
  • org.apache.logging.log4j.LogManager - because I don't need this static block that does some discovery - I have single Log4jv2LoggerContextFactory
  • org.apache.logging.log4j.ThreadContext - because Pax Logging has single Log4jv2ThreadContextStack and Log4jv2ThreadContextMap
  • org.apache.logging.log4j.core.appender.db.jdbc.DataSourceConnectionSource (and JdbcDatabaseManager) - because I implement lazy, OSGi service based connection source
  • org.apache.logging.log4j.core.config.plugins.util.ResolverUtil - because I additionally handle bundle: URL protocol - this indeed could be implemented in Log4j2 itself
  • org.apache.logging.log4j.core.config.status.StatusConfiguration - because I don't want registerNewStatusConsoleListener() called - again, because of OSGi
  • org.apache.logging.log4j.core.impl.ReusableLogEventFactory - I changed mutableLogEventThreadLocal to a threadlocal of WeakReference<MutableLogEvent> instead of just MutableLogEvent - there were memory leaks when (OSGi) refreshing - see ClassLoader leak when pax-logging-log4j2 bundle is refreshed [PAXLOGGING-239] ops4j/org.ops4j.pax.logging#309
  • org.apache.logging.log4j.core.impl.ThrowableProxy - special handling of extra java.lang.Exception field endorsed in Karaf 4
  • org.apache.logging.log4j.core.pattern.DatePatternConverter - threadLocalMutableInstant and threadLocalFormatter keep weak references of original data

It's not that much and maybe some of these could be backported. But I have no strong opinion on that.

2.21.0 also have much more required capabilities, like:

Require-Capability:
    osgi.ee
        filter := (&(osgi.ee=JavaSE)(version=1.8))
    osgi.extender
        filter := (&(osgi.extender=osgi.serviceloader.processor)(version>=1.0.0)(!(version>=2.0.0)))
        resolution := optional
    osgi.extender
        filter := (&(osgi.extender=osgi.serviceloader.registrar)(version>=1.0.0)(!(version>=2.0.0)))
        resolution := optional
    osgi.serviceloader
        osgi.serviceloader = org.apache.logging.log4j.message.ThreadDumpMessage$ThreadInfoFactory
        cardinality := single
        filter := (osgi.serviceloader=org.apache.logging.log4j.message.ThreadDumpMessage$ThreadInfoFactory)
        resolution := optional
    osgi.serviceloader
        osgi.serviceloader = org.apache.logging.log4j.spi.Provider
        cardinality := multiple
        filter := (osgi.serviceloader=org.apache.logging.log4j.spi.Provider)
        resolution := optional
    osgi.serviceloader
        osgi.serviceloader = org.apache.logging.log4j.util.PropertySource
        cardinality := multiple
        filter := (osgi.serviceloader=org.apache.logging.log4j.util.PropertySource)
        resolution := optional

which I also support - these tell OSGi developers that Log4j uses Service Loader and it makes Log4j2 really more OSGi friendly.

These requirements were added by @ServiceProvider BND annotation. We need it for the provides statements in the JPMS module descriptor. Regarding OSGi we have some custom solutions like OsgiServiceLocator or the activator you mentioned before, that should allow Log4j API and Log4j Core to function properly without a Service Loader Mediator. They are not well tested, though.

If Service Loader Mediator is a standard component in OSGi environments and it supports the way we use ServiceLoader (which is through MethodHandles.Lookup, cf. ServiceLoaderUtil), we should probably get over the "Not invented here" syndrome and remove our custom code.

The defaults are usually safe and bnd is quite good at generating such defaults. I (Pax Logging) simply don't want to use Aries-SPI-Fly (implementation of this osgi.serviceloader.processor) because it makes deployments more complex. And Pax Logging does its own, more predictable discovery.

There are some confusing headers, like org.apache.logging.log4j.core bundle with this import:

Import-Package:
...
    org.apache.logging.log4j.core
        version = [2.20,3)
    org.apache.logging.log4j.core.appender.db
        version = [2.21,3)
...

Why some packages are imported with [2.20,3) and some with [2.21,3)?

I started adding @Version annotations to all our packages, with the minimal version dumps suggested by bnd-baseline-maven-plugin. So Log4j 2.21.0 is the first release where some packages have a 2.20.x version and other 2.21.x.

The idea behind this is: we rarely add new methods to the Log4j API. So we don't want to scare implementors by announcing "Log4j API 2.42.x is out, you need to upgrade your implementations".

This is THE SINGLE, BIGGEST OSGi thing that makes it great/bad at the same time. It's all about practice - how many times do you deal with Maven versions (per jar) and how many times you work with OSGi versions (per package). In theory these are two different worlds and this discrepancy is something to be left for developers to solve (according to OSGi designers).

However in my opinion we CAN'T make these package/jar versions disjoint! A package is NOT a deployment unit (despite all these clever OSGi repository specification goals). From the point of view of developer, deployer and even security scanner, it's not that "a package at version x.y.z in a jar a.b.c is vulnerable" - CVE scanners just say that "jar a.b.c is vulnerable". Yes - it's a candidate for great discussion involving good beer. I can only say from my 10 years OSGi experience that while being great design, package versions are the biggest OSGi adoption blocker... In 2023 we can say package versions lead to the point where OSGi is nowadays...

So minimal OSGi runtime could use:

* System bundle

* Pax Logging API bundle (with all the facades)

* Pax Logging Log4j2 bundle as implementation

I guess that these do not require Service Loader Mediator.

So if I understand correctly, Pax Logging covers the needs of embedded application developers and the fact that Log4j API does not require any additional OSGi bundles (except an implementation) is not a real advantage.

Yes - you should treat Pax Logging (API + impl) as kind of embedded Log4j2 with deep (really) OSGi awareness. Having multiple bundles installed makes OSGi runtime much more fragile (especially when you repeatedly install/uninstall new bundles or Karaf features there). Yes - Karaf which resolves bundles on each feature installation/update was refreshing Logging bundles, so making the dependencies minimal made systems much more stable (I'm speaking from JBoss/Red Hat Fuse product based on Karaf)

We should probably simplify our OSGi code and require users to use Apache SPI Fly. We can document Pax Logging as an alternative for developers that require a minimal environment.

From the point of view of Log4j2 itself it's a good idea, because generally, SPI Fly is the best way to handle java.util.ServiceLoader services.

I hope my comment can be helpful (even if a bit complex and long) :)

It is very helpful, thanks.

:)

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.

4 participants