Open asodja opened 2 months ago
The problem I have is that whether to force forking or not depends on whether the task uses a toolchain that is the same as the current JVM (and its version), so this can only be determined late (and at a minimum later than the plugin application).
If isFork becomes a lazy property then I could probably compute a convention value lazily with a provider (although AFAICT having providers that read from other lazy properties of the same task is seen by Gradle as a circular dependency: https://github.com/gradle/gradle/issues/16604) and then verify at execution time.
Otherwise, I could probably use an afterEvaluate
(and still verify at execution time).
WDYT?
The idea with validation is that it could change the strategy and not auto correct value for the user, but user would be forced to set a fork option. So if it's not correctly set, you fail a build in doFirst
and tell what to do to the user.
But if that is too annoying, I guess the only option is to use afterEvaluate
. Of course once we have a lazy property, that can be changed.
A question: What would change if plugin would set fork
to true
always? I see that plugin opens low level compiler APIs that I would guess most users don't have open by default anyway.
although AFAICT having providers that read from other lazy properties of the same task is seen by Gradle as a circular dependency: https://github.com/gradle/gradle/issues/16604
I believe that issue from a link is only a problem for properties that are outputs (could be wrong though). In such case you can use FileSystemLocationProperty.getLocationOnly().
I think something like:
options.generatedSourceOutputDirectory.locationOnly.map {}
could work for you.
The idea with validation is that it could change the strategy and not auto correct value for the user, but user would be forced to set a fork option. So if it's not correctly set, you fail a build in
doFirst
and tell what to do to the user.
Understood, but I'd like to avoid this as it adds friction given most users would have to configure forking (see below), so afterEvaluate
it would be but…
But if that is too annoying, I guess the only option is to use
afterEvaluate
. Of course once we have a lazy property, that can be changed.
Is this a blocker for you? What if I update the plugin for Gradle 9 asap? I'd rather do that than add friction for users if possible.
Do I understand correctly that with Gradle 9 isFork()
would become a Property<Boolean>
? Or would you add a getFork()
instead? (no idea how Groovy would behave with both an isFork and getFork, and no idea how Kotlin behaves when setting the property: should you set isFork or fork? maybe both?)
I could easily add an if (GradleVersion.current().baseVersion >= GradleVersion.version("9.0"))
that uses reflection to get that isFork
(or getFork
) using reflection and set it to a provider.
BTW, with lazy properties, can we set a value that depends on possibly previously set value? (i.e. do something equivalent to fork = fork || someComputation
, but without reading the property value eager, keeping everything lazy)
Currently the plugin only forces forking when needed, but will not change whatever strategy was configured in the build when forking is not needed; changing that to set the lazy property to a provider means that when forking is not need the provider would have to avoid changing the behavior that could have been set by another plugin that happened to be applied before (if a plugin configures all compilation tasks to fork, the errorprone plugin should not revert the tasks to not forking)
A question: What would change if plugin would set
fork
totrue
always? I see that plugin opens low level compiler APIs that I would guess most users don't have open by default anyway.
Yes, for most users nowadays it would force forking. Cases where it would not are:
org.gradle.jvmargs
properties (see test)The reasons I didn't want to unconditionally fork are:
Now if you think those are "bad reasons", I'm open to change things (it would likely simplify the plugin's code too)
although AFAICT having providers that read from other lazy properties of the same task is seen by Gradle as a circular dependency: gradle/gradle#16604
I believe that issue from a link is only a problem for properties that are outputs (could be wrong though). In such case you can use FileSystemLocationProperty.getLocationOnly().
I think something like:
options.generatedSourceOutputDirectory.locationOnly.map {}
could work for you.
TIL about locationOnly
Will try it, thanks!
Is this a blocker for you? What if I update the plugin for Gradle 9 asap? I'd rather do that than add friction for users if possible.
This is only blocker for us in a sense that we run smoke tests for ErrorProne Gradle plugin, so these fail atm. And I would guess also building Gradle itself with Gradle 9 will fail since we use ErrorProne.
What if you change logic to something like:
if (!options.isFork && (it == JavaVersion.VERSION_1_8 || (it == JavaVersion.current() && CURRENT_JVM_NEEDS_FORKING))) options.isFork = true
That way isFork
will be set only when not set to true
. That way we (users) can workaround this by explicitly setting isFork = true
at configuration time.
Would that be an acceptable solution for you for now? That will basically make it work the same as now for Gradle <= 8 and with Gradle 9 it will fail, only if fork == false
. And once Gradle 9 is released you can probably have a better solution with lazy properties.
Do I understand correctly that with Gradle 9 isFork() would become a Property
? Or would you add a getFork() instead? (no idea how Groovy would behave with both an isFork and getFork, and no idea how Kotlin behaves when setting the property: should you set isFork or fork? maybe both?) I could easily add an if (GradleVersion.current().baseVersion >= GradleVersion.version("9.0")) that uses reflection to get that isFork (or getFork) using reflection and set it to a provider.
For Gradle 9 we plan to remove a setter and change isFork()
to:
Property<Boolean> getFork();
// Added just for Kotlin DSL source compatibility
@Deprecated
Property<Boolean> getIsFork() {
return getFork();
}
That way in build.gradle.kts
you could still do:
compileOptions.fork = true
compileOptions.isFork = true // this will show deprecation
If your plugin is compiled with Gradle <= 8 we will "upgrade" your plugin (transform bytecode), so if it contains code like:
compileOptions.setFork(true)
compileOptions.isFork()
it will be changed on a bytecode level to:
compileOptions.getFork().set(true)
compileOptions.getFork().getOrElse(false)
and that way old plugins will be binary compatible with Gradle 9. That is currently what we are experimenting.
BTW, with lazy properties, can we set a value that depends on possibly previously set value? (i.e. do something equivalent to fork = fork || someComputation, but without reading the property value eager, keeping everything lazy)
We will probably add some transform
like functionality to Providers, either in 8.x, or some early 9.x, but it's still not entirely clear how it should work (maybe you know we added [replace](https://docs.gradle.org/8.8-rc-1/release-notes.html#:~:text=Updating%20lazy%20property%20based%20on%20its%20current%20value%20with%20replace()) method in 8.8-rc-1 but then removed it)
Is this a blocker for you? What if I update the plugin for Gradle 9 asap? I'd rather do that than add friction for users if possible.
This is only blocker for us in a sense that we run smoke tests for ErrorProne Gradle plugin, so these fail atm. And I would guess also building Gradle itself with Gradle 9 will fail since we use ErrorProne.
What if you change logic to something like:
if (!options.isFork && (it == JavaVersion.VERSION_1_8 || (it == JavaVersion.current() && CURRENT_JVM_NEEDS_FORKING))) options.isFork = true
That way
isFork
will be set only when not set totrue
. That way we (users) can workaround this by explicitly settingisFork = true
at configuration time.
:man_facepalming: Indeed!
Just released version 4.0.1 with this change.
Would that be an acceptable solution for you for now? That will basically make it work the same as now for Gradle <= 8 and with Gradle 9 it will fail, only if
fork == false
. And once Gradle 9 is released you can probably have a better solution with lazy properties.
I'll keep this issue open until then.
Awesome, thanks! 🎉
In Gradle 9.0 we plan to migrate majority of JVM plugins to lazy properties. We plan to bridge old raw type methods to new lazy methods on the bytecode level for old plugins, so they will be bytecode compatible. But we cannot bridge some of the behaviours.
While upgrading CompileOptions.isFork() to lazy
Property<Boolean>
type, we've noticed via smoke tests that ErrorProne sets fork option at the execution time indoFirst
block. WithProperty<Boolean>
that becomes an error, sinceProperty
is finalized and can't be changed at execution time, so upgrade to lazy property breaksErrorProne
plugin.One solution that could work here for you, is that you configure
CompileOptions.isFork
at configuration time and then you validate the value at the execution time and fail if it's not set to true, see also https://github.com/gradle/gradle/issues/25836#issuecomment-1966663444.Would be possible to change the behaviour that way?
The error that is reported after property upgrade to a lazy property: