fix: skip deep dependency comparison for Gradle projects

[jjohannes]

This is unnecessary and leads to a long runtime (and possible an infinite loop/recursion) for large dependency graphs as observed in #9763.

[codecov]

Codecov Report

Attention: Patch coverage is 20.00000% with 4 lines in your changes missing coverage. Please review.

Project coverage is 56.46%. Comparing base (a9ce79a) to head (8b36075).
Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
...el/src/main/kotlin/utils/DependencyGraphBuilder.kt	0.00%	2 Missing and 2 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #9817      +/-   ##
============================================
- Coverage     56.47%   56.46%   -0.02%     
  Complexity     1572     1572              
============================================
  Files           328      328              
  Lines         12381    12385       +4     
  Branches       1162     1163       +1     
============================================
+ Hits           6992     6993       +1     
- Misses         4926     4928       +2     
- Partials        463      464       +1     

Flag	Coverage Δ
funTest-docker	68.70% <ø> (ø)
funTest-non-docker	33.24% <0.00%> (-0.03%)	⬇️
test-ubuntu-24.04	39.23% <20.00%> (-0.01%)	⬇️
test-windows-2022	39.21% <20.00%> (-0.01%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[sschuberth]

There are some test differences to inspect.

[sschuberth]

There are some test differences to inspect.

For whatever reason, the change causes a lot of the following differences in the dependency tree:

It basically seems that cycles are now cut too early or so.

[sschuberth]

It basically seems that cycles are now cut too early or so.

Or actually, it seems that runtime dependencies are omitted now. Take a look at e.g. lifecycle-livedata-core:2.5.0:

core-common:2.1.0 and core-runtime:2.1.0 are exactly the omitted direct dependencies.

[sschuberth]

BTW, @oheger-bosch do you recall why the original code uses an "uni-directional" containsAll() here? In other words, why did we continue here if dependencies2 was a superset of dependencies1, instead of also returning false early?

[oheger-bosch]

I think this is because the two sets only contain identifier IDs, but it has to be checked whether the structure of the dependency sub trees is identical.

[sschuberth]

Hmm, ok, but that does not really answer my question why we can't / shouldn't do

if (dependencies1 != dependencies2.keys) return false

because if those sets of IDs are not equal, also their sub-trees cannot be equal.

[sschuberth]

Let's anyway extract this a bit unrelated change: #9940

[jjohannes]

@sschuberth in order to make this solution workable, the selected variants need to be tracked. The same component may have a different set of dependencies, depending on which variant was selected. In most cases, it is only one, but Gradle also supports to select multiple in one scope under certain conditions.

I added variants as an additional optional field of the Identifier data class to check if the tests pass with this. It likely works, but you have to check is such an addition is desired.

[jjohannes]

Wit the change in this PR, the problem described in the description of #9763 no longer occurs.

[mnonnenmacher]

I added variants as an additional optional field of the Identifier data class to check if the tests pass with this. It likely works, but you have to check is such an addition is desired.

Changing the Identifier class is indeed problematic as it is used in so many places, so even if no tests are failing there could be unintended side effects. We have discussed to switch to PURL (#8982) which should also make it easier to add such custom properties, but that's a huge refactoring.

However, I wonder if even you even use the new property anywhere in this change? I couldn't find it.

[jjohannes]

Changing the Identifier class is indeed problematic...

Yes I was afraid that it may not be a simple thing to do. Although, the change only adds a new field (variants) that defaults to emptySet(). Therefore, it should/would be non breaking. But I can understand, if you still see it as too risky.

However, I wonder if even you even use the new property anywhere in this change? I couldn't find it.

I is implicitly used by dependencies1 == dependencies2.keys as it adds a new value that is used when the data objects are compared.

[mnonnenmacher]

Changing the Identifier class is indeed problematic...

Yes I was afraid that it may not be a simple thing to do. Although, the change only adds a new field (variants) that defaults to emptySet(). Therefore, it should/would be non breaking. But I can understand, if you still see it as too risky.

Yes, I would prefer not change that class. But if the information is only required during dependency resolution and not in the output, it should be possible to store it separately.

@sschuberth I only commented on the identifier change because I haven't really looked into the issue itself, do you have comments on the remaining implementation?

[fviernau]

I haven't looked into any details here, but noticed the discussion.
I'm not sure how far you are with the current approach.

A smaller performance optimization alternative idea could be, to start from the original code and just introduce a lazy property for the hashcode, and perform deep comparison only if the hash code equals. Not sure how much that'd gain though.

[sschuberth]

@heliocastro, please try this PR (rebased to main) to see if it fixes your issues.

[heliocastro]

Local test results improved a lot with the current patch, for analyzer case. We we're able to finalize in 1/5 of the time now with less memory and no fault.
The only issue comes that we can't verify results due the fact that previously we couldn't get a proper result, so if analyzer is good enough with results is under judgement.