-
Notifications
You must be signed in to change notification settings - Fork 162
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.
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
Fix configuration cache for relative paths #722
Fix configuration cache for relative paths #722
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few bits of feedback and some questions. Other than that, nice work!
@@ -58,15 +59,13 @@ class ConfigurationCacheTest : AbstractPluginTest() { | |||
val formatTaskName = KtLintFormatTask.buildTaskNameForSourceSet("main") | |||
build( | |||
configurationCacheFlag, | |||
configurationCacheWarnFlag, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
@@ -90,6 +90,8 @@ abstract class GenerateReportsTask @Inject constructor( | |||
@get:Optional | |||
internal abstract val baseline: RegularFileProperty | |||
|
|||
private val rootDir: File = project.rootDir |
There was a problem hiding this comment.
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? 🙂
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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! 🙂
…on test with relative paths enabled
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Please add a changelog entry for this change. Then this should be good to go |
Added a line to the changelog @JLLeitschuh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Now to wait for CI
@JLLeitschuh , I don't have to click/do anything to continue, correct? |
Merged! Thanks for the fix!! |
resolves #721
The new test would fail before the changes to generate reports task and succeed afterwards