#292 Fix custom sources location not being recognized by ktfmt

[simonhauck]

🚀 Description

As described in #292 the main issue is that the SourceDirectorySet::getSourceDirectories() can change. When the ktfmt plugin is configured before the sourceDirectory is changed, the plugin will format the old directory where no sources are located.

That's why we have to evaluate it lazily. This is done in this PR.

Unfortunately, there are one side effect that I am not able to solve.

In the example module, we get the following error when running the check task

A problem was found with the configuration of task ':example:generateMainDatabaseInterface' (type 'SqlDelightTask').
  - Gradle detected a problem with the following location: '/home/shauck/dev/workspace/ktfmt-gradle2/example/build/generated/sqldelight/code/Database/main'.
    
    Reason: Task ':example:ktfmtCheckMain' uses this output of task ':example:generateMainDatabaseInterface' without declaring an explicit or implicit dependency. This can lead to incorrect results being produced, depending on what order the tasks are executed.
    
    Possible solutions:
      1. Declare task ':example:generateMainDatabaseInterface' as an input of ':example:ktfmtCheckMain'.
      2. Declare an explicit dependency on ':example:generateMainDatabaseInterface' from ':example:ktfmtCheckMain' using Task#dependsOn.
      3. Declare an explicit dependency on ':example:generateMainDatabaseInterface' from ':example:ktfmtCheckMain' using Task#mustRunAfter.

The sqlDelight plugin generates a kotlin main sourceSet. So it is expected that the dependencies between the two tasks must be explicitly configured. This is done on the example build.gradle.kts.

This is a "breaking change" and can require users of this plugin to perform additional configuration.

📄 Motivation and Context

#292

🧪 How Has This Been Tested?

An integration test was added for the check and format task.

📦 Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)

it is in some sense a breaking change because users might have to explicitly specify the dependencies of ktfmt to other tasks that modify the sourceSets.

✅ Checklist

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.

[cortinico]

Thanks for investigating on this one @simonhauck

This is a "breaking change" and can require users of this plugin to perform additional configuration.

I think we should filter the source folders we receive (by either using an euristic or applying the includes/excludes the users provide). It's pretty annoying that Gradle doesn't handle this out of the box, despite them providing SourceTask with includes/excludes

[simonhauck]

Before implementing something, maybe you could have a look at the following options. Because I am not happy with either one :D

Option 1: Heuristic with the regex
That solution is relatively easy to implement and straightforward.

# 
private fun createCheckTask(...){
        # ...
        val inputDirs =
            project.provider {
                srcDir.toList().filterNot { it.path.matches(KtfmtPlugin.defaultExcludesRegex) }
            }
         # ...
}

From my testing this seems to work. We could also simplify the base task a bit by removing

    @get:Internal
    internal val defaultExcludesFilter: Spec<File> =
        Spec<File> {
            if (this.excludes.containsAll(KtfmtPlugin.defaultExcludes) && this.excludes.size == 1) {
                it.absolutePath.matches(KtfmtPlugin.defaultExcludesRegex).not()
            } else {
                true
            }
        }

But that would in theory also be a breaking change. Because now we always exclude the build folder from the formatted files. From what I can see, it would be possible to overwrite the default exclude when specifying anything else in the excludes of the source task.

Option 2: Applying the ant matchers from the excludes filed
Honestly, I don't even know how to make this nice or work.

        return project.tasks.register(taskName, KtfmtFormatTask::class.java) { task ->
            task.setSource(
                project.provider {
                    srcDir
                        .toList()
                        .filterNot { path ->
                            task.excludes.contains(path.name)
                        }
                     
                })
            task.setIncludes(KtfmtPlugin.defaultIncludes)
            task.setExcludes(KtfmtPlugin.defaultExcludes)
        }

First, it is not really nice to implement :D now the includes and excludes are still set in the task but the source must be filtered with the provider, that is also accessing the task. That is such a weird circular dependency. Additionally, this does not even work, because the excludes are ANT style paths pattersn. I am not aware of an easy way to filter a file with an ANT path....

Personally, I could live with excluding the build folder, but still, it is a breaking change. If somebody wanted to format sources files in the build folder, they would have to create a custom format and check task, because there the filder would not be applied.

If we want to go with option 1 and want to mitigate the risk of breaking the logic for someone, we could add a boolean flag to the ktfmt extension, if the files in the build directory should be excluded.

Last but not least, there is always the option to just accept that users have to define the task dependencies.

Please let me know what you think about these ideas. Maybe I am missing a completely simple solution.

[cortinico]

Additionally, this does not even work, because the excludes are ANT style paths pattersn. I am not aware of an easy way to filter a file with an ANT path....

I would personally prefer option 2. Gradle has a FileTree class that should allow us to compose the srcDir we get in input with the excludes.

If we want to go with option 1 and want to mitigate the risk of breaking the logic for someone, we could add a boolean flag to the ktfmt extension, if the files in the build directory should be excluded.

Also for option 1., excluding the build folder is a reasonable thing to implement. Adding a boolean flag to toggle this behavior can just be a nice-to-have and probably something not really hard to maintain in the long run.

[simonhauck]

Good news first :D yes it works with the filetree

But honestly, I am not really sure if it is a good solution.

return project.tasks.register(taskName, KtfmtFormatTask::class.java) { task ->
            task.description =
                "Run Ktfmt formatter validation for sourceSet '$name' on project '${project.name}'"
            task.setSource(
                project.provider {
                    srcDir.flatMap { srcRoot ->
                        project
                            .fileTree(srcRoot) {
                                it.include(task.includes)
                                it.setExcludes(task.excludes)
                            }
                            .files
                    }
                })
            task.setIncludes(KtfmtPlugin.defaultIncludes)
            task.setExcludes(KtfmtPlugin.defaultExcludes)
        }

With that it even uses the tasks includes and excludes, but it feels not great :D This is basically a re-implementation of the source task. We filter now the files before they are passed on.

But honestly, this feels really hacky...

While investigating this, I noticed that the getSource() method just does not apply any exclude rules even though they are set and the documentation also says, that those are the files after the includes and excludes were applied. I have the feeling, if that filtering would be working, this would also solve the problem.

    @PathSensitive(PathSensitivity.RELATIVE)
    @InputFiles
    @IgnoreEmptyDirectories
    override fun getSource(): FileTree = super.getSource().also {
        println("---  Excludes. ${this.excludes}")
        println("--- ${it.files}") }

Do you, by any chance, know why the source task does not apply the excludes at all? I think at some point you solve this by manually excluding the files. Maybe it is related to this 12 year old issue https://discuss.gradle.org/t/inconsistent-behavior-from-sourcetask-exclude/5469 ?

[cortinico]

Do you, by any chance, know why the source task does not apply the excludes at all?

I'm not entirely sure, but it could be related to relative paths.
I remember reading about it on another issue (that I can't find at the moment) that the srcDir we receive in input contains relative paths and the excludes/includes are applied only on the relative path section.

[simonhauck]

Thanks for the advice and you are right.
It seems the filtering is only applied to the relative path of the files inside the file tree and not on the parent.
This might be related to gradle/gradle#3994

So when we call setSource(absolute-root/workspace/ktfmt-gradle/example/build/generated/sqldelight/code/Database/main), the build folder, that we want to ignore is not in the relative path.

I was also not able to make the solution above work. Maybe it never did, and I was just misreading something. Additionally, I am a bit concerned about performance in large codebases since this would essentially build the file tree twice (I think) and is also really hard to understand.

Therefore, my proposal is to introduce a new optional property in the ktfmt configuration with a default value, something like

ktfmt {
      sourceDirectoryExclusionPattern = "^(.*[\\/])?build([\\/].*)?$"
}

This is essentially the regex that is already part of the KtfmtPlugin companion object. So by default, all build folders are completely ignored. No need to build a filetree for that. This offers even more flexibility as you can now exclude any source directory, not just in the build folder.

This also simplifies the actual ktfmt base class, since the additional filter is no longer required

    @get:Internal
    internal val defaultExcludesFilter: Spec<File> =
        Spec<File> {
            if (this.excludes.containsAll(KtfmtPlugin.defaultExcludes) && this.excludes.size == 1) {
                it.absolutePath.matches(KtfmtPlugin.defaultExcludesRegex).not()
            } else {
                true
            }
        }

If you agree, I would try to finish the implementation this weekend.

[cortinico]

Therefore, my proposal is to introduce a new optional property in the ktfmt configuration with a default value, something like

So that's something doable.

I guess the only annoying part is that the BaseTask was implemented as a SourceTask so we would use include/exclude as other tasks in the ecosystem (for example, the compileKotlin task uses it).

If we create our own "exclusion pattern" we sort of deviate from how SourceTasks behave in Gradle.

If there is no other solution, it's something we can go with (is going to be a niche feature nonetheless).

[simonhauck]

Yeah i know and fully agree it is annoying.

In the end it is in theory something different. The include / exclude work on files in a sourceset but we want to filter the entire sourceset.

And for the source task it would probably be weird to just ignore random sources... if you do not want it as source. It should not be added as a source.

But yeah... that does not change the fact that it is annoying 😒

[cortinico]

And for the source task it would probably be weird to just ignore random sources...

Yeah that's also a really valid point and I can see users getting frustrated because of this

[simonhauck]

Soooo :D
It is done... and it would be great if you could have a look @cortinico.
It was a bit more complicated than expected.

What I have done

• Added the new property to exclude arbitrary build folders
• Resolve the fileCollection lazily so that users can change them, and the plugin detects the sources correctly
• Cleaned up the source task, since we no longer have to manually filter files there
• and last but not least, I have changed the ouputs up-to-date check in the Format task from the input files to outputs.upToDateWhen { true }. Gradle has a really "unique" behavior of deleting stale outputs, and I noticed that in a test. I was also able to reproduce it in the example. It only happens when the project is running for the first time, but then it was removing the sourceSet content in the build folder. This is not really acceptable :D When running the project multiple times, the behavior was different, but in the test it was always reproducible. The up to date check should still work as expected, since the task is rerun if the input files change, which of course happens if some file was formatted.

One last note, just FYI:

    @Test
    fun `format task should detect the source and test files in a flattened project structure and format them`() {
        appendToBuildGradle(
            """
            |kotlin {
            |    sourceSets.main {
            |       kotlin.setSrcDirs(listOf("src"))
            |    }
            |    sourceSets.test {
            |       kotlin.setSrcDirs(listOf("test"))
            |    }
            |}
        """
                .trimMargin())
 // ....

It is really important to use setSrcDirs. If not the sourceSet is only appended.
In this example, this leads to the sourceSets for main and test
Main: src/main/java, src/main/kotlin, src
Test: src/test/java, src/test/kotlin/, test.

And now multiple tasks use the same inputs and Gradle will complain about it. This is, in my opinion, nothing we have to fix, but is relevant for #292. If this PR is merged, I would comment on this issue as well. But that was one of the issues I encountered in the tests.

[cortinico]

LGTM 👍 Sorry for the late review @simonhauck