[GitHub] [activemq-artemis] gemmellr commented on a change in pull request #3846: ARTEMIS-3546 Jakarta javax.json incompability on Jakarta all client

[GitBox <gi...@apache.org>]

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: gitbox-unsubscribe@activemq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org