Make exec methods of tasks open

[ThisIsANiceName]

It would be really great if you could add the keyword open to the exec method in the abstract tasks like NodeTask. This would enable plugin developers to overwrite the task action and provide additional flexibility.

E.g like this:

abstract class NodeTask : DefaultTask() {
...

    @TaskAction
    open fun exec() {
        ....
    }
...
}

[deepy]

That's one of the two major use-cases for execOverrides, but given the currently very light-weight nature of NodeTask would subclassing it really be useful? I mean looking at the class it's just holding a bunch of parameters and then passing it off to NodeExecConfiguration + NodeExecRunner

[deepy]

Which now that I think of it are both internal so that's not really very useful.

But for modifying it, right now execOverrides would be the best course of action, though we should probably open runners and their configurations, which would make adding support for new systems (like say pnpm) easier

[nickcaballero]

Overriding the exec method would allow plugin developers to execute the command differently, maybe forking it to the background or handling the output differently. Right now doing that with execOverrides is not trivial.

[deepy]

@nickcaballero I'm thinking what you really want is for the NodeExecRunners and their util classes to be opened, not the NodeTask itself

Currently I don't see any big issue with opening them but since they're all kinda coupled it'd be great if you could have a look at one of the FooExecRunners and see if opening that and the utils it uses would let you do what you want

[nickcaballero]

That's fair. I'm currently creating NpmTask instances and then using execOverrides to hijack the execution and use ProcessBuilder instead. I should be able to do what I want using NpmExecRunner.

[deepy]

