Migrate plugin to use KtLint directly instead invoking it via CLI. #424
Conversation
This file has new package suffix - "worker".
So later it could be restored from file and consumed on running KtLint.
Now it directly runs KtLint rather then invoking it via CLI.
This laoded reporters are serialized into file for later consume.
And wire them up to use LoadReportersTask and LoadRuleSetsTasks.
Save loaded reporter providers into one file and save into another file loaded reporter providers ids, file extensions and optional addtional reporter options.
This task will also fail the whole build in case of any non-corrected lint issue.
Task themselves and in tests.
plugin/src/main/kotlin/org/jlleitschuh/gradle/ktlint/PluginUtil.kt
Outdated
Show resolved
Hide resolved
plugin/src/main/kotlin/org/jlleitschuh/gradle/ktlint/worker/ConsoleReportWorkAction.kt
Outdated
Show resolved
Hide resolved
| disabledRules: Set<String> | ||
| ): Set<RuleSet> = loadRuleSetsFromClasspath() | ||
| .filterKeys { enableExperimental || it != "experimental" } | ||
| .filterKeys { !(disabledRules.contains("standard") && it == "\u0000standard") } |
There was a problem hiding this comment.
Why would we get a string starting with the null character here?
There was a problem hiding this comment.
It is copy-paste from KtLint CLI: https://github.com/pinterest/ktlint/blob/aa03a6b6b74cf71d8c2ede0c0a5e2305ce2e4327/ktlint/src/main/kotlin/com/pinterest/ktlint/internal/RuleSetsLoader.kt#L24
Initially it was introduced by Shyiko to enforce "standard" ruleset to be always the first.
| .associateBy { | ||
| val key = it.get().id | ||
| // Adapted from KtLint CLI module | ||
| if (key == "standard") "\u0000$key" else key |
There was a problem hiding this comment.
Why is the null character present here?
There was a problem hiding this comment.
Same as in the previous comment
plugin/src/main/kotlin/org/jlleitschuh/gradle/ktlint/worker/LintErrorResult.kt
Show resolved
Hide resolved
plugin/src/main/kotlin/org/jlleitschuh/gradle/ktlint/tasks/LoadReportersTask.kt
Outdated
Show resolved
Hide resolved
plugin/src/main/kotlin/org/jlleitschuh/gradle/ktlint/tasks/BaseKtLintCheckTask.kt
Show resolved
Hide resolved
plugin/src/main/kotlin/org/jlleitschuh/gradle/ktlint/tasks/GenerateReportsTask.kt
Show resolved
Hide resolved
plugin/src/main/kotlin/org/jlleitschuh/gradle/ktlint/tasks/LoadReportersTask.kt
Show resolved
Hide resolved
plugin/src/main/kotlin/org/jlleitschuh/gradle/ktlint/tasks/LoadReportersTask.kt
Outdated
Show resolved
Hide resolved
This will be properly implemented in followup releases.
To unify approach with other tasks.
|
I see that it's moving which is a good news. @Tapchicoma any idea when it can be merged / released? |
|
@pbielicki haven't had time before, but has a plan to address review comments till Friday. |
…guration. This should reduce probability of strange errors, because 3rd party ruleset or reporter are using newer version of shared with KtLint dependencies.
|
If time is the bottleneck for these last bits, let me know. I could carve out some time next week |
Please check c899d63 commit.
From my tests if worker in linting task fails the build, finalizer task is not called (from documentation I would expected otherwise). I've asked related question in Gradle slack https://gradle-community.slack.com/archives/CA745PZHN/p1611928971081900, but nobody answered if it is expected behaviour 🤷♂️ TLDR: I would propose to leave as it is currently. |
| private fun Configuration.ensureConsistencyWith( | ||
| target: Project, | ||
| otherConfiguration: Configuration | ||
| ) { | ||
| if (GradleVersion.version(target.gradle.gradleVersion) >= GradleVersion.version("6.8")) { | ||
| shouldResolveConsistentlyWith(otherConfiguration) | ||
| } else { | ||
| // Inspired by | ||
| // https://android.googlesource.com/platform/tools/base/+/refs/heads/mirror-goog-studio-master-dev/build-system/gradle-core/src/main/java/com/android/build/gradle/internal/dependency/ConstraintHandler.kt | ||
| incoming.beforeResolve { | ||
| val configName = it.name | ||
| otherConfiguration.incoming.resolutionResult.allDependencies { dependency -> | ||
| if (dependency is ResolvedDependencyResult) { | ||
| val id = dependency.selected.id | ||
| if (id is ModuleComponentIdentifier) { | ||
| // using a repository with a flatDir to stock local AARs will result in an | ||
| // external module dependency with no version. | ||
| if (!id.version.isNullOrEmpty()) { | ||
| if (id.module != "listenablefuture" || | ||
| id.group != "com.google.guava" || | ||
| id.version != "1.0" | ||
| ) { | ||
| target.dependencies.constraints.add( | ||
| configName, | ||
| "${id.group}:${id.module}:${id.version}" | ||
| ) { constraint -> | ||
| constraint.because("${otherConfiguration.name} uses version ${id.version}") | ||
| constraint.version { versionConstraint -> | ||
| versionConstraint.strictly(id.version) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } else if (id is ProjectComponentIdentifier && | ||
| id.build.isCurrentBuild && | ||
| dependency.requested is ModuleComponentSelector | ||
| ) { | ||
| // Requested external library has been replaced with the project dependency, so | ||
| // add the project dependency to the target configuration, so it can be chosen | ||
| // instead of the external library as well. | ||
| // We should avoid doing this for composite builds, so we check if the selected | ||
| // project is from the current build. | ||
| target.dependencies.add( | ||
| configName, | ||
| target.dependencies.project(mapOf("path" to id.projectPath)) | ||
| ) | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Do we have some kind of test verifying that this does what we expect it to do?
There was a problem hiding this comment.
Yes, check this test
By running on lowest supported and current one Gradle versions - both branches are been verified.
plugin/src/test/kotlin/org/jlleitschuh/gradle/ktlint/worker/SerializableReporterProviderTest.kt
Show resolved
Hide resolved
|
For those who is interested - I will try to push new release tomorrow. |
Main motivation of this huge refactoring is to migrate plugin to use directly
KtLintclass instead of doingjavaexecfor CLI.Several reasons are behind this:
org.gradle.parallelflag is enabled.This PR introduces new tasks and task hierarchy, plus uses workers everywhere.
WorkActions are running in isolated classloader mode, allowing to still support seamless KtLint version changes without updating plugin itself (unless KtLint will have some breaking changes).Following task structure is created:
LoadRuleSetsTask- preloadsRuleSetProviders and serialize them into intermediate file. Such approach allows to skip this step on incremental run and start doing linting directly.LoadReportersTask- preloadsReporterProviderand based on enabled and custom reporters serialize two files - one with only enabledReporterProviders, second with some useful info forGenerateReportsTask. Again such approach allows to skip this step on incremental run and start doing linting directly.BaseKtlintCheckTask* runs as previously actual linting. Currently only oneWorkActionis submitted for all files inSourceSet.GenerateReportsTask- generates actual reports and writes errors into Gradle console. This task will fail the build if errors were not autocorrected and present. Each report generation is running in ownWorkAction, plus console output is additionalWorkAction.Added a tests to verify supported KtLint versions are working correctly and they are besides two:
0.37.0on Windows crashes. This was fixed in0.37.1patch release.0.38.0on all OS crashes, because it was build with Kotlin api 1.4 level that is not supported yet by Gradle. Issue was fixed in0.38.1patch release.Did some testing on average project (312 modules, 4150 Kotlin files, 240_000 lines of code):
Single
SourceSetcheck runtime is slightly increased now. For example, for module with 232 Kotlin files it is increased from 10.1s to 17.4s. This single module performance issue could be addressed by splitting files into buckets in followup PR.Changes itself fix #395 and #229 issues.