-
-
Notifications
You must be signed in to change notification settings - Fork 778
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
Set strict dependency on tested Kotlin compiler version #4822
Conversation
This effectively pins the Kotlin compiler version used by detekt-cli, minimising the chance of it being overridden and causing issues such as detekt#4786
Worth commenting on the error case when someone tries to upgrade - if detekt falls behind in upgrading, and someone tries upgrading the Kotlin version in their project using configuration overrides to a newer version than what's used by detekt, it will cause a build failure. At least the error is more meaningful and accurate, but it will inevitably lead to new support issues being raised. The shadow/bundled approach would be more robust. |
@@ -16,6 +16,11 @@ dependencies { | |||
implementation(libs.jcommander) | |||
implementation(projects.detektTooling) | |||
implementation(projects.detektParser) | |||
implementation(libs.kotlin.compilerEmbeddable) { | |||
version { | |||
strictly(libs.versions.kotlin.get()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Q: Is the issue arising only on .compilerEmbeddable
or should be pin also the stdlib and other deps we import?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a good question. Probably yes... but perhaps we can just roll out this change to begin with, and see if there are any new issues, before becoming more strict with the other dependencies?
kotlin-compiler-embeddable
has a few other dependencies too, and in the very worst case they might also be overridden by the user's build script, so we'd want to pin everything, but then there's a maintenance burden being introduced (we have to remember to check if there are changes to the transitive dependencies that we need to update when each new Kotlin version is released).
I'm still of the opinion that we shouldn't be building workarounds into this project just to workaround issues introduced by users in their own build scripts, and instead we should educate users about how to fix it on their side. For that reason, if we merge this PR, I'd prefer the limited scope that it currently has.
Will this change affect the kotlin version in the "main project" of our users? I assume that if they need to use |
There should be no impact at all to the main project. The dependency on detekt-cli is added to the "detekt" configuration and I believe the constraints only apply to that configuration and no others. If this is merged the snapshot could be used to test. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think one review comment is still open.
#4822 (comment)
Otherwise, we should merge this into main asap.
This effectively pins the Kotlin compiler version used by detekt-cli, minimising the chance of it being overridden and causing issues such as #4786. The only alternative I can think of to this approach is using the shadowed/bundled JAR in the Gradle plugin, and only publishing that artifact of detekt-cli.
On the consumer side, the version can still be overridden, but only in specific configurations which a user would have to specifically go out of their way to use (and, given the only override on the consumer side requires use of a
strictly
version declaration, it should be respected).On the consumer side, the version can be overridden with strictly applied versions (when this PR was authored, detekt-cli strictly depends on version 1.6.21):
When overriding the version using non-strictly applied versions, the version is unchanged, though upgrade attempts lead to this error:
There's more background here and here.
Closes #4817.