Can't use YarnTask using Gradle lazy configuration API

[jfbibeau]

The YarnTask class has a project.afterEvaluate call in the constructor, which conflicts with the Gradle task configuration avoidance concept (https://docs.gradle.org/current/userguide/task_configuration_avoidance.html).

Specifically:

Some APIs may be disallowed if you try to access them from the new API’s configuration blocks. For example, Project.afterEvaluate() cannot be called when configuring a task registered with the new API.

Reproduction scenario, simple project:

plugins {
  id "com.github.node-gradle.node" version "2.1.1"
}

tasks.register('runSomething', YarnTask) {
    args = ['run', 'anything']
}

tasks.register('doCopy', Copy) {
    from runSomething
}

Execute gradlew runSomething.

Result:

> Could not create task ':projectA:doCopy'.
   > Could not create task ':projectA:runSomething'.
      > Could not create task of type 'YarnTask'.
         > Project#afterEvaluate(Closure) on project ':projectA' cannot be executed in the current context.
...
Caused by: org.gradle.api.internal.AbstractMutationGuard$IllegalMutationException: Project#afterEvaluate(Closure) on project ':projectA' cannot be executed in the current context.
        at org.gradle.api.internal.AbstractMutationGuard.createIllegalStateException(AbstractMutationGuard.java:39)
        at org.gradle.api.internal.AbstractMutationGuard.assertMutationAllowed(AbstractMutationGuard.java:34)
        at org.gradle.api.internal.project.DefaultProject.assertMutatingMethodAllowed(DefaultProject.java:1434)
        at org.gradle.api.internal.project.DefaultProject.afterEvaluate(DefaultProject.java:1005)
        at org.gradle.api.Project$afterEvaluate.call(Unknown Source)
        at com.moowork.gradle.node.yarn.YarnTask.<init>(YarnTask.groovy:29)
        at com.moowork.gradle.node.yarn.YarnTask_Decorated.<init>(Unknown Source)

Can we remove the project.afterEvaluate call from the YarnTask constructor to enable using this plugin with the lazy task API (which is now the default in Gradle)?

Let me know if you need more details!

[jfbibeau]

Here's the Gradle folks' recommendation on how to fix this:

gradle/gradle#10923 (comment)

Looking at the code used inside the constructor of YarnTask, I can confirm that Gradle is correct. The quick solution would be to move the content of afterEvaluate to the @TaskAction annotated method.

[deepy]

I think we can quickly get rid of half of the afterEvalute, the section here YarnTask.groovy#L36-39

Could probably be moved to the ExecRunner's run() leaving the rest to be moved to the @TaskAction

[deepy]

Apparently not, if we do that then the up-to-date detection breaks.

[bsautel]

Yes, but as discussed in #40, we are probably going to remove the working directory from the task inputs, because it causes unwanted rebuilds. If we do that, this will probably remove the issue you are facing.

[bsautel]

I pushed a first commit that removed the usages of Project.afterEvaluate() in the tasks. It should fix the issue you encountered.

But I would like to go a bit further and always use this configuration avoidance API. The nodeSetup, npmInstall, yarn, npmSetup and yarnSetup tasks are declared using the create method and not the register one. I changed that but I am not able to get it work for now.

[jfbibeau]

Yes, there is another afterEvaluate in NodePlugin which is giving us quite a hard time. Changing that one might require significant re-work of the plugin to use the Gradle Property and Provider concepts for extensions.

https://docs.gradle.org/current/userguide/lazy_configuration.html#sec:connecting_properties_together

[bsautel]

I could replace all the afterEvaluate usages by the configuration API but it is not sufficient to solve the issue. In some basic cases, it works. But in the case a task is not created lazily, for instance because it is referenced directly by another one (without using a task provider), the configuration will be invoked before the Gradle configuration evaluation and the task will be configured with the default configuration and will not take into account the custom configuration.

You are right, in order to fix this issue, we have to use the Property concept in the configuration.

I suggest we do it in the next major version. The issue #17 proposes to rewrite the code in Kotlin, the static typing will help us doing this refactoring.

[bsautel]

I just pushed a big refactoring that enabled me to remove nearly all usages of project.afterEvaluate.

It sounds like the issue is now fixed. I improved the Kotlin DSL integration test to ensure that your use case works and it works.

I did not have to use Gradle's Property. It sounds like it is the recommended way, for instance the Gradle archive tasks use only properties.

I wonder whether we should use them or not. If we have to use them, we should do it now because we already broke backwards compatibility.

It does not change anything with the Groovy DSL but with the Kotlin DSL we have to replace property = 'value' by property.set('value'). What do you think about that @deepy and @jfbibeau?

[jfbibeau]

Fantastic! I think Properties are indeed to way to go forward for setting properties, compatible with Gradle's lazy task configuration.

[bsautel]

Sorry @jfbibeau , I'm not sure I understood what you mean, probably because my english is not very good. Do you think we should use properties right now or not?

[jfbibeau]

No worries, yes, I think we should use Properties, instead of getting values from an extension.

[deepy]

if property.set() gets tedious we could probably just create setters as iirc we're not supposed to re-assign the property itself.
I'm perfectly ok with Properties and Providers, although I've not yet used them myself

[bsautel]

Thanks for your answers. I am ok to work on that but I still have some questions.

Wrapping all task properties in the Property type will be simple.

But I don't know what to do with the NodeExtension. We can use properties inside the extension too. But many tasks depend on the Variant class that contains some values computed from the extension (configuration) and the environment (OS type for instance).

If we use properties in the NodeExtension, we will have to call .get() on all of them when building the variant and the result will be nearly the same as if we did not use properties at all.

That would be nice to maintain the Property chain and replace the Variant field by a Propvider<Variant> but I don't know how to compute it. The input contains many inputs of type Provider and I did not find a way of merging them like the zip operator of ReactiveX or the Promise.all method in Javascript. I just found the flatMap method, but is is able to merge only two Providers.

Do you see what I mean? Do you have any tips regarding this issue? Or any alternative solution?

Thanks!

[jfbibeau]

There's some good examples of this in the Gradle docs, including with a project extension:
https://docs.gradle.org/current/userguide/custom_plugins.html#sec:mapping_extension_properties_to_task_properties
https://docs.gradle.org/current/userguide/lazy_configuration.html#lazy_configuration

I think Variant as it is coded today would probably have to change significantly.