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

Stop overriding project version when publishing #6014

Closed
wants to merge 1 commit into from

Conversation

3flex
Copy link
Member

@3flex 3flex commented Apr 18, 2023

detekt-compiler-plugin has a unique versioning scheme which should not be overridden when publishing.

The most recent release was set to version 1.23.0-RC1 when it should have been 1.8.20-1.23.0-RC1.

detekt-compiler-plugin has a unique versioning scheme which should not be
overridden when publishing.
@3flex 3flex requested a review from cortinico April 18, 2023 12:32
@github-actions github-actions bot added the build label Apr 18, 2023
@3flex 3flex added the housekeeping Marker for housekeeping tasks and refactorings label Apr 18, 2023
@codecov
Copy link

codecov bot commented Apr 18, 2023

Codecov Report

Merging #6014 (6af4c4c) into main (bd44c2d) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##               main    #6014   +/-   ##
=========================================
  Coverage     84.66%   84.66%           
  Complexity     3840     3840           
=========================================
  Files           549      549           
  Lines         13071    13071           
  Branches       2305     2305           
=========================================
  Hits          11066    11066           
  Misses          868      868           
  Partials       1137     1137           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@cortinico
Copy link
Member

The most recent release was set to version 1.23.0-RC1 when it should have been 1.8.20-1.23.0-RC1.

Why is this the case?
Also I'm glad we found this out while we're still in RC.
Still I'm unsure if 1.8.20-1.23.0 will prevail on 1.23.0-RC1 as Gradle uses string comparison to compare versions IIRC 🤔

@3flex
Copy link
Member Author

3flex commented Apr 19, 2023

Why is this the case?

#5492 (comment) - it's trying to signify that the compiler plugin is only known to be compatible with specific Kotlin versions. I've used the same versioning scheme as KSP to avoid reinventing the wheel.

That said - this is only really relevant for those using the plugin with kotlinc, since the detekt Gradle plugin doesn't allow users to select the compiler plugin version to use at this time. So maybe this is not as important as I'd thought.

Still I'm unsure if 1.8.20-1.23.0 will prevail on 1.23.0-RC1 as Gradle uses string comparison to compare versions IIRC 🤔

Ugh... good point. Rules are here: https://docs.gradle.org/current/userguide/single_versions.html#version_ordering

It might need to be left as is for now, but add the Kotlin version for detekt 2? Switching the Kotlin & detekt versions so it's 1.23.0-1.8.20 won't work either since RC is considered newer than 1.8.20 if I read the link above correctly.

@cortinico
Copy link
Member

#5492 (comment) - it's trying to signify that the compiler plugin is only known to be compatible with specific Kotlin versions. I've used the same versioning scheme as KSP to avoid reinventing the wheel.

Got it 👍 I have to mention that I'm not a huge fan of this versioning notation, mostly because there is a 1:1 relationship between Detekt version and Kotlin version. So being extra explicit here is probably unnecessary and it creates confusion with the previous Detekt versions.

@3flex
Copy link
Member Author

3flex commented Apr 19, 2023

there is a 1:1 relationship between Detekt version and Kotlin version

Sort of: when running detekt CLI or Gradle plugin detekt brings its own version of the compiler in as a dependency. The project under analysis can use a different version of Kotlin and everything will work (as long as detekt classpath isn't overridden).

The compiler plugin uses the Kotlin compiler used by the project under analysis. So the Kotlin version that detekt was compiled with is much more relevant for the compiler plugin, and generally will have to match the Kotlin version the project under analysis uses, hence adding the Kotlin version to the artifact version. That's exactly why KSP does it that way.

@cortinico
Copy link
Member

and generally will have to match the Kotlin version the project under analysis uses, hence adding the Kotlin version to the artifact version. That's exactly why KSP does it that way.

Makes sense. Does this mean we'll have to release a minor every Kotlin minor/patch release?

@cortinico
Copy link
Member

Also how do we feel about: 1.23.0-RC1-1.8.20 instead?

@chao2zhang
Copy link
Member

Also how do we feel about: 1.23.0-RC1-1.8.20 instead?

KSP has been using the version scheme <kotlin_version>-<ksp_version> (https://github.com/google/ksp/releases/tag/1.8.20-1.0.11)
Other compiler plugins have been using just their <plugin_version>.

I think 1.8.20-1.23.0-RC1 is reasonable given it is following the existing convention.

@3flex
Copy link
Member Author

3flex commented Apr 23, 2023

Makes sense. Does this mean we'll have to release a minor every Kotlin minor/patch release?

Not necessarily, it depends if there are incompatible changes, though it will be confusing for users if we adopt this versioning scheme and don't release new versions along with new Kotlin releases.

I think 1.8.20-1.23.0-RC1 is reasonable given it is following the existing convention.

As Nicola pointed out above this won't be possible now since Gradle will see that as an older version than 1.23.0-RC1 which has already been pushed.

We could switch artifact group to dev.detekt? That way the version history starts fresh.

Also note that as it stands the versioning will actually need to apply to both the new Gradle detekt compiler plugin as well as detekt-compiler-plugin since the Gradle plugin will try to add a specific, fixed version of detekt-compiler-plugin to the Kotlin compiler's plugin classpath.

I'm going to close this PR, I think we need to discuss the overall approach in a separate issue then we can set up the build properly, and then release RC2.

@3flex 3flex closed this Apr 23, 2023
@github-actions
Copy link

Warnings
⚠️ This PR is approved with no milestone set. If merged, it won't appear in the detekt release notes.

Generated by 🚫 dangerJS against 6af4c4c

@chao2zhang
Copy link
Member

We could switch artifact group to dev.detekt? That way the version history starts fresh.

I believe changing artifact group is one of the goals and niceties for detekt 2.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build housekeeping Marker for housekeeping tasks and refactorings
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants