0.49 support #667

wakingrufus · 2023-04-27T17:20:26Z

convert ktlint invocations from reflection to new method of using multiple compileOnly targets

JLLeitschuh · 2023-04-28T00:30:39Z

Looking solid so far. I didn't realize how easy source sets were to work with these days. I would have created separate projects per version of Ktlint, but your solution is so much simpler. I love it!

Nice work! 🎉

…tiple compileOnly targets create common abstraction for Lint errors and reporters

JLLeitschuh

In general, looking good. I would suggest using ValidatingInputStream when deserializing java objects if possible

plugin/build.gradle.kts

plugin/src/adapter/kotlin/org/jlleitschuh/gradle/ktlint/reporter/GenericReporterProvider.kt

JLLeitschuh · 2023-05-18T02:29:46Z

plugin/src/adapter/kotlin/org/jlleitschuh/gradle/ktlint/worker/LintErrorResult.kt

@@ -10,9 +9,9 @@ import java.io.Serializable
 * @param lintedFile file that was checked by KtLint
 * @param lintErrors list of found errors and flag indicating if this error was corrected
 */
-internal data class LintErrorResult(


I presume this is because kotlin doesn't see this class as being a part of the same project? There is a way to configure the kotlin compiler "friend path" but I don't know if that API is exposed via Gradle. So this is reasonable.

But if you're curious, feel free to look into it.

Oh hey... Look what I found. A ticket I opened 5 years ago on this same topic: https://youtrack.jetbrains.com/issue/KT-20760

I was able to get friend paths working, but making this internal will also require KtLintInvocation to be internal, and then the work action can't invoke it without also being internal, and I assume those can't be internal

JLLeitschuh · 2023-05-18T02:31:48Z

...apter34/kotlin/org/jlleitschuh/gradle/ktlint/reporter/SerializableReportersProviderLoader.kt

+class SerializableReportersProviderLoader :
+    ReportersLoaderAdapter<Reporter, SerializableReporterProvider, Ktlint34Reporter, Ktlint34ReporterProvider> {
+    override fun loadAllReporterProviders(): List<ReporterProviderWrapper<SerializableReporterProvider>> = ServiceLoader
+        .load(ReporterProvider::class.java)
+        .toList().map {
+            ReporterProviderWrapper(it.id, SerializableReporterProvider(it))
+        }


Good use of service loaders IMHO!

I can't take credit, this code was already here :)

JLLeitschuh · 2023-05-18T02:38:58Z

plugin/src/adapter49/kotlin/org/jlleitschuh/gradle/ktlint/reporter/ReportersProviderV2Loader.kt

+            serializedReporterProviders.inputStream().buffered()
+        ).use {
+            @Suppress("UNCHECKED_CAST")
+            it.readObject() as List<ReporterProviderV2<ReporterV2>>


Sanity check. No way for this deserialization to come from untrusted user input? Arbitrary deserialization of untrusted java objects is a common source of remote code execution vulnerabilities, via deserialization gadget chains.

its deserializing from a file created by this plugin, which is a list of the reporters used.
if you were able to replace that file between the time the plugin writes and and when it is read, I suppose it could be malicious input. or if you modified the build to point loadedReporterProviders another location containing the bad file. This is always how this plugin has worked though (since i came to it). I think we could remove this serialization, but it would require doing the serviceloader call every time we need reporters which would impact performance.

There's also the validating deserializer that comes from apache Commons. I think we have a use of it elsewhere

JLLeitschuh · 2023-05-18T02:39:54Z

plugin/src/adapter49/kotlin/org/jlleitschuh/gradle/ktlint/reporter/ReportersProviderV2Loader.kt

+        return enabledReporters
+            .map { reporterType ->
+                val provider = enabledProviders.find { reporterType.reporterName == it.id }
+                    ?: throw RuntimeException(
+                        "KtLint plugin failed to load reporter ${reporterType.reporterName}."
+                    )
+
+                val options = if (reporterType == ReporterType.PLAIN_GROUP_BY_FILE) {
+                    reporterType.options.associateWith { "true" }
+                } else {
+                    emptyMap()
+                }


I feel like I've read this exact snippet a few times here. Any way to unify it into a common method?

Its the same logic, applied to different ReporterProvider types, and we do access properties on those types. We don't have duck typing so i'd have to wrap those classes and convert back and forth to commonize this code, and imho the duplication isn't bad enough to add that much complexity

Fair enough! I thought that may have been what was probably going on. Just wanted to check

JLLeitschuh · 2023-05-18T02:50:06Z

plugin/src/test/kotlin/org/jlleitschuh/gradle/ktlint/KtlintBaselineSupportTest.kt

@@ -135,6 +135,7 @@ class KtlintBaselineSupportTest : AbstractPluginTest() {
                buildGradle.appendText(
                    """
                    ktlint.version = "$ktLintVersion"
+                    ktlint.debug = true


Very sane, why we haven't done this before, I don't know 😂

hah I needed this to debug some issues. I could remove it, but I guess we might as well leave this here otherwise we will probably forget about that option.

JLLeitschuh · 2023-05-18T02:50:22Z

plugin/src/test/kotlin/org/jlleitschuh/gradle/ktlint/testdsl/TestAnnotations.kt

@@ -12,7 +12,7 @@ import kotlin.streams.asStream

 object TestVersions {
    const val minSupportedGradleVersion = KtlintBasePlugin.LOWEST_SUPPORTED_GRADLE_VERSION
-    const val maxSupportedGradleVersion = "8.0.1"
+    const val maxSupportedGradleVersion = "8.1.1"


JLLeitschuh · 2023-05-18T22:53:33Z

This is looking good. Anything else to make work?

JLLeitschuh

LGTM!

It may be a good sanity check to use the validating input stream when deserializing java objects though.

JLLeitschuh

One thing. Other than that. LGTM!

plugin/src/main/kotlin/org/jlleitschuh/gradle/ktlint/worker/KtLintClassesSerializer.kt

…tiple compileOnly targets create common abstraction for Lint errors and reporters

JLLeitschuh

LGTM!

wakingrufus force-pushed the ktlint-49 branch 2 times, most recently from 6adfe1c to 260474a Compare April 27, 2023 17:55

wakingrufus force-pushed the ktlint-49 branch from 260474a to c073cd5 Compare April 28, 2023 19:33

convert ktlint invocations from reflection to new method of using mul…

17eb806

…tiple compileOnly targets create common abstraction for Lint errors and reporters

wakingrufus force-pushed the ktlint-49 branch 3 times, most recently from f6467ca to f87b082 Compare May 16, 2023 21:45

wakingrufus marked this pull request as ready for review May 17, 2023 00:18

wakingrufus requested review from Tapchicoma and JLLeitschuh May 17, 2023 00:18

JLLeitschuh reviewed May 18, 2023

View reviewed changes

wakingrufus force-pushed the ktlint-49 branch from f87b082 to 3d9d2a1 Compare May 18, 2023 21:41

wakingrufus requested a review from JLLeitschuh May 18, 2023 22:03

JLLeitschuh approved these changes May 24, 2023

View reviewed changes

wakingrufus force-pushed the ktlint-49 branch 3 times, most recently from 430c08e to d8b4213 Compare May 31, 2023 00:09

JLLeitschuh approved these changes May 31, 2023

View reviewed changes

plugin/src/main/kotlin/org/jlleitschuh/gradle/ktlint/worker/KtLintClassesSerializer.kt Outdated Show resolved Hide resolved

LiberaTeMetuMortis mentioned this pull request May 31, 2023

KtLint plugin failed to load reporter plain with Ktlint 0.49.0 #665

Closed

wakingrufus force-pushed the ktlint-49 branch 2 times, most recently from bf55d0b to ca092e9 Compare June 1, 2023 18:39

convert ktlint invocations from reflection to new method of using mul…

ddf5484

…tiple compileOnly targets create common abstraction for Lint errors and reporters

wakingrufus force-pushed the ktlint-49 branch from ca092e9 to ddf5484 Compare June 1, 2023 21:08

JLLeitschuh approved these changes Jun 4, 2023

View reviewed changes

JLLeitschuh merged commit c54565c into main Jun 5, 2023

wakingrufus deleted the ktlint-49 branch June 21, 2023 17:03

aliabdoli77 mentioned this pull request Jun 26, 2023

Dala 1862 manual maintenance 2 sprint45 d-fine/Dataland#250

Merged

93 tasks

aliabdoli77 mentioned this pull request Jul 25, 2023

Dala 2042 manual maintenance sprint 47 d-fine/Dataland#263

Merged

93 tasks

marcelrasbachdfine mentioned this pull request Aug 21, 2023

Manual maintenance 2 sprint 49 d-fine/Dataland#284

Merged

94 tasks

aliabdoli77 mentioned this pull request Oct 17, 2023

Dala 2589 manual maintenance 53 d-fine/Dataland#322

Merged

95 tasks

HussainRoker mentioned this pull request Dec 13, 2023

Dala 3045 manual maintenance sprint 57 d-fine/Dataland#381

Merged

97 tasks

BenediktLaubmann mentioned this pull request Jan 3, 2024

Manual Maintenance Sprint 58 d-fine/Dataland#388

Merged

exatar mentioned this pull request Feb 2, 2024

Manual maintenance sprint 60 d-fine/Dataland#430

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

0.49 support #667

0.49 support #667

wakingrufus commented Apr 27, 2023

JLLeitschuh commented Apr 28, 2023

JLLeitschuh left a comment

JLLeitschuh May 18, 2023

JLLeitschuh May 18, 2023

wakingrufus May 18, 2023

JLLeitschuh May 18, 2023

JLLeitschuh May 18, 2023

wakingrufus May 18, 2023

JLLeitschuh May 18, 2023

wakingrufus May 18, 2023

JLLeitschuh May 18, 2023

JLLeitschuh May 18, 2023

wakingrufus May 18, 2023

JLLeitschuh May 18, 2023

JLLeitschuh May 18, 2023

wakingrufus May 18, 2023

JLLeitschuh May 18, 2023

JLLeitschuh commented May 18, 2023

JLLeitschuh left a comment

JLLeitschuh left a comment

JLLeitschuh left a comment

0.49 support #667

0.49 support #667

Conversation

wakingrufus commented Apr 27, 2023

JLLeitschuh commented Apr 28, 2023

JLLeitschuh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JLLeitschuh commented May 18, 2023

JLLeitschuh left a comment

Choose a reason for hiding this comment

JLLeitschuh left a comment

Choose a reason for hiding this comment

JLLeitschuh left a comment

Choose a reason for hiding this comment