jitsi / ice4j

A Java implementation of the ICE protocol
Apache License 2.0
437 stars 233 forks source link

Could org.jitsi:ice4j:3.0-SNAPSHOT drop off redundant dependencies? #258

Open Celebrate-future opened 2 years ago

Celebrate-future commented 2 years ago

Hi! I found the pom file of project org.jitsi:ice4j:3.0-SNAPSHOT introduced 68 dependencies. However, among them, 10 libraries (14%) are not used by your project. I list the redundant dependencies below (labelled as red ones in the figure):

Redundant dependencies

com.google.guava:listenablefuture:jar:9999.0-empty-to-avoid-conflict-with-guava:compile com.google.j2objc:j2objc-annotations:jar:1.3:compile io.mockk:mockk-dsl:jar:1.10.0:test net.java.dev.jna:jna:jar:5.9.0:compile com.google.errorprone:error_prone_annotations:jar:2.7.1:compile com.google.code.findbugs:jsr305:jar:3.0.2:compile org.apiguardian:apiguardian-api:jar:1.1.2:test org.junit.platform:junit-platform-suite-api:jar:1.6.2:test org.jetbrains.kotlin:kotlin-stdlib-common:jar:1.5.31:compile io.mockk:mockk-common:jar:1.10.0:test

Outdated dependencies

org.junit.platform:junit-platform-suite-api:1.6.2 (1206 days without maintenance) com.google.j2objc:j2objc-annotations:1.3 (2383 days without maintenance)


Removing the redundant dependencies can reduce the size of project and prevent potential dependency conflict issues (i.e., multiple versions of the same library). More importantly, one of the redundant dependencies org.junit.platform:junit-platform-suite-api:jar:1.6.2:test incorporates an incompatible license ECLIPSE PUBLIC LICENSE V2.0 (ECLIPSE PUBLIC LICENSE V2.0 cannot be used by the project with license Apache-2.0). 2 of the redundant dependencies io.mockk:mockk-dsl:jar:1.10.0:test, org.apiguardian:apiguardian-api:jar:1.1.2:test induced dependency conflict in the dependency graph. As such, I suggest a refactoring operation for org.jitsi:ice4j:3.0-SNAPSHOT’s pom file.

The attached PR helps resolve the reported problem. It is safe to remove the unused libraries (we considered Java reflection relations when analyzing the dependencies). These changes have passed org.jitsi:ice4j:3.0-SNAPSHOT’s maven tests.

Best regards

jitsi-jenkins commented 2 years ago

Hi, thanks for your contribution! If you haven't already done so, could you please make sure you sign our CLA (https://jitsi.org/icla for individuals and https://jitsi.org/ccla for corporations)? We would unfortunately be unable to merge your patch unless we have that piece :(.

Celebrate-future commented 2 years ago

Hi, thanks for your contribution! If you haven't already done so, could you please make sure you sign our CLA (https://jitsi.org/icla for individuals and https://jitsi.org/ccla for corporations)? We would unfortunately be unable to merge your patch unless we have that piece :(.

Thank you, I have already finished it.

bgrozev commented 2 years ago

Hi, @Celebrate-future,

Thanks for your contribution! I confirm the CLA has been received.

Did you use an automated tool to find the redundant dependencies? I see a lot of them come from jitsi-utils. Do you think they could be removed there instead?

bgrozev commented 2 years ago

Jenkins please test this

codecov[bot] commented 2 years ago

Codecov Report

Merging #258 (e34aa08) into master (1f97cf2) will increase coverage by 0.00%. The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #258   +/-   ##
=========================================
  Coverage     39.00%   39.01%           
+ Complexity     1376     1375    -1     
=========================================
  Files           178      178           
  Lines         12176    12176           
  Branches       1842     1842           
=========================================
+ Hits           4749     4750    +1     
+ Misses         6773     6770    -3     
- Partials        654      656    +2     
Impacted Files Coverage Δ
src/main/java/org/ice4j/util/PeriodicRunnable.java 74.24% <0.00%> (-1.52%) :arrow_down:
src/main/java/org/ice4j/stack/StunStack.java 53.68% <0.00%> (-0.24%) :arrow_down:
src/main/java/org/ice4j/ice/Agent.java 55.11% <0.00%> (-0.16%) :arrow_down:
...c/main/java/org/ice4j/pseudotcp/PseudoTCPBase.java 64.00% <0.00%> (-0.16%) :arrow_down:
...rc/main/java/org/ice4j/stack/NetAccessManager.java 60.95% <0.00%> (ø)
src/main/java/org/ice4j/message/Message.java 50.55% <0.00%> (+0.36%) :arrow_up:
...in/java/org/ice4j/stack/MessageProcessingTask.java 70.00% <0.00%> (+8.00%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 1f97cf2...e34aa08. Read the comment docs.

JonathanLennox commented 2 years ago

Some of these are only "test"-scope dependencies, and as such shouldn't introduce any issues of version conflicts, project size, or license issues for consumers of the library. (mvn dependency:tree -Dscope=compile will show you only non-test-scope dependencies.)

The guava dependency is used by the org.jitsi.utils.logging2 package, which ice4j does use - the fact that the tests pass without using it might mean that the unit tests aren't giving the coverage we should have, but I don't think it's safe to remove it. Some others are recursive dependencies of guava.

kotlin-stdlib is similarly used by logging2.

The remaining one is JNA. This is used by jitsi-utils, but not by a portion of jitsi-utils that ice4j uses. As such it can probably be safely excluded.

bgrozev commented 2 years ago

We removed the guava dependency from jitsi-utils

Celebrate-future commented 2 years ago

We removed the guava dependency from jitsi-utils

OK,thanks for your attention.