Why checkstyle would depend on files in build output?

I am refactoring an old build config. And while doing so I converted some code to kotlin

    val mockitoExtConfigFiles = listOf(
        mockitoExtensionConfigFile(project, "org.mockito.plugins.MockMaker"),
        mockitoExtensionConfigFile(project, "org.mockito.plugins.MemberAccessor"),
    )

    // This task is used to generate Mockito extensions for testing see ci.yml build matrix.
    val createTestResources by registering {
        outputs.files(mockitoExtConfigFiles)
        doLast {
            // Configure MockMaker from environment (if specified), otherwise use default
            configureMockitoExtensionFromCi(project, "org.mockito.plugins.MockMaker", "MOCK_MAKER")

            // Configure MemberAccessor from environment (if specified), otherwise use default
            configureMockitoExtensionFromCi(project, "org.mockito.plugins.MemberAccessor", "MEMBER_ACCESSOR")
        }
    }
    processTestResources {
        dependsOn(createTestResources)
    }

    val removeTestResources by registering {
        outputs.files(mockitoExtConfigFiles)
        doLast {
            mockitoExtConfigFiles.forEach {
                if (it.exists()) {
                    it.delete()
                }
            }
        }
    }
    test {
        finalizedBy(removeTestResources)
    }
Other methods in build script
fun Task.configureMockitoExtensionFromCi(
    project: Project,
    mockitoExtension: String,
    ciMockitoExtensionEnvVarName: String,
) {
    val mockitoExtConfigFile = mockitoExtensionConfigFile(project, mockitoExtension)
    val mockMaker = project.providers.environmentVariable(ciMockitoExtensionEnvVarName)
    if (mockMaker.isPresent && !mockMaker.get().endsWith("default")) {
        logger.info("Using $mockitoExtension ${mockMaker.get()}")
        mockitoExtConfigFile.run {
            parentFile.mkdirs()
            createNewFile()
            writeText(mockMaker.get())
        }
    } else {
        logger.info("Using default $mockitoExtension")
    }
}

fun mockitoExtensionConfigFile(project: Project, mockitoExtension: String) =
    file("${project.sourceSets.test.get().output.resourcesDir}/mockito-extensions/$mockitoExtension")

This fails because checkstyleTest has a hidden dependency on removeTestResources :

 - Gradle detected a problem with the following location: '/home/runner/work/mockito/mockito/mockito-core/build/resources/test/mockito-extensions/org.mockito.plugins.MockMaker'.
    
    Reason: Task ':mockito-core:checkstyleTest' uses this output of task ':mockito-core:removeTestResources' 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 ':mockito-core:removeTestResources' as an input of ':mockito-core:checkstyleTest'.
      2. Declare an explicit dependency on ':mockito-core:removeTestResources' from ':mockito-core:checkstyleTest' using Task#dependsOn.
      3. Declare an explicit dependency on ':mockito-core:removeTestResources' from ':mockito-core:checkstyleTest' using Task#mustRunAfter.
    
    For more information, please refer to https://docs.gradle.org/8.10.2/userguide/validation_problems.html#implicit_dependency in the Gradle documentation.

This looks odd to me because the above config files are only created in the build directory, while checkstyle should operate on the src folders I believe?

Got a reproducer, see point 2.

implicit-dependency-gradle-31353.zip (71.9 KB)

I did not look at your reproducer, just at your post here.
You declare mockitoExtConfigFiles as output files of createTestResources and removeTestResources.
I guess you register the createTestResources task as test source directory so that checkstyle checks its ouputs.
And as you declare the same files as outputs of removeTestResources, Gradle complains that you use the output of that task in the checkstyle task without dependency.

Actually there are some major problems:

  • tasks should never have overlapping outputs if you prefer not to end in hell
  • for removeTestResources it does not even make any sense to declare those as output, as they are not output of that task, that task does not create those files, it destroys them, you should use destroyables instead to register the files the task destroys instead of declaring it as output wrongly
  • practically any explicit dependsOn that does not have a lifecycle-task on the left-hand side is a code smell and usually a sign of you doing something not properly, most often not properly wiring task outputs and inputs together but configuring paths manually or smiliar.
1 Like

I guess you register the createTestResources task as test source directory so that checkstyle checks its ouputs.

The build script do not at least explicitly, every related task or config is shown in this snippet, the reproducer won’t show much more in this matter.

Regardless your answer is insightful, I need to digest that. About the overlapping, I had a background idea to represent these test resources with a different sourceset / configuration.

