search

ben-manes / gradle-versions-plugin

Gradle plugin to discover dependency updates
Apache License 2.0
3.82k stars 199 forks source link

Ensure Kotlin dependencies are properly handled when not forced #825

Closed tresat closed 5 months ago

tresat commented 5 months ago

This will handle the case of Kotlin plugins (applied to the parentless classpath configuration).

ben-manes commented 5 months ago

odd, the test suite breaks with this change

tresat commented 5 months ago

You're right, this breaks additional tests.

I was getting failures in DifferentGradleVersionsSpec when I checked out master and ran build for the first time, so I assumed they weren't related. Then I didn't run the full test suite after that, since it would always fail.

I'll take another look.

ben-manes commented 5 months ago

iirc, that happens when you run with a newer jdk. You can use jenv, asdf, etc. to set a local one for the project. Trying right now with jdk11 on master to see if it passes locally. I don't recall what the issue was, but remember being confused by that test failure too

ben-manes commented 5 months ago

yep, jdk11 worked locally for master

tresat commented 5 months ago

A little more context: I saw the comments about plugins trying to force versions, so that led me to this path. My approach to fixing this has been to preserve the special case logic for inheritedKotlin by making the smallest change that fixes these new test cases and leaving that alone.

But after talking with @jvandort, it seems isForce isn't doing anything in newer versions of Gradle, and there is no way remaining for external deps to be marked forced. So the effect of this change is to just always allow Kotlin deps to be included in that first loop that collects them into latest.

So I'm wondering if any of the special casing for Kotlin is still necessary? If I remove it all, I get the same passing build. Perhaps it is no longer useful? I couldn't find any existing tests that hinted at why it was still there. Do you know how we could create one to confirm if it is needed?

ben-manes commented 5 months ago

Here is the commit that introduced it, https://github.com/ben-manes/gradle-versions-plugin/commit/0a290fb86d8ba3962fead9d3c2be8fa91f1ab5e5, which seems to be trying to fix #424.

If we don't need it anymore that is better. wdyt?

ben-manes commented 5 months ago

I released to unblock, but happy to revisit the removal of that code if its useless nowadays.

tresat commented 5 months ago

@ben-manes Can you take a look at https://github.com/ben-manes/gradle-versions-plugin/pull/826 when you get a chance? I think this is all that is needed. I added tests for the kotlin stdlib dependency that was the origin of the special case in the Resolver.

ben-manes commented 5 months ago

that's awesome, thank you @tresat!