ktlint-gradle icon indicating copy to clipboard operation
ktlint-gradle copied to clipboard

Migrate to ktlint 0.46.0

Open scana opened this issue 3 years ago • 8 comments

Important/breaking changes:

  • Updated Kotlin plugin to 1.6.21 - required as newer ktlint is compiled with Kotlin 1.7.0 and the build fails due to it being incompatible with 1.5.21
  • Updated minimum support version of ktlint to 0.46.1 - this was unavoidable because ktlint has introduce an API breaking change in ReporterProvider interface

Fixes https://github.com/JLLeitschuh/ktlint-gradle/issues/589. Opening this as a draft, as there are few things to be resolved.

Notes/questions/problems:

  • [x] I am unable to run tests, as ./plugin/gradlew pluginUnderTestMetadata returns:
Task 'pluginUnderTestMetadata' not found in root project 'ktlint-gradle-samples'.

At the same time, running tests from IDE result in:

Resolving dependency configuration 'compileOnly' is not allowed as it is defined as 'canBeResolved=false'.
Instead, a resolvable ('canBeResolved=true') dependency configuration that extends 'compileOnly' should be resolved.
  • [ ] Kotlin plugin update is required, but can easily happen in a separate PR, please let me know if you'd like me to open it
  • [ ] new version of ktlint complains about main.kt files not being PascalCase. This is problematic because git is case-insensitive and if all of those changes get squashed, the filename won't change on other repo clones. This could be perhaps resolved by just disabling this particular rule for every sample. Or do a rename in separate PR and instead of squashing, just rebase two commits. Wdyt?
  • [x] I removed userData parameter from Ktlint.Params/ExperimentalParams as ktlint would complain that this field is deprecated and will be removed. Is it needed for any particular reason?

scana avatar Jun 29 '22 19:06 scana

There is a bug in documentation that should be fixed :) Instead of

./plugin/gradlew pluginUnderTestMetadata

try

./plugin/gradlew -p ./plugin pluginUnderTestMetadata

than, you should be able to run the tests:

./plugin/gradlew -p ./plugin check

😃

new version of ktlint complains about main.kt files not being PascalCase. Do you mean github will squash commits during merge, thus removing the file name change?

Thanks for the contribution, I am not the maintainer of this repo, but I actively use it at work!

AleksanderBrzozowski avatar Jun 29 '22 20:06 AleksanderBrzozowski

Is there any way to write this change so that it is backwards compatible via reflection?

If it is not, then there needs to be an exception thrown somewhere if someone attempts to use an older, incompatible, version of ktlint with this plugin

JLLeitschuh avatar Jun 29 '22 22:06 JLLeitschuh

Is there any way to write this change so that it is backwards compatible via reflection?

@JLLeitschuh If the only breaking change was removal of Params class then sure, it would work. However, with change of ReporterProvider interface I think it is not feasible. I will add an exception.

Do you mean github will squash commits during merge, thus removing the file name change?

@AleksanderBrzozowski not really, the name change will still be registered in Git history, but if you have this project cloned on your local machine and you make a pull, casing will not change (it will remain main.kt).
This is at least what I used to struggle with back in the past and I assume is still the same.

I tried this: ./plugin/gradlew -p ./plugin pluginUnderTestMetadata and it still does not work. Any chance you could make a clean clone of this repo and try on your side?

scana avatar Jun 30 '22 09:06 scana

@scana I cloned the repo, invoked the command and everything worked correctly - build passed.

java --version
openjdk 11.0.7 2020-04-14
OpenJDK Runtime Environment AdoptOpenJDK (build 11.0.7+10)
OpenJDK 64-Bit Server VM AdoptOpenJDK (build 11.0.7+10, mixed mode)

AleksanderBrzozowski avatar Jun 30 '22 09:06 AleksanderBrzozowski

However, with change of ReporterProvider interface I think it is not feasible. I will add an exception.

I think the addition of generics is not an API breaking chance because generics are erased by the compiler.

I'd try doing the reflection thing and leave the generics as you have them and see if you can support backwards compatibility. If you can't, oh well.. but it would be worth trying

JLLeitschuh avatar Jun 30 '22 13:06 JLLeitschuh

Small update - still working on this as I managed to get tests to run, but at least half of them fail with:

Could not initialize class org.jetbrains.kotlin.gradle.plugin.KotlinGradleBuildServices

java.lang.NoClassDefFoundError: Could not initialize class org.jetbrains.kotlin.gradle.plugin.KotlinGradleBuildServices
	at org.jetbrains.kotlin.gradle.plugin.KotlinBasePluginWrapper.apply(KotlinPluginWrapper.kt:92)
	at org.jetbrains.kotlin.gradle.plugin.KotlinBasePluginWrapper.apply(KotlinPluginWrapper.kt:54)

scana avatar Jul 01 '22 10:07 scana

@scana Please provide more details about system / java version that you use ;)

AleksanderBrzozowski avatar Jul 01 '22 11:07 AleksanderBrzozowski

Added some more changes, some of them taken directly from @fthouraud's PR (https://github.com/JLLeitschuh/ktlint-gradle/pull/595). UserData has been replaced with EditorConfigOverride.

Had to remove some of the test cases as they are failing before plugin can even start, due to missing Gradle-related classes (ie. it's not so easy to throw an exception).

The following test cases still fail, but I am not sure how long it make take me to resolve them.

[KtlintPluginTest](https://github.com/JLLeitschuh/ktlint-gradle/pull/classes/org.jlleitschuh.gradle.ktlint.KtlintPluginTest.html). [Gradle Gradle 7.1.1: Should enable experimental indentation rule](https://github.com/JLLeitschuh/ktlint-gradle/pull/classes/org.jlleitschuh.gradle.ktlint.KtlintPluginTest.html#enableExperimentalIndentationRule(GradleVersion)[1])
[KtlintPluginTest](https://github.com/JLLeitschuh/ktlint-gradle/pull/classes/org.jlleitschuh.gradle.ktlint.KtlintPluginTest.html). [Gradle Gradle 7.1.1: Should apply internal git filter to check task](https://github.com/JLLeitschuh/ktlint-gradle/pull/classes/org.jlleitschuh.gradle.ktlint.KtlintPluginTest.html#gitFilterOnCheck(GradleVersion)[1])
[UnsupportedGradleTest](https://github.com/JLLeitschuh/ktlint-gradle/pull/classes/org.jlleitschuh.gradle.ktlint.UnsupportedGradleTest.html). [Should raise exception on applying plugin for build using unsupported Gradle version.](https://github.com/JLLeitschuh/ktlint-gradle/pull/classes/org.jlleitschuh.gradle.ktlint.UnsupportedGradleTest.html#errorOnOldGradleVersion$ktlint_gradle())

@AleksanderBrzozowski I am using the same java version as you are + MacOS M1. I also can build and run master branch. It all started to fail as soon as had to bump Kotlin to 1.6.21 (as ktlint requires it).

scana avatar Jul 07 '22 11:07 scana

I believe that this is now outdated by #597, correct?

JLLeitschuh avatar Aug 23 '22 15:08 JLLeitschuh

@JLLeitschuh yeah, #597 covers some of the changes here - let me close this one. I think that migration to 0.46.0 will still require a breaking changes, though (I'll give it another try if I find enough free time)

scana avatar Aug 23 '22 16:08 scana