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.

Sign up for GitHub

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

Jump to bottom

Fix configuration cache for relative paths #722

Merged
merged 2 commits into from
Nov 6, 2023
Merged

Fix configuration cache for relative paths #722

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4e2de3f
Let configuration tests fail and not warn and add failing configurati…
rschattauer Nov 3, 2023
fc680e1
Read project rootdir within task creation rather than within task action
rschattauer Oct 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/).
## [Unreleased]

- update latest version text file manually [#716](https://github.com/JLLeitschuh/ktlint-gradle/pull/716)
- Fix configuration cache for relative paths [#722](https://github.com/JLLeitschuh/ktlint-gradle/pull/722)

## [11.6.1] - 2023-10-10

Expand Down
9 changes: 8 additions & 1 deletion plugin/src/main/kotlin/org/jlleitschuh/gradle/ktlint/tasks/GenerateReportsTask.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,13 @@ abstract class GenerateReportsTask @Inject constructor(
@get:Optional
internal abstract val baseline: RegularFileProperty

/**
* Reading a project's rootDir within a task's action is not allowed for configuration
* cache, so read it eagerly on task initialization.
* see: https://docs.gradle.org/current/userguide/configuration_cache.html#config_cache
*/
private val rootDir: File = project.rootDir
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be class scoped? Or can it be scoped smaller, to only the init block? Also, can you elaborate on why this fixes this, or, at minimum, can you include an explicit comment in the code explaining why this is extracted to a variable eagerly. That way the rationale for this change isn't lost in the future? 🙂

Copy link
Contributor Author

@rschattauer rschattauer Nov 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to https://docs.gradle.org/current/userguide/configuration_cache.html#config_cache:requirements:~:text=Use%3A-,project.rootDir,-A%20task%20input for configuration caching rootDir you should use
A task input or output property or a script variable to capture the result of using project.rootDir to calculate the actual parameter.
So according to https://docs.gradle.org/current/userguide/configuration_cache.html#config_cache:requirements:use_project_during_execution A task must not use any Project objects at execution time. This includes calling Task.getProject() while the task is running. and to achieve that I was calling it within the class. I could also let the input set the relative rootdir path, but I thought this is easier to grasp? I added a comment to hopefully make it understandable.

What do you mean with init block scoped? There is no difference to this and

private val rootDir: File
init {
     rootDir = project.rootDir
}

or do I misunderstand you?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I misunderstood the code. I thought the rootDir was only used inside of the init block, but re-reading the code, I was completely wrong. My mistake.

Thanks for adding a comment inline in the code! 🙂


init {
// Workaround for https://github.com/gradle/gradle/issues/2919
onlyIf {
Expand Down Expand Up @@ -142,7 +149,7 @@ abstract class GenerateReportsTask @Inject constructor(
param.baseline.set(baseline)
param.projectDirectory.set(projectLayout.projectDirectory)
if (relative.get()) {
param.filePathsRelativeTo.set(project.rootDir)
param.filePathsRelativeTo.set(rootDir)
}
}
}
Expand Down
53 changes: 44 additions & 9 deletions plugin/src/test/kotlin/org/jlleitschuh/gradle/ktlint/ConfigurationCacheTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ import org.junit.jupiter.api.DisplayName
@GradleTestVersions
class ConfigurationCacheTest : AbstractPluginTest() {
private val configurationCacheFlag = "--configuration-cache"
private val configurationCacheWarnFlag = "--configuration-cache-problems=warn"

@DisplayName("Should support configuration cache without errors on running linting")
@CommonTest
Expand All @@ -29,15 +28,13 @@ class ConfigurationCacheTest : AbstractPluginTest() {

build(
configurationCacheFlag,
configurationCacheWarnFlag,
CHECK_PARENT_TASK_NAME
) {
assertThat(task(":$mainSourceSetCheckTaskName")?.outcome).isEqualTo(TaskOutcome.SUCCESS)
}

build(
configurationCacheFlag,
configurationCacheWarnFlag,
CHECK_PARENT_TASK_NAME
) {
assertThat(task(":$mainSourceSetCheckTaskName")?.outcome).isEqualTo(TaskOutcome.UP_TO_DATE)
Expand All @@ -58,15 +55,13 @@ class ConfigurationCacheTest : AbstractPluginTest() {
val formatTaskName = KtLintFormatTask.buildTaskNameForSourceSet("main")
build(
configurationCacheFlag,
configurationCacheWarnFlag,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you elaborate on why this isn't needed anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed it everywhere in this test now. Reason being that this would not fail the test when configuration actually fails. With relative path you would get a message that reads something like

FAILURE: Build failed with an exception.

* What went wrong:
Configuration cache problems found in this build.

2 problems were found storing the configuration cache, 1 of which seems unique.

So we probably want this test to fail in the future if someone implements something that is not capable of configuration caching?

FORMAT_PARENT_TASK_NAME
) {
assertThat(task(":$formatTaskName")?.outcome).isEqualTo(TaskOutcome.SUCCESS)
assertThat(task(":$mainSourceSetFormatTaskName")?.outcome).isEqualTo(TaskOutcome.SUCCESS)
}
build(
configurationCacheFlag,
configurationCacheWarnFlag,
FORMAT_PARENT_TASK_NAME,
"--debug"
) {
Expand All @@ -77,6 +72,50 @@ class ConfigurationCacheTest : AbstractPluginTest() {
}
}

@DisplayName("Should support configuration cache on running format tasks with relative paths")
@CommonTest
fun configurationCacheForFormatTasksWithRelativePaths(gradleVersion: GradleVersion) {
project(gradleVersion) {
buildGradle.appendText(
//language=Groovy
"""
repositories {
jcenter()
}

ktlint {
relative = true
reporters {
reporter "plain"
reporter "checkstyle"
}
}
""".trimIndent()
)
val sourceFile = "\nval foo = \"bar\"\n"
createSourceFile(
"src/main/kotlin/CleanSource.kt",
sourceFile
)
val formatTaskName = KtLintFormatTask.buildTaskNameForSourceSet("main")
build(
configurationCacheFlag,
FORMAT_PARENT_TASK_NAME
) {
assertThat(task(":$formatTaskName")?.outcome).isEqualTo(TaskOutcome.SUCCESS)
assertThat(task(":$mainSourceSetFormatTaskName")?.outcome).isEqualTo(TaskOutcome.SUCCESS)
}
build(
configurationCacheFlag,
FORMAT_PARENT_TASK_NAME
) {
assertThat(task(":$formatTaskName")?.outcome).isEqualTo(TaskOutcome.UP_TO_DATE)
assertThat(task(":$mainSourceSetFormatTaskName")?.outcome).isEqualTo(TaskOutcome.UP_TO_DATE)
assertThat(output).contains("Reusing configuration cache.")
}
}
}

@DisplayName("Should support configuration cache for git hook format install task")
@CommonTest
internal fun configurationCacheForGitHookFormatInstallTask(gradleVersion: GradleVersion) {
Expand All @@ -85,15 +124,13 @@ class ConfigurationCacheTest : AbstractPluginTest() {

build(
configurationCacheFlag,
configurationCacheWarnFlag,
INSTALL_GIT_HOOK_FORMAT_TASK
) {
assertThat(task(":$INSTALL_GIT_HOOK_FORMAT_TASK")?.outcome).isEqualTo(TaskOutcome.SUCCESS)
}

build(
configurationCacheFlag,
configurationCacheWarnFlag,
INSTALL_GIT_HOOK_FORMAT_TASK
) {
assertThat(task(":$INSTALL_GIT_HOOK_FORMAT_TASK")?.outcome).isEqualTo(TaskOutcome.SUCCESS)
Expand All @@ -110,15 +147,13 @@ class ConfigurationCacheTest : AbstractPluginTest() {

build(
configurationCacheFlag,
configurationCacheWarnFlag,
INSTALL_GIT_HOOK_CHECK_TASK
) {
assertThat(task(":$INSTALL_GIT_HOOK_CHECK_TASK")?.outcome).isEqualTo(TaskOutcome.SUCCESS)
}

build(
configurationCacheFlag,
configurationCacheWarnFlag,
INSTALL_GIT_HOOK_CHECK_TASK
) {
assertThat(task(":$INSTALL_GIT_HOOK_CHECK_TASK")?.outcome).isEqualTo(TaskOutcome.SUCCESS)
Expand Down
Loading