ErrorProne plugin configures Java tasks at the execution time

[asodja]

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 in doFirst block. With Property<Boolean> that becomes an error, since Property is finalized and can't be changed at execution time, so upgrade to lazy property breaks ErrorProne 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 gradle/gradle#25836 (comment).

Would be possible to change the behaviour that way?

---

The error that is reported after property upgrade to a lazy property:

* What went wrong:
Execution failed for task ':compileJava'.
> The value for task ':compileJava' property 'options' property 'fork' is final and cannot be changed any further.

[tbroyer]

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: gradle/gradle#16604) and then verify at execution time.
Otherwise, I could probably use an afterEvaluate (and still verify at execution time).

WDYT?

[asodja]

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: 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.

[tbroyer]

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 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.

Yes, for most users nowadays it would force forking. Cases where it would not are:

• JVM used to run Gradle is different from the toolchain (will already fork anyway)
• JVM version is between 9 and 15 (inclusive)
• ErrorProne is disabled for the task through the extension
• the compiler APIs are opened in the org.gradle.jvmargs properties (see test)

The reasons I didn't want to unconditionally fork are:

• performance mainly; IIRC, the Android plugin used to fork unconditionally and at some point managed to run compilation in the same process because forking was hurting performance; I don't have numbers as I personally only work in rather small projects but there are users of the plugin with big projects where it could matter; now maybe Gradle's forking is lighterweight nowadays (using a compile daemon rather than forking for each compile task)
• it makes it a bit harder to debug the Java compilation process, for those using annotation processors, other JavaC plugins, or ErrorProne custom checks, or who need to debug ErrorProne itself

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!

[asodja]

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 method in 8.8-rc-1 but then removed it)

[tbroyer]

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.

🤦‍♂️ 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.

[asodja]

Awesome, thanks! 🎉