What is strange is that this issue was not captured before the refactoring. I barely ported existing logic from a groovy dsl script. It was however coming from an applied script that I decided to merge into the main build script.

I declared the following

val testExtensionConfigFiles: SourceSet by sourceSets.creating
sourceSets {
  test {
    runtimeClasspath += testExtensionConfigFiles.output
  }
}

And of course adapting the paths to use the introduced source set.
I also declared destroyables instead of outputs.

This seem to work! Thank you!

However I am still unsure about your third point about dependsOn, ie, there is still the processTestResources dependency on createTestResources.

What is strange is that this issue was not captured before the refactoring. I barely ported existing logic from a groovy dsl script. It was however coming from an applied script that I decided to merge into the main build script.

The recognition of the problem is sub-optimal.
It currently only triggers if the involved tasks are actually run and only if run in the “wrong” order.
With fixing Validation problem "Implicit dependencies between tasks" is not consistently reported · Issue #27576 · gradle/gradle · GitHub this hopefully improves and consistently reports the problem.
So it could for example be that after your refactoring the task run order changed and thus the error is found now when before it was present but not detected.

However I am still unsure about your third point about dependsOn, ie, there is still the processTestResources dependency on createTestResources.

You only mentioned testExtensionConfigFiles in your last snippet, so I don’t know what the full situation is.
But as I said, practically any explicit dependsOn where on the left-hand side is not a lifecycle task is a code smell hinting at something is not idiomatic.
If you for example have processTestResources { dependsOn(createTestResources) }, then this most often means that createTestResources is not configured as test resources source directory, but that you for example have configured the output directory of createTestResources configured manually as source directory, or that createTestResources outputs files into src/test/resources which is also not the best idea and so on. This also means, that any other task operates on the resources like a sourcesJar task and similar do not have the necessary task dependency and maybe get the files if they were produced before or not if not, or get a stale state of those files.

Usually createTestResources would output into a dedicated output directory only for that task like layout.buildDirectory.dir("generated/resources/foo") or whatever and then registered as source directory like sourceSets { test { resources { srcDir(createTestResources) } } }. Then any task that consumes test resources, including processTestResources and others automatically have the necessary task dependency and get the files as resources as needed.

1 Like

That feels like a better approach indeed. Sorry for the small snippet, I was on the phone back then.
Here’s the code I tried yesterday.

val testExtensionConfigFiles: SourceSet by sourceSets.creating
sourceSets {
    test {
        runtimeClasspath += testExtensionConfigFiles.output
    }
}

tasks {
    // ...
    val mockitoExtConfigFiles = listOf(
        mockitoExtensionConfigFile(testExtensionConfigFiles, "org.mockito.plugins.MockMaker"),
        mockitoExtensionConfigFile(testExtensionConfigFiles, "org.mockito.plugins.MemberAccessor"),
    )

    // This task is used to generate Mockito extensions for testing see ci.yml build matrix.
    val createTestResources by registering {
        outputs.files(mockitoExtConfigFiles)
        doLast {
            // Configure MockMaker from environment (if specified), otherwise use default
            configureMockitoExtensionFromCi(project, "org.mockito.plugins.MockMaker", "MOCK_MAKER")

            // Configure MemberAccessor from environment (if specified), otherwise use default
            configureMockitoExtensionFromCi(project, "org.mockito.plugins.MemberAccessor", "MEMBER_ACCESSOR")
        }
    }
    processTestResources {
        dependsOn(createTestResources)
    }

    val removeTestResources by registering {
        mockitoExtConfigFiles.forEach(destroyables::register)
    }
    test {
        finalizedBy(removeTestResources)
    }
}

I’ll try your approach with the generated dir.


Actually, maybe that’s my mental model there where left-hand side is still a bit fuzzy to me with the way tasks are written.

explicit dependsOn where on the left-hand side is not a lifecycle task

Here’s the code I tried yesterday

You are still sharing incomplete snippets, because the mockitoExtensionConfigFile you shared previously took Project as first parameter, not Configuration. :smiley:

Actually, maybe that’s my mental model there where left-hand side is still a bit fuzzy to me with the way tasks are written.

Well, if you use eager ways (don’t do it), left-hand size is a bit more obvious:

val bar by tasks.existing
val foo by tasks.getting
foo.dependsOn(bar)

Using lazy way this is the left-hand side like in:

val bar by tasks.existing
val foo by tasks.existing {
    dependsOn(bar)
}

