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

Migrate to ktlint 0.46.0 #593

Closed
wants to merge 11 commits into from

Conversation

scana
Copy link

@scana scana commented Jun 29, 2022

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 #589.
Opening this as a draft, as there are few things to be resolved.

Notes/questions/problems:

  • 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?
  • 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?

- migrated `ReporterProvider` usage to `ReporterProvider<*>` - using wildcards as exact type is not required for plugin's use cases
- Ktlint.Params class is no longer available, removed it in favor of Ktlint.ExperimentalParams
- afaik git is case-insensitive, so you need to run two commits in order to change main->Main (error reported by new version of ktlint)
-- ktlint suggests that this is no longer necessary - is it?
   > UserData contains properties [android]. However, userData is deprecated and will be removed in a future version. Please create an issue that shows how this field is actively used.
@AleksanderBrzozowski
Copy link
Contributor

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!

@JLLeitschuh
Copy link
Owner

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

@scana
Copy link
Author

scana commented Jun 30, 2022

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?

@AleksanderBrzozowski
Copy link
Contributor

@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)

@JLLeitschuh
Copy link
Owner

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

@scana
Copy link
Author

scana commented Jul 1, 2022

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)

@AleksanderBrzozowski
Copy link
Contributor

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

@fthouraud fthouraud mentioned this pull request Jul 2, 2022
scana added 4 commits July 7, 2022 13:41
-- with lowest supported version 0.46.1, those tests do not longer bring value to us
-- added missing google repository (Android Gradle Plugin could not be found issue)
-- fixed file names (ktlint started to complain about them not being pascal case)
-- indentations changed from tabs to spaces
-- some error messages changes (phrasing mostly)
@scana
Copy link
Author

scana commented Jul 7, 2022

Added some more changes, some of them taken directly from @fthouraud's PR (#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).

@JLLeitschuh
Copy link
Owner

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

@scana
Copy link
Author

scana commented Aug 23, 2022

@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 scana closed this Aug 23, 2022
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.

The plugin does not work when using Ktlint 0.46.0
3 participants