I've created https://github.com/node-gradle/gradle-node-plugin/pull/202 where I've started opening classes, it doesn't have all of them yet but I believe it should be the major ones necessary, let me know if there's any specifics you feel might be missing (except NpmExecRunner/YarnExecRunner, I haven't gotten around to those yet)

[deepy]

@nickcaballero I don't suppose your usage is available open-source somewhere?

I'm being torn on what to actually open vs what to make public, right now I'm leaning towards almost exclusively making things public.

The ExecRunners mostly do a lot of composition between the utility classes and I don't see anywhere where inheritance would fit, the exception being VariantComputer but currently there's no way to swap that out. So I think the most sensible thing is making the ExecRunners public, documenting what NodeExecConfiguration actually does. (or maybe at least mention that command is the part that goes after npm/npx/yarn or in the case of node the script + arguments)

[laurie71]

I just ran into this trying to migrate from the old moowork version of this plugin; we have a task definition that extends from NodeTask and is then used in multiple places; it looks something like this:

abstract class JasmineTests extends NodeTask {
    Boolean coverage = true

    @TaskAction
    void exec() {
        if (coverage) {
            script = ...
            args = ...
        } else {
            ...
        }

        super.exec()
    }
}

where the exec() override configures working directory, script and args according to project config and and task invocation -- e.g. whether coverage is set to false where the task is used, as in:

task unittests(type: JasmineTests, dependsOn: npm_install) {
    onlyIf { executeUnitTests == "yes" }
    coverage = executeCodeCoverage == "yes"
    ...
}

The custom task abstracts running tests in various configurations (with/without coverage reporting, etc). How would I migrate this to using execOverrides?

[deepy]

Would something like this work?

tasks.register("jasmineFalse", JasmineTests) {
    coverage.set(false)
}

abstract class JasmineTests extends NodeTask {
    @Input
    abstract Property<Boolean> getCoverage()

    JasmineTests() {
        args.set(coverage.map {
            if (it) {
                ["--no-coverage"]
            } else {
                ["--coverage"]
            }
        })
    }
}

One sidenote is that you may want to depend on npmInstall instead of npm_install, for most cases it's the one you want to use

[laurie71]

Thanks @deepy, will try that once I've fixed some lazy property related issues.

[laurie71]

Unfortunately that doesn't seem to work; it looks like properties aren't set yet in the constructor so any input property I try to reference there comes back null. Let me show our full usage with the old plugin:

class JasmineTests extends NodeTask {
    Boolean coverage = true
    String specRoot

    @TaskAction
    void exec() {
        def workDir = this.runner.getWorkingDir()
        def specConfig = "${specRoot}/support/jasmine.json"
        def jasmineBin = "${workDir}/node_modules/.bin/jasmine"
        def nycBin = "${project.projectDir}/node_modules/.bin/nyc"

        if (coverage) {
            script = new File("${nycBin}")
            args = [
                    "-s",
                    "--clean=false",
                    "--temp-dir=${project.buildDir}/coverage",
                    "--report-dir=${project.buildDir}/coverage",
                    "--exclude=src/agent/test",
                    "${jasmineBin}",
                    "JASMINE_CONFIG_PATH=${specConfig}",
                    "JASMINE_DEFAULT_TIMEOUT_INTERVAL=180000"
            ]
        } else {
            script = new File("${jasmineBin}")
            args = [
                    "JASMINE_CONFIG_PATH=${specConfig}",
                    "JASMINE_DEFAULT_TIMEOUT_INTERVAL=180000"
            ]
        }

        super.exec()
    }
}

task unittests_agent(type: JasmineTests, dependsOn: npm_install) {
    onlyIf { executeUnitTests == "yes" }

    coverage = executeCodeCoverage == "yes"
    workingDir = "${projectDir}/path/to/tests"
    specRoot = "${projectDir}/src/foo/test/spec"
    inputs.dir { "${projectDir}/src/foo/lib" }
    inputs.dir { specRoot }
    outputs.dir { "build/coverage" }
}

Converting the exec() override into a constructor, properties evaluate to null. I'm also having issues setting the workingDir property, which doesn't accept a string, and retrieving it (this.runner isn't available in this iteration of the plugin?).

[deepy]

@laurie71 I ran out of patience midway and it doesn't work on Windows, but here's a quick stab at this that does properly set the script depending on -Pcoverage=yes|no being passed when building.

With some fixes and adapting it to your environment I think this should resolve your issue.

The important difference between your old usage of it is that I'm never .get()-ing the value just mapping it to new providers down the line

abstract class JasmineTests extends com.github.gradle.node.task.NodeTask {
    @javax.inject.Inject
    abstract ProviderFactory getProviders();

    @javax.inject.Inject
    abstract ProjectLayout getLayout();

    @org.gradle.api.tasks.Internal
    abstract DirectoryProperty getSpecRoot();

    @javax.inject.Inject
    JasmineTests() {
        def jasmineBin = layout.projectDirectory.file("node_modules/.bin/jasmine.cmd")
        def specConfig = specRoot.file("support/jasmine.json")
        def coverageProperty = providers.gradleProperty("coverage")
                .forUseAtConfigurationTime()
                .map { it == "yes"}
        def scriptFile = coverageProperty
                .map {
                    if (it) {
                        layout.projectDirectory.file("node_modules/.bin/nyc")
                    } else {
                        layout.projectDirectory.file("node_modules/.bin/jasmine")
                    }
                }
        def arguments = coverageProperty.map {
            if (it) {
                [
                        "-s",
                        "--clean=false",
                        "--temp-dir=${layout.buildDirectory}/coverage",
                        "--report-dir=${layout.buildDirectory}/coverage",
                        "--exclude=src/agent/test",
                        "${jasmineBin}",
                        "JASMINE_CONFIG_PATH=${specConfig}",
                        "JASMINE_DEFAULT_TIMEOUT_INTERVAL=180000"
                ]
            } else {
                [
                        "JASMINE_CONFIG_PATH=${specConfig}",
                        "JASMINE_DEFAULT_TIMEOUT_INTERVAL=180000"
                ]
            }
        }
        args.set(arguments)
        script.set(scriptFile)
    }
}

tasks.register("unittestsAgent", JasmineTests) {
    specRoot.set(project.layout.projectDirectory.dir("src/foo/test/spec"))
    inputs.dir { "${projectDir}/src/foo/lib" }
    outputs.dir { "build/coverage" }
}

[deepy]

I'm going to close this as done, if anyone finds more cases where exec() needs to be open feel free to re-open :-)

[laurie71]

@deepy Thanks so much for spending the time to come up with a solution! We ended up having to abandon the custom task definition and using NodeTask directly, which resulted in some annoying code duplication. Hopefully we'll be able to use this to fix that.