Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Celebrate-future
Copy link

@Celebrate-future Celebrate-future commented Apr 30, 2022

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
Copy link

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
Copy link
Author

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
Copy link
Member

bgrozev commented May 2, 2022

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
Copy link
Member

bgrozev commented May 2, 2022

Jenkins please test this

@codecov
Copy link

codecov bot commented May 2, 2022

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%) ⬇️
src/main/java/org/ice4j/stack/StunStack.java 53.68% <0.00%> (-0.24%) ⬇️
src/main/java/org/ice4j/ice/Agent.java 55.11% <0.00%> (-0.16%) ⬇️
...c/main/java/org/ice4j/pseudotcp/PseudoTCPBase.java 64.00% <0.00%> (-0.16%) ⬇️
...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%) ⬆️
...in/java/org/ice4j/stack/MessageProcessingTask.java 70.00% <0.00%> (+8.00%) ⬆️

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
Copy link
Member

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
Copy link
Member

bgrozev commented May 2, 2022

We removed the guava dependency from jitsi-utils

@Celebrate-future
Copy link
Author

We removed the guava dependency from jitsi-utils

OK,thanks for your attention.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants