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

Ensure Kotlin dependencies are properly handled when not forced #825

Merged

Conversation

tresat
Copy link
Contributor

@tresat tresat commented Jan 22, 2024

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

@ben-manes
Copy link
Owner

odd, the test suite breaks with this change

@tresat
Copy link
Contributor Author

tresat commented Jan 22, 2024

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

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

ben-manes commented Jan 22, 2024

yep, jdk11 worked locally for master

val latest = configuration.allDependencies
.filterIsInstance<ExternalDependency>()
.filterNot(kotlinDeps)
.filter { configuration.hierarchy.size > 1 && kotlinDeps(it) }
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe filterNot? trying locally

Copy link
Owner

@ben-manes ben-manes Jan 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

passes except for your new test, which I think might be a formatting difference?

PluginUpdateDetectionSpec > kotlin plugin in the implementation configuration is properly handled FAILED
    Condition not satisfied:

    result.output.contains """The following dependencies have later milestone versions: - org.jetbrains.kotlin:kotlin-gradle-plugin [$KOTLIN_VERSION -> """
    |      |      |                                                                                                                   |
    |      |      false                                                                                                               1.6.0
    |
    |      > Task :dependencyUpdates
    |
    |      ------------------------------------------------------------
    |      : Project Dependency Updates (report to plain text file)
    |      ------------------------------------------------------------
    |
    |      The following dependencies are using the latest milestone version:
    |       - org.jetbrains.kotlin:kotlin-gradle-plugin:1.6.0
    |       - org.jetbrains.kotlin:kotlin-stdlib:1.6.0

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think detecting by isForced might be what's needed.

This will handle the case of detecting Kotlin plugins properly.
@tresat tresat force-pushed the tt/dont-exclude-kolin-if-no-parent-confs branch from 658d6c2 to 7e792e9 Compare January 22, 2024 16:59
@ben-manes ben-manes merged commit 0763cb4 into ben-manes:master Jan 22, 2024
2 checks passed
@tresat
Copy link
Contributor Author

tresat commented Jan 22, 2024

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

Here is the commit that introduced it, 0a290fb, which seems to be trying to fix #424.

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

@ben-manes
Copy link
Owner

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

@tresat tresat changed the title Ensure Kotlin dependencies are properly handled when present in a configuration with no parents Ensure Kotlin dependencies are properly handled when not forced Jan 22, 2024
@tresat
Copy link
Contributor Author

tresat commented Jan 23, 2024

@ben-manes Can you take a look at #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
Copy link
Owner

that's awesome, thank you @tresat!

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.

2 participants