[CELEBORN-906][BUILD] Aligning dependencies between SBT and Maven #1831

cfmcgrady · 2023-08-24T02:56:47Z

What changes were proposed in this pull request?

As title

Why are the changes needed?

This PR ensures dependency alignment between SBT and Maven, based on the audit results implemented in #1797

Does this PR introduce any user-facing change?

No

How was this patch tested?

Pass GA and Review

codecov · 2023-08-24T03:07:25Z

Codecov Report

Merging #1831 (269b9ce) into main (aa35c1c) will increase coverage by 0.06%.
Report is 8 commits behind head on main.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1831      +/-   ##
==========================================
+ Coverage   46.43%   46.48%   +0.06%     
==========================================
  Files         163      163              
  Lines       10135    10143       +8     
  Branches      934      934              
==========================================
+ Hits         4705     4714       +9     
  Misses       5118     5118              
+ Partials      312      311       -1

see 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

pan3793 · 2023-08-24T03:53:10Z

project/CelebornBuild.scala

-  val ioDropwizardMetricsJvm = "io.dropwizard.metrics" % "metrics-jvm" % metricsVersion
-  val ioNetty = "io.netty" % "netty-all" % nettyVersion
+  val hadoopClientApi = "org.apache.hadoop" % "hadoop-client-api" % hadoopVersion excludeAll(
+    ExclusionRule("org.slf4j", "slf4j-api"))


does sbt support globally pin dep's version?

Certainly, the SBT does allow for the specification of a forced dependency revision, e.g.

libraryDependencies ++= Seq( "log4j" % "log4j" % "1.2.14" force() )

However, the docs also point out it is advised against, as indicated by the following considerations:

Forcing can create logical inconsistencies so it’s no longer recommended.

this is an Ivy-only feature and cannot be included in a published pom.xml.

~~another approach, Disable Transitivity~~

By default, these declarations fetch all project dependencies, transitively. In some instances, you may find that the dependencies listed for a project aren’t necessary for it to build. Projects using the Felix OSGI framework, for instance, only explicitly require its main jar to compile and run. Avoid fetching artifact dependencies with either intransitive() or notTransitive(), as in this example:

libraryDependencies += "org.apache.felix" % "org.apache.felix.framework" % "1.8.0" intransitive()

Perform local testing. the intransitive() cannot achieve this functionality.

[warn] Found intransitive dependency (org.apache.hadoop:hadoop-client-api:3.2.4 intransitive) while publishMavenStyle is true, but Maven repositories [warn] do not support intransitive dependencies. Use exclusions instead so transitive dependencies [warn] will be correctly excluded in dependent projects.

Let's use force() way.

Forcing can create logical inconsistencies so it’s no longer recommended.

We know what we are doing, IMO, rules-based dependency derivation is more unpredictable.

this is an Ivy-only feature and cannot be included in a published pom.xml.

It does not affect our use case, since we only publish client-shaded modules, without transitive deps.

Explicitly Exclude from all deps looks ugly. dependencyOverrides seems as clean as force()? if yes, let's try this way.

addressed in 269b9ce, it looks more clearly now.

BTW, dependencyOverrides seems does not support pattern, e.g. io.netty:*, right?

cfmcgrady · 2023-08-24T04:51:32Z

FYI:
SBT's approach to handling dependency conflicts differs from Maven's.

Learn more: SBT Library Management - Eviction Warning.

pan3793 · 2023-08-24T05:09:33Z

pom.xml

+        <exclusions>
+          <exclusion>
+            <groupId>io.netty</groupId>
+            <artifactId>netty-transport-native-epoll</artifactId>


seems we can simply exclude io.netty:* as we manage netty deps by ourselves

This reverts commit 32f7ce0.

waitinfuture

LGTM, thanks! Merging to main. BTW, does this change dependencies? If so, maybe we should modify licenses accordingly.

…2-impl` from SBT profile `spark-3.4` ### What changes were proposed in this pull request? As title ### Why are the changes needed? To address the CI failure introduced in #1831, this pull request resolves the issue by removing the `log4j-slf4j2-impl` dependency from SBT profile `spark-3.4`. This change is prompted by the pinning of `slf4j-api` to version 1.7.36, rendering `log4j-slf4j2-impl` unnecessary. ``` [error] Test org.apache.spark.shuffle.celeborn.SortBasedPusherSuiteJ failed: java.lang.NoSuchMethodError: org.apache.logging.slf4j.Log4jLoggerFactory: method <init>()V not found, took 0.0 sec [error] at org.slf4j.impl.StaticLoggerBinder.<init>(StaticLoggerBinder.java:53) [error] at org.slf4j.impl.StaticLoggerBinder.<clinit>(StaticLoggerBinder.java:41) [error] at org.slf4j.LoggerFactory.bind(LoggerFactory.java:150) [error] at org.slf4j.LoggerFactory.performInitialization(LoggerFactory.java:124) [error] at org.slf4j.LoggerFactory.getILoggerFactory(LoggerFactory.java:417) [error] at org.slf4j.LoggerFactory.getLogger(LoggerFactory.java:362) [error] at org.slf4j.LoggerFactory.getLogger(LoggerFactory.java:3[88](https://github.com/apache/incubator-celeborn/actions/runs/5974971986/job/16210071148#step:4:89)) [error] at org.apache.spark.shuffle.celeborn.SortBasedPusherSuiteJ.<clinit>(SortBasedPusherSuiteJ.java:51) [error] ... [error] Test org.apache.spark.shuffle.celeborn.SortBasedPusherSuiteJ failed: java.lang.NoClassDefFoundError: Could not initialize class org.apache.spark.shuffle.celeborn.SortBasedPusherSuiteJ, took 0.0 sec [error] at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) [error] ... ``` ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Pass GA Closes #1844 from cfmcgrady/celeborn-906-followup. Authored-by: Fu Chen <[email protected]> Signed-off-by: Cheng Pan <[email protected]>

…2-impl` from SBT profile `spark-3.4` ### What changes were proposed in this pull request? As title ### Why are the changes needed? To address the CI failure introduced in apache/celeborn#1831, this pull request resolves the issue by removing the `log4j-slf4j2-impl` dependency from SBT profile `spark-3.4`. This change is prompted by the pinning of `slf4j-api` to version 1.7.36, rendering `log4j-slf4j2-impl` unnecessary. ``` [error] Test org.apache.spark.shuffle.celeborn.SortBasedPusherSuiteJ failed: java.lang.NoSuchMethodError: org.apache.logging.slf4j.Log4jLoggerFactory: method <init>()V not found, took 0.0 sec [error] at org.slf4j.impl.StaticLoggerBinder.<init>(StaticLoggerBinder.java:53) [error] at org.slf4j.impl.StaticLoggerBinder.<clinit>(StaticLoggerBinder.java:41) [error] at org.slf4j.LoggerFactory.bind(LoggerFactory.java:150) [error] at org.slf4j.LoggerFactory.performInitialization(LoggerFactory.java:124) [error] at org.slf4j.LoggerFactory.getILoggerFactory(LoggerFactory.java:417) [error] at org.slf4j.LoggerFactory.getLogger(LoggerFactory.java:362) [error] at org.slf4j.LoggerFactory.getLogger(LoggerFactory.java:3[88](https://github.com/apache/incubator-celeborn/actions/runs/5974971986/job/16210071148#step:4:89)) [error] at org.apache.spark.shuffle.celeborn.SortBasedPusherSuiteJ.<clinit>(SortBasedPusherSuiteJ.java:51) [error] ... [error] Test org.apache.spark.shuffle.celeborn.SortBasedPusherSuiteJ failed: java.lang.NoClassDefFoundError: Could not initialize class org.apache.spark.shuffle.celeborn.SortBasedPusherSuiteJ, took 0.0 sec [error] at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) [error] ... ``` ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Pass GA Closes #1844 from cfmcgrady/celeborn-906-followup. Authored-by: Fu Chen <[email protected]> Signed-off-by: Cheng Pan <[email protected]>

cfmcgrady added 2 commits August 24, 2023 10:42

align deps

7c4be37

exclude -> excludeAll

9e3467d

cfmcgrady changed the title ~~[CELEBORN-906] Aligning dependencies between SBT and Maven~~ [CELEBORN-906][BUILD] Aligning dependencies between SBT and Maven Aug 24, 2023

pan3793 reviewed Aug 24, 2023

View reviewed changes

cfmcgrady added 4 commits August 24, 2023 14:04

force instead exclude

32f7ce0

Revert "force instead exclude"

53bb6b1

This reverts commit 32f7ce0.

address commment

7d594d3

dependencyOverrides

269b9ce

cfmcgrady force-pushed the align-deps-2 branch from 23a8396 to 269b9ce Compare August 25, 2023 10:45

pan3793 approved these changes Aug 25, 2023

View reviewed changes

waitinfuture approved these changes Aug 26, 2023

View reviewed changes

waitinfuture closed this in 6d7c5c0 Aug 26, 2023

cfmcgrady mentioned this pull request Aug 28, 2023

[CELEBORN-906][FOLLOWUP] Removal of redundant dependency log4j-slf4j2-impl from SBT profile spark-3.4 #1844

Closed

cfmcgrady deleted the align-deps-2 branch August 28, 2023 03:08

cfmcgrady mentioned this pull request Aug 28, 2023

[CELEBORN-861][BUILD] exclude netty-handler-ssl-ocsp for sbt #1777

Closed

cfmcgrady mentioned this pull request Oct 10, 2023

[CELEBORN-999] MR deps check #1928

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CELEBORN-906][BUILD] Aligning dependencies between SBT and Maven #1831

[CELEBORN-906][BUILD] Aligning dependencies between SBT and Maven #1831

cfmcgrady commented Aug 24, 2023

codecov bot commented Aug 24, 2023 •

edited

Loading

pan3793 Aug 24, 2023

cfmcgrady Aug 24, 2023

cfmcgrady Aug 24, 2023 •

edited

Loading

cfmcgrady Aug 24, 2023

pan3793 Aug 24, 2023

pan3793 Aug 25, 2023

cfmcgrady Aug 25, 2023

cfmcgrady Aug 25, 2023

pan3793 Aug 25, 2023

pan3793 Aug 25, 2023

cfmcgrady commented Aug 24, 2023

pan3793 Aug 24, 2023

waitinfuture left a comment

[CELEBORN-906][BUILD] Aligning dependencies between SBT and Maven #1831

[CELEBORN-906][BUILD] Aligning dependencies between SBT and Maven #1831

Conversation

cfmcgrady commented Aug 24, 2023

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

codecov bot commented Aug 24, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfmcgrady Aug 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfmcgrady commented Aug 24, 2023

Choose a reason for hiding this comment

waitinfuture left a comment

Choose a reason for hiding this comment

codecov bot commented Aug 24, 2023 •

edited

Loading

cfmcgrady Aug 24, 2023 •

edited

Loading