gemmellr commented on a change in pull request #3846:
URL: https://github.com/apache/activemq-artemis/pull/3846#discussion_r748722072
##########
File path: artemis-commons/pom.xml
##########
@@ -53,6 +71,7 @@
<dependency>
<groupId>com.google.errorprone</groupId>
<artifactId>error_prone_core</artifactId>
+ <scope>provided</scope>
Review comment:
Did this make a difference? The dependencyManagement is setting the
scope so it should already be at an effective provided. (Mainly just curious
why you actually changed it, personally I do prefer each module states it)
##########
File path: artemis-core-client-osgi/pom.xml
##########
@@ -35,6 +35,17 @@
<groupId>org.apache.activemq</groupId>
<artifactId>artemis-commons</artifactId>
<version>${project.version}</version>
+ <!-- TODO: even though these are shaded, maven still picking these up
on builds -->
+ <exclusions>
+ <exclusion>
+ <groupId>org.apache.johnzon</groupId>
+ <artifactId>johnzon-core</artifactId>
+ </exclusion>
+ <exclusion>
+ <groupId>jakarta.json</groupId>
+ <artifactId>jakarta.json-api</artifactId>
+ </exclusion>
+ </exclusions>
Review comment:
As with other comment, you had to do this since the deps are still
listed as hard deps and so anything using artemis-commons will still get them
transitively, unless you say otherwise in artemis-commons that they shouldnt.
If you change that you will be able to remove all these repeated exclusions.
##########
File path: artemis-commons/pom.xml
##########
@@ -32,6 +32,24 @@
</properties>
<dependencies>
+
+ <!-- Johnzon and JSON is meant to be referenced only here on this
package (commons)
+ and we should not leak any dependencies to JSON or Johnzon in any
of our packages -->
+ <dependency>
+ <groupId>org.apache.johnzon</groupId>
+ <artifactId>johnzon-core</artifactId>
+ <version>${johnzon.version}</version>
+ <scope>compile</scope>
Review comment:
You should use "provided" scope if you dont want things to get these
transitively because you are going to shade them, otherwise anything depending
on this module will clearly still depend on it transitively, since the module
says it depends on them. If you use provided scope then dependent modules wont
get these unless they declared their own dependency on it.
##########
File path:
artemis-core-client/src/main/java/org/apache/activemq/artemis/api/core/JsonUtil.java
##########
@@ -16,13 +16,13 @@
*/
package org.apache.activemq.artemis.api.core;
-import javax.json.JsonArray;
-import javax.json.JsonArrayBuilder;
-import javax.json.JsonNumber;
-import javax.json.JsonObject;
-import javax.json.JsonObjectBuilder;
-import javax.json.JsonString;
-import javax.json.JsonValue;
+import org.apache.activemq.artemis.commons.json.JsonArray;
+import org.apache.activemq.artemis.commons.json.JsonArrayBuilder;
+import org.apache.activemq.artemis.commons.json.JsonNumber;
+import org.apache.activemq.artemis.commons.json.JsonObject;
+import org.apache.activemq.artemis.commons.json.JsonObjectBuilder;
+import org.apache.activemq.artemis.commons.json.JsonString;
+import org.apache.activemq.artemis.commons.json.JsonValue;
Review comment:
Just to be sure its intended/realised...noting this is presumably a
breaking change for all existing management related usage of these bits. EDIT:
As the later test changes show.
##########
File path: artemis-commons/pom.xml
##########
@@ -111,6 +126,46 @@
</execution>
</executions>
</plugin>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-shade-plugin</artifactId>
+ <version>3.2.4</version>
Review comment:
This plugins version is already being managed (to 3.2.0) in the parent
pom pluginManagent, as it should be. Rely on that consistent version instead of
adding inconsistent module-specific plugin usages.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