“left-hand side” just means “the thing you call dependsOn on”.

1 Like

Haha, I try to avoid adding too much noise.

The full code is in this PR: Convert module build scripts to kotlin script by bric3 · Pull Request #3514 · mockito/mockito · GitHub
Build script at last commit in this branch : mockito/mockito-core/build.gradle.kts at 830090ad684fb5a77a8c0ee239373e2272b0edc4 · mockito/mockito · GitHub

“left-hand side” just means “the thing you call dependsOn on”.

Ah ok, this removes the doubts there, thank you for the explanation.

I’m not going to analyze all that (I know you don’t expect that :slight_smile: ), but there are quite some things. For example I think removeTestResources is pointless.

In the current state it will not do anything (registering destroyables is like registering input or output files, you tell Gradle what the task will use as inputs or use as outputs or delete when it is run, but does not actually do anything on its own). And even if it would again delete the files as in your OP here, you should be able to simply use the task cleanCreateTestResources which should be added by a task rule for all tasks and delete the tasks output files.

1 Like

Thank you for all your comments @Vampire. Your points made me stumble on how some projects what I think you were describing, even if they have a few issues as you spotted.

So now it looks like that (which I believe is “self contained”). The builtBy appears to even have made the clean task superfluous.

val mockitoExtConfigFiles = listOf(
    mockitoExtensionConfigFile(project, "org.mockito.plugins.MockMaker"),
    mockitoExtensionConfigFile(project, "org.mockito.plugins.MemberAccessor"),
)

val createTestResources by tasks.registering {
    outputs.files(mockitoExtConfigFiles)
    doLast {
        // Configure MockMaker from environment (if specified), otherwise use default
        configureMockitoExtensionFromCi(project, "org.mockito.plugins.MockMaker", "MOCK_MAKER")

        // Configure MemberAccessor from environment (if specified), otherwise use default
        configureMockitoExtensionFromCi(project, "org.mockito.plugins.MemberAccessor", "MEMBER_ACCESSOR")
    }
}

sourceSets.test {
    resources {
        output.dir(
            layout.buildDirectory.dir("generated/resources/ext-config/test"),
            "builtBy" to createTestResources
        )
    }
}

fun Task.configureMockitoExtensionFromCi(
    project: Project,
    mockitoExtension: String,
    ciMockitoExtensionEnvVarName: String,
) {
    val mockitoExtConfigFile = mockitoExtensionConfigFile(project, mockitoExtension)
    val extEnvVar = project.providers.environmentVariable(ciMockitoExtensionEnvVarName)
    if (extEnvVar.isPresent && !extEnvVar.get().endsWith("default")) {
        logger.info("Using $mockitoExtension ${extEnvVar.get()}")
        mockitoExtConfigFile.run {
            parentFile.mkdirs()
            createNewFile()
            writeText(extEnvVar.get())
        }
    } else {
        logger.info("Using default $mockitoExtension")
    }
}

fun mockitoExtensionConfigFile(project: Project, mockitoExtension: String) =
    file(project.layout.buildDirectory.dir("generated/resources/ext-config/test/mockito-extensions/$mockitoExtension"))

I didn’t knew about the builtBy before, this is barely documented, I only saw it mentioned in SourceSetOutput doc but I didn’t make an extensive research.

Your points made me stumble on how some projects

Dunno why it there is done like it is done.
Could have some reason, or just being done unidiomatically.
In the first link I’d probably try it something like output.dir(apiDeclaration.flatMap { it.destinationFile }.map { it.asFile.parentFile }),
in the second link probably something like output.dir(xsdResources.map { it.destinationDir.parentFile.parentFile.parentFile.parentFile }).
But the second link is not even leveraging task-configuration avoidance but eagerly creates the task, so :man_shrugging:.

Indeed. The spring boot may need to use register for the Gradle config, I suppose it’s possible because the task type has a destinationFile property but it’s not available for “simple” Task.

It all depends as usual. :smiley:
If you want all outputs of a task, you can just use the task.
If a task has multiple output properties, but only want the files of one of the properties, there are two cases.
If it is one of the “new” Property types and properly annotated with an output property, it carries the task dependency just like the task is, so you can directly use the output property if you want all outputs as is of that property.
If it is an older implementation where it for example is a File property, then you either can use builtBy, or you can use the task provider with .map as then still the task dependency is coming from task provider.
Nowadays you seldomly need builtBy except maybe in rare edge-cases.