search

vert-x3 / vertx-dependencies

Defines the versions of the Vert.x components of the Vert.x stack.
Apache License 2.0
12 stars 40 forks source link

Vert.x forces users to import log4j bom #179

Open tsegismont opened 5 months ago

tsegismont commented 5 months ago

In order to make sure logging frameworks are not transitive dependencies of vert.x, we have forced log4j and others versions and scope in 1c6a087478902b552ccc2afd05960a616925561b

This prevents users from simply adding log4j core or logback to their project dependencies in order to get logging to work (see forum or StackOverflow)

The workaround is to add the log4j bom before the vertx stack depchain in the user project. We do the same in the Vert.x starter:

https://github.com/vert-x3/vertx-starter/blob/03615431ea8dab854cf7893bf294fd6811711850/pom.xml#L20-L44

tsegismont commented 5 months ago

In order to fix this, we could use the Maven enforcer plugin in the Vert.x parent, configured with a rule that checks the scope/optional attributes of the logging framework dependencies.

tsegismont commented 5 months ago

There is the banned dependencies built-in rule in the Maven enforcer project which does almost what we need: it can be configured to authorize some dependencies only if they have a certain scope.

For example, this config allows log4j and slf4j only if they have test scope.

                  <excludes>
                    <exclude>org.slf4j</exclude>
                    <exclude>org.apache.logging.log4j</exclude>
                  </excludes>
                  <includes>
                    <include>org.slf4j:*:*:jar:test</include>
                    <include>org.apache.logging.log4j:*:*:jar:test</include>
                  </includes>

But this is not exactly what we need, because we want to authorize logging libraries if they are declared optional.

I made this patch to the project:

Index: enforcer-rules/src/main/java/org/apache/maven/enforcer/rules/dependency/BannedDependencies.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/enforcer-rules/src/main/java/org/apache/maven/enforcer/rules/dependency/BannedDependencies.java b/enforcer-rules/src/main/java/org/apache/maven/enforcer/rules/dependency/BannedDependencies.java
--- a/enforcer-rules/src/main/java/org/apache/maven/enforcer/rules/dependency/BannedDependencies.java   (revision 7c543f03b31c7009eb65401b1ed8b2bc80ea97a0)
+++ b/enforcer-rules/src/main/java/org/apache/maven/enforcer/rules/dependency/BannedDependencies.java   (date 1707469829111)
@@ -33,6 +33,11 @@
 @Named("bannedDependencies")
 public final class BannedDependencies extends BannedDependenciesBase {

+    /**
+     * Whether a banned dependency can be permitted if it is declared optional.
+     */
+    private boolean permitOptionalDependencies = true;
+
     @Inject
     BannedDependencies(MavenSession session, ResolverUtil resolverUtil) {
         super(session, resolverUtil);
@@ -41,7 +46,8 @@
     @Override
     protected boolean validate(Artifact artifact) {
         return !ArtifactUtils.matchDependencyArtifact(artifact, getExcludes())
-                || ArtifactUtils.matchDependencyArtifact(artifact, getIncludes());
+                || ArtifactUtils.matchDependencyArtifact(artifact, getIncludes())
+                || (permitOptionalDependencies && artifact.isOptional());
     }

     @Override
@@ -52,7 +58,23 @@
     @Override
     public String toString() {
         return String.format(
-                "BannedDependencies[message=%s, excludes=%s, includes=%s, searchTransitive=%b]",
-                getMessage(), getExcludes(), getIncludes(), isSearchTransitive());
+                "BannedDependencies[message=%s, excludes=%s, includes=%s, searchTransitive=%b, permitOptionalDependencies=%b]",
+                getMessage(), getExcludes(), getIncludes(), isSearchTransitive(), isPermitOptionalDependencies());
+    }
+
+    /**
+     * @return {@code true} if a banned dependency can be permitted if it is declared optional
+     */
+    public boolean isPermitOptionalDependencies() {
+        return permitOptionalDependencies;
+    }
+
+    /**
+     * Set whether a banned dependency can be permitted if it is declared optional.
+     *
+     * @param permitOptionalDependencies {@code true} to permit, otherwise {@code false}
+     */
+    public void setPermitOptionalDependencies(boolean permitOptionalDependencies) {
+        this.permitOptionalDependencies = permitOptionalDependencies;
     }
 }

Using a snapshot build of the enforcer Maven plugin, with this configuration:

          <execution>
            <id>enforce-banned-dependencies</id>
            <goals>
              <goal>enforce</goal>
            </goals>
            <configuration>
              <skip>${skipBannedLoggingDependencyRule}</skip>
              <rules>
                <bannedDependencies>
                  <message>No logging dependencies unless explicitly declared optional</message>
                  <excludes>
                    <exclude>org.slf4j</exclude>
                    <exclude>org.apache.logging.log4j</exclude>
                  </excludes>
                  <includes>
                    <include>org.slf4j:*:*:jar:test</include>
                    <include>org.apache.logging.log4j:*:*:jar:test</include>
                  </includes>
                  <permitOptionalDependencies>true</permitOptionalDependencies>
                </bannedDependencies>
              </rules>
              <fail>true</fail>
            </configuration>
          </execution>

I was able to confirm optional logging dependencies are permitted, others are banned (build fails).

But some projects require a logging dependency (if, for example, the implementation relies on a 3rd-party library that only works with slf4j). In this case, we'd only have to add a property to the module POM file:

diff --git a/pom.xml b/pom.xml
index 4a4a9fa..e39070a 100644
--- a/pom.xml
+++ b/pom.xml
@@ -5,7 +5,7 @@
   <parent>
     <groupId>io.vertx</groupId>
     <artifactId>vertx-ext-parent</artifactId>
-    <version>38</version>
+    <version>39-SNAPSHOT</version>
   </parent>

   <artifactId>vertx-cassandra-client</artifactId>
@@ -22,6 +22,8 @@
     <logback.version>1.3.12</logback.version>

     <jar.manifest>${project.basedir}/src/main/resources/META-INF/MANIFEST.MF</jar.manifest>
+
+    <skipBannedLoggingDependencyRule>true</skipBannedLoggingDependencyRule>
   </properties>

   <dependencyManagement>

I've tested these changes with the vertx aggregator project and then the dependency convergence test in vertx-stack. The builds and test pass succesfully.

tsegismont commented 5 months ago

@vietj any comments before I start discussing the patch with the Maven enforcer plugin committers?

tsegismont commented 4 months ago

Things to consider before moving forward with this: