reschke commented on code in PR #2959:
URL: https://github.com/apache/jackrabbit-oak/pull/2959#discussion_r3425681505


##########
oak-blob-cloud-azure/pom.xml:
##########
@@ -285,7 +285,6 @@
         <dependency>
             <groupId>com.google.guava</groupId>
             <artifactId>guava</artifactId>
-            <version>33.1.0-jre</version>

Review Comment:
   This depencency is embedded intentionally. It matches the version used by 
the Azure SDK. Updating it could be possible, but Azure support doesn't have 
adequate test coverage over here, so I would leave that to someone more 
knowledgeable of the SDK.
   
   For that matter, this dependency will go away when the SDK 8 use is gone.



##########
oak-parent/pom.xml:
##########
@@ -855,6 +879,166 @@
         <artifactId>jsr305</artifactId>
         <version>3.0.2</version>
       </dependency>
+      <!-- azure-keyvault-core:1.2.6 pulls guava:30.1.1-jre, conflicting with 
the direct guava
+           deps in oak-blob-cloud-azure and oak-segment-azure. Additionally, 
in reactor builds

Review Comment:
   It doesn't conflict, because these are embeds. One reason for being embeds 
is to avoid conflicts.



##########
oak-parent/pom.xml:
##########
@@ -706,6 +724,12 @@
         <artifactId>caffeine</artifactId>
         <version>3.1.8</version>
       </dependency>
+      <!-- guava:33.5.0-jre pulls 2.41.0; caffeine:3.1.8 pulls 2.21.1 -->

Review Comment:
   1) Guava will go anyway.
   
   2) It would be good to understand why that shows up as a dependency; given 
it's nature as a code checking tool. It might be better to suppress it in the 
place where we use Caffeine.
   
   In any case, if it ever disappears as a Caffeine dependeny, this will leave 
noise over here.



##########
oak-parent/pom.xml:
##########
@@ -855,6 +879,166 @@
         <artifactId>jsr305</artifactId>
         <version>3.0.2</version>
       </dependency>
+      <!-- azure-keyvault-core:1.2.6 pulls guava:30.1.1-jre, conflicting with 
the direct guava
+           deps in oak-blob-cloud-azure and oak-segment-azure. Additionally, 
in reactor builds
+           Maven uses oak-shaded-guava's original pom.xml rather than 
dependency-reduced-pom.xml,
+           so guava:33.5.0-jre also appears as a transitive dep (making guava 
optional in
+           oak-shaded-guava would eliminate that false positive, but not the 
azure-keyvault conflict). -->
+      <dependency>
+        <groupId>com.google.guava</groupId>
+        <artifactId>guava</artifactId>
+        <version>33.5.0-jre</version>
+      </dependency>
+      <!-- jackrabbit-core:2.22.3 pulls oak-jackrabbit-api:1.88.0 -->
+      <dependency>
+        <groupId>org.apache.jackrabbit</groupId>
+        <artifactId>oak-jackrabbit-api</artifactId>
+        <version>${project.version}</version>
+      </dependency>
+      <!-- oak-auth-ldap has a direct dep on 2.12.0; api-all:2.0.1 pulls 2.8.0 
-->
+      <dependency>
+        <groupId>org.apache.commons</groupId>

Review Comment:
   This is embedded for good reasons.



##########
oak-parent/pom.xml:
##########
@@ -855,6 +879,166 @@
         <artifactId>jsr305</artifactId>
         <version>3.0.2</version>
       </dependency>
+      <!-- azure-keyvault-core:1.2.6 pulls guava:30.1.1-jre, conflicting with 
the direct guava
+           deps in oak-blob-cloud-azure and oak-segment-azure. Additionally, 
in reactor builds
+           Maven uses oak-shaded-guava's original pom.xml rather than 
dependency-reduced-pom.xml,
+           so guava:33.5.0-jre also appears as a transitive dep (making guava 
optional in
+           oak-shaded-guava would eliminate that false positive, but not the 
azure-keyvault conflict). -->
+      <dependency>
+        <groupId>com.google.guava</groupId>
+        <artifactId>guava</artifactId>
+        <version>33.5.0-jre</version>
+      </dependency>
+      <!-- jackrabbit-core:2.22.3 pulls oak-jackrabbit-api:1.88.0 -->
+      <dependency>
+        <groupId>org.apache.jackrabbit</groupId>
+        <artifactId>oak-jackrabbit-api</artifactId>
+        <version>${project.version}</version>
+      </dependency>
+      <!-- oak-auth-ldap has a direct dep on 2.12.0; api-all:2.0.1 pulls 2.8.0 
-->
+      <dependency>
+        <groupId>org.apache.commons</groupId>
+        <artifactId>commons-pool2</artifactId>
+        <version>2.12.0</version>
+      </dependency>
+      <!-- oak-auth-ldap has a direct dep on 2.1.12; api-all:2.0.1 pulls 2.1.3 
-->
+      <dependency>
+        <groupId>org.apache.mina</groupId>
+        <artifactId>mina-core</artifactId>

Review Comment:
   embedded.



##########
oak-run-commons/pom.xml:
##########
@@ -141,6 +141,10 @@
                     <groupId>org.apache.lucene</groupId>
                     <artifactId>lucene-core</artifactId>
                 </exclusion>
+                <!-- jackrabbit-core:2.22.3 (via jackrabbit-parent) declares 
tika.version=2.4.1,

Review Comment:
   Actually, the problem was that there *was* embedding, and that was caused by 
a plugin version change (probably fixing a bug).



##########
oak-parent/pom.xml:
##########
@@ -855,6 +879,166 @@
         <artifactId>jsr305</artifactId>
         <version>3.0.2</version>
       </dependency>
+      <!-- azure-keyvault-core:1.2.6 pulls guava:30.1.1-jre, conflicting with 
the direct guava
+           deps in oak-blob-cloud-azure and oak-segment-azure. Additionally, 
in reactor builds
+           Maven uses oak-shaded-guava's original pom.xml rather than 
dependency-reduced-pom.xml,
+           so guava:33.5.0-jre also appears as a transitive dep (making guava 
optional in
+           oak-shaded-guava would eliminate that false positive, but not the 
azure-keyvault conflict). -->

Review Comment:
   we are removing Guava.



##########
oak-parent/pom.xml:
##########
@@ -855,6 +879,166 @@
         <artifactId>jsr305</artifactId>
         <version>3.0.2</version>
       </dependency>
+      <!-- azure-keyvault-core:1.2.6 pulls guava:30.1.1-jre, conflicting with 
the direct guava
+           deps in oak-blob-cloud-azure and oak-segment-azure. Additionally, 
in reactor builds
+           Maven uses oak-shaded-guava's original pom.xml rather than 
dependency-reduced-pom.xml,
+           so guava:33.5.0-jre also appears as a transitive dep (making guava 
optional in
+           oak-shaded-guava would eliminate that false positive, but not the 
azure-keyvault conflict). -->
+      <dependency>
+        <groupId>com.google.guava</groupId>
+        <artifactId>guava</artifactId>
+        <version>33.5.0-jre</version>
+      </dependency>
+      <!-- jackrabbit-core:2.22.3 pulls oak-jackrabbit-api:1.88.0 -->
+      <dependency>
+        <groupId>org.apache.jackrabbit</groupId>
+        <artifactId>oak-jackrabbit-api</artifactId>
+        <version>${project.version}</version>
+      </dependency>
+      <!-- oak-auth-ldap has a direct dep on 2.12.0; api-all:2.0.1 pulls 2.8.0 
-->
+      <dependency>
+        <groupId>org.apache.commons</groupId>
+        <artifactId>commons-pool2</artifactId>
+        <version>2.12.0</version>
+      </dependency>
+      <!-- oak-auth-ldap has a direct dep on 2.1.12; api-all:2.0.1 pulls 2.1.3 
-->
+      <dependency>
+        <groupId>org.apache.mina</groupId>
+        <artifactId>mina-core</artifactId>
+        <version>2.1.12</version>
+      </dependency>
+      <!-- parsson:1.0.5 pulls 2.0.2; elasticsearch-java:8.19.5 pulls 2.0.1 -->
+      <dependency>
+        <groupId>jakarta.json</groupId>
+        <artifactId>jakarta.json-api</artifactId>
+        <version>2.0.2</version>
+      </dependency>
+
+      <!-- jackrabbit-jcr-server:2.22.3 pulls tika-core:2.4.1; oak-lucene uses 
${tika.version} -->
+      <dependency>
+        <groupId>org.apache.tika</groupId>
+        <artifactId>tika-core</artifactId>
+        <version>${tika.version}</version>
+      </dependency>
+      <!-- jetty-annotations (via spring-boot-starter-jetty:2.7.18) pulls 9.6; 
tika-parsers:1.28.5 pulls 9.3 -->
+      <dependency>
+        <groupId>org.ow2.asm</groupId>
+        <artifactId>asm</artifactId>
+        <version>9.6</version>
+      </dependency>
+      <!-- poi:5.2.2 and poi-scratchpad:5.2.2 pull 2.17.2; xmlbeans:5.0.3 (via 
poi-ooxml) pulls 2.17.1 -->
+      <dependency>
+        <groupId>org.apache.logging.log4j</groupId>
+        <artifactId>log4j-api</artifactId>
+        <version>2.17.2</version>
+      </dependency>
+      <!-- oak-run has a direct dep on 1.3.4; aws-java-sdk-core:1.12.791 pulls 
1.1.3, httpclient pulls 1.2 -->
+      <dependency>
+        <groupId>commons-logging</groupId>
+        <artifactId>commons-logging</artifactId>
+        <version>1.3.4</version>
+      </dependency>
+      <!-- azure-identity:1.11.3 pulls 5.13.0; tika-parsers:1.28.5 pulls 
5.12.1 -->
+      <dependency>
+        <groupId>net.java.dev.jna</groupId>
+        <artifactId>jna</artifactId>
+        <version>5.13.0</version>
+      </dependency>
+      <!-- jackson-dataformat-xml:2.19.4 (via oak-segment-azure) pulls 7.1.1; 
tika-parsers:1.28.5 pulls 6.3.1 -->
+      <dependency>

Review Comment:
   we are updating Tika



##########
oak-parent/pom.xml:
##########
@@ -855,6 +879,166 @@
         <artifactId>jsr305</artifactId>
         <version>3.0.2</version>
       </dependency>
+      <!-- azure-keyvault-core:1.2.6 pulls guava:30.1.1-jre, conflicting with 
the direct guava
+           deps in oak-blob-cloud-azure and oak-segment-azure. Additionally, 
in reactor builds
+           Maven uses oak-shaded-guava's original pom.xml rather than 
dependency-reduced-pom.xml,
+           so guava:33.5.0-jre also appears as a transitive dep (making guava 
optional in
+           oak-shaded-guava would eliminate that false positive, but not the 
azure-keyvault conflict). -->
+      <dependency>
+        <groupId>com.google.guava</groupId>
+        <artifactId>guava</artifactId>
+        <version>33.5.0-jre</version>
+      </dependency>
+      <!-- jackrabbit-core:2.22.3 pulls oak-jackrabbit-api:1.88.0 -->
+      <dependency>
+        <groupId>org.apache.jackrabbit</groupId>
+        <artifactId>oak-jackrabbit-api</artifactId>
+        <version>${project.version}</version>
+      </dependency>
+      <!-- oak-auth-ldap has a direct dep on 2.12.0; api-all:2.0.1 pulls 2.8.0 
-->
+      <dependency>
+        <groupId>org.apache.commons</groupId>
+        <artifactId>commons-pool2</artifactId>
+        <version>2.12.0</version>
+      </dependency>
+      <!-- oak-auth-ldap has a direct dep on 2.1.12; api-all:2.0.1 pulls 2.1.3 
-->
+      <dependency>
+        <groupId>org.apache.mina</groupId>
+        <artifactId>mina-core</artifactId>
+        <version>2.1.12</version>
+      </dependency>
+      <!-- parsson:1.0.5 pulls 2.0.2; elasticsearch-java:8.19.5 pulls 2.0.1 -->

Review Comment:
   I just don't see how this matters. We should discuss this in the ticket.



##########
oak-run/pom.xml:
##########
@@ -53,7 +53,7 @@
       + 41 MB build failing on the release profile (OAK-6250)
       + 38 MB. Initial value. Current 35MB plus a 10%
     -->
-    <max.jar.size>92274688</max.jar.size>
+    <max.jar.size>93323264</max.jar.size>

Review Comment:
   why did it increase? (just trying to understand)



##########
oak-segment-azure/pom.xml:
##########
@@ -330,7 +330,6 @@
         <dependency>
             <groupId>com.google.guava</groupId>
             <artifactId>guava</artifactId>
-            <version>33.1.0-jre</version>
         </dependency>

Review Comment:
   No. Embedded for good reasons.



##########
oak-examples/webapp/pom.xml:
##########
@@ -111,7 +111,7 @@
     <dependency>
       <groupId>com.googlecode.json-simple</groupId>
       <artifactId>json-simple</artifactId>
-      <version>1.1</version>

Review Comment:
   Yes, but should be a separate ticket; I can do that.



##########
oak-run/src/main/java/org/apache/jackrabbit/oak/plugins/tika/CSVFileGenerator.java:
##########
@@ -46,6 +46,8 @@ public void generate(FluentIterable<BinaryResource> binaries) 
throws IOException
             CSVPrinter printer = new CSVPrinter(new BufferedWriter(new 
FileWriter(outFile, StandardCharsets.UTF_8)),
                     CSVFileBinaryResourceProvider.FORMAT);
             closer.register(printer);
+            // commons-csv 1.2+ no longer auto-writes the header on 
construction; must be explicit

Review Comment:
   When a code change is needed, that's a good indication that it should happen 
in a separate ticket.
   
   I can do that.



##########
oak-parent/pom.xml:
##########
@@ -855,6 +879,166 @@
         <artifactId>jsr305</artifactId>
         <version>3.0.2</version>
       </dependency>
+      <!-- azure-keyvault-core:1.2.6 pulls guava:30.1.1-jre, conflicting with 
the direct guava
+           deps in oak-blob-cloud-azure and oak-segment-azure. Additionally, 
in reactor builds
+           Maven uses oak-shaded-guava's original pom.xml rather than 
dependency-reduced-pom.xml,
+           so guava:33.5.0-jre also appears as a transitive dep (making guava 
optional in
+           oak-shaded-guava would eliminate that false positive, but not the 
azure-keyvault conflict). -->
+      <dependency>
+        <groupId>com.google.guava</groupId>
+        <artifactId>guava</artifactId>
+        <version>33.5.0-jre</version>
+      </dependency>
+      <!-- jackrabbit-core:2.22.3 pulls oak-jackrabbit-api:1.88.0 -->
+      <dependency>
+        <groupId>org.apache.jackrabbit</groupId>
+        <artifactId>oak-jackrabbit-api</artifactId>
+        <version>${project.version}</version>
+      </dependency>
+      <!-- oak-auth-ldap has a direct dep on 2.12.0; api-all:2.0.1 pulls 2.8.0 
-->
+      <dependency>
+        <groupId>org.apache.commons</groupId>
+        <artifactId>commons-pool2</artifactId>
+        <version>2.12.0</version>
+      </dependency>
+      <!-- oak-auth-ldap has a direct dep on 2.1.12; api-all:2.0.1 pulls 2.1.3 
-->
+      <dependency>
+        <groupId>org.apache.mina</groupId>
+        <artifactId>mina-core</artifactId>
+        <version>2.1.12</version>
+      </dependency>
+      <!-- parsson:1.0.5 pulls 2.0.2; elasticsearch-java:8.19.5 pulls 2.0.1 -->
+      <dependency>
+        <groupId>jakarta.json</groupId>
+        <artifactId>jakarta.json-api</artifactId>
+        <version>2.0.2</version>
+      </dependency>
+
+      <!-- jackrabbit-jcr-server:2.22.3 pulls tika-core:2.4.1; oak-lucene uses 
${tika.version} -->
+      <dependency>
+        <groupId>org.apache.tika</groupId>
+        <artifactId>tika-core</artifactId>
+        <version>${tika.version}</version>
+      </dependency>
+      <!-- jetty-annotations (via spring-boot-starter-jetty:2.7.18) pulls 9.6; 
tika-parsers:1.28.5 pulls 9.3 -->
+      <dependency>
+        <groupId>org.ow2.asm</groupId>
+        <artifactId>asm</artifactId>
+        <version>9.6</version>
+      </dependency>

Review Comment:
   See above. These are no problems technically. This change just adds noise 
here. We would need to maintain the reference here, plus remove it when it's 
not needed anymore.
   
   Do we have tooling for that?



-- 
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]

Reply via email to