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.

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

Task.onlyIf, cacheIf, setEnabled should accept Provider<Boolean> #16080

Open
stephanenicolas opened this issue Feb 9, 2021 · 5 comments
Open
Assignees

Comments

@stephanenicolas
Copy link

Using Gradle 6.8+, consider a plugin:

 fun apply(project: Project) {
   val b: Boolean = project.hasProperty("foo")
    project.tasks.register("task", MyTask::class.java) {
       it.onlyIf { b }
    }
 }

because of the way kotlin compiles lambdas, it will create a lambda that contains a reference to the outer scope of the onlyIf lambda, which will include the plugin instance. This will most often break as plugins are allowed to use non serializable types such as projects, tasks, etc.
See : https://stackoverflow.com/q/44538627/693752

as a workaround, one can create a subclass of Spec that accepts a boolean as a constructor parameter and pass an instance of this class to onlyIf, so that there is no leak of the plugin instance.

To solve this, it would be nice to be able to provide an overload of onlyIf that would accept a provider of boolean.

@stephanenicolas
Copy link
Author

Actually, another workaround is to do

project.tasks.register("task", MyTask::class.java) {
       val b: Boolean = it.project.hasProperty("foo")
       it.onlyIf { b }
    }

So that the lambda is more self contained.

But yes, definitely onlyIf could accept a provider.

@eskatos
Copy link
Member

eskatos commented Feb 11, 2021

Note that this workaround above will miss a build logic input. It should be:

project.tasks.register<MyTask>("task") {
    val foo = project.providers.gradleProperty("foo")
    onlyIf { foo.isPresent }
}

👍 on having onlyIf(Provider<Boolean>).

@tasomaniac
Copy link

Hi folks, I hit this problem too and would appreciate any feedback if my workaround is ok. The last workaround is better but only applicable to property usage.

In my case, I was accessing a boolean property from my extension. I resolved it by assigning the extension to a "redundant" local variable. GradleUp/auto-manifest#14

@h0tk3y
Copy link
Member

h0tk3y commented Feb 7, 2023

To be more precise, the scenario that this issue affects is when the lambda that is SAM-converted to a Spec (or another interface) and stored in the configuration cache captures the properties of the enclosing class that reference non-serializable model objects.

It does not reproduce under the following conditions:

  • If the lambda does not reference the properties of any class – e.g. if it only references local variables of the method where it is declared, which is one of the workarounds for the issue;
  • If the enclosing class does not attempt to store the references to non-serializable model objects, for example:
    • abstract @get:Inject property to access the Project instance instead of storing it manually in the constructor
    • the class may use @Transient on the property that holds a non-serializable model object, if it is only used at configuration time.

The example in the original issue description does not seem to reproduce the issue in the modern Gradle versions as-is, unless some additional conditions are created.

@mlopatkin mlopatkin changed the title Task.onlyIf is hardly compatible with Configuration Cache, due to kotlin lambda compilation Task.onlyIf, cacheIf, setEnabled should accept Provider<Boolean> Mar 20, 2023
@bamboo
Copy link
Member

bamboo commented Mar 24, 2023

Note to implementer: The provider should be able to carry task dependencies.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants