siouan / frontend-gradle-plugin

All-in-one Gradle Node/NPM/PNPM/Yarn plugin to build Javascript applications: package manager activation with Corepack, built-in tasks, additional task types.
https://siouan.github.io/frontend-gradle-plugin/
Apache License 2.0
153 stars 22 forks source link

Exit codes of scripts do not fail gradle tasks #215

Closed F43nd1r closed 10 months ago

F43nd1r commented 10 months ago

Hi,

I had some yarn tasks silently failing and noticed that a non-zero exit code from yarn did not fail my gradle build. I assume that's not the intended behaviour from this line: https://github.com/siouan/frontend-gradle-plugin/blob/2add49d3a74c927abc813d98787be116d0074afe/plugin/src/main/java/org/siouan/frontendgradleplugin/infrastructure/gradle/GradleScriptRunnerAdapter.java#L47

Environment

Attachments I wrote a test to demonstrate the problem:

package org.siouan.frontendgradleplugin.infrastructure.gradle;

import lombok.val;
import org.gradle.internal.impldep.org.junit.Rule;
import org.gradle.internal.impldep.org.junit.rules.TemporaryFolder;
import org.gradle.testkit.runner.GradleRunner;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.mockito.junit.jupiter.MockitoExtension;

import java.io.FileWriter;
import java.io.IOException;

@ExtendWith(MockitoExtension.class)
class AbstractRunCommandTaskTest {
    @Rule
    public TemporaryFolder dir = new TemporaryFolder();

    @Test
    void should_fail_if_script_returns_nonzero_exit_value() throws IOException {
        dir.create();
        val buildFile = dir.newFile("build.gradle.kts");
        try(val buildFileWriter = new FileWriter(buildFile)) {
            buildFileWriter.write("""
                    plugins {
                        id("org.siouan.frontend-jdk17")
                    }

                    frontend {
                        nodeInstallDirectory.set(file("build/nodejs"))
                        nodeVersion.set("16.20.0")
                        packageJsonDirectory.set(file("."))
                        assembleScript.set("exec sh -c 'exit 1'")
                    }
                    """);
        }
        val packageFile = dir.newFile("package.json");
        try(val packageFileWriter = new FileWriter(packageFile)) {
            packageFileWriter.write("""
                    {
                      "packageManager": "yarn@1.22.17"
                    }
                    """);
        }
        GradleRunner.create()
                .withProjectDir(dir.getRoot())
                .withArguments("assembleFrontend", "--stacktrace")
                .withPluginClasspath()
                .buildAndFail();
    }

    @Test
    void should_succeed_if_script_returns_zero_exit_value() throws IOException {
        dir.create();
        val buildFile = dir.newFile("build.gradle.kts");
        try(val buildFileWriter = new FileWriter(buildFile)) {
            buildFileWriter.write("""
                    plugins {
                        id("org.siouan.frontend-jdk17")
                    }

                    frontend {
                        nodeInstallDirectory.set(file("build/nodejs"))
                        nodeVersion.set("16.20.0")
                        packageJsonDirectory.set(file("."))
                        assembleScript.set("exec sh -c 'exit 0'")
                    }
                    """);
        }
        val packageFile = dir.newFile("package.json");
        try(val packageFileWriter = new FileWriter(packageFile)) {
            packageFileWriter.write("""
                    {
                      "packageManager": "yarn@1.22.17"
                    }
                    """);
        }
        GradleRunner.create()
                .withProjectDir(dir.getRoot())
                .withArguments("assembleFrontend", "--stacktrace")
                .withPluginClasspath()
                .build();
    }

}

(second test is a sanity check to be sure nothing else fails)

v1nc3n4 commented 10 months ago

Hi @F43nd1r,

Thank you for using the plugin. I found this configuration a bit strange because the project seems to be based on Yarn 1, but command exec is theorically only available with Yarn 2+. If it's working, I can't tell what's happening under the hood with Yarn.

  1. I didn't find any problems with exit codes forwarding to Gradle through the plugin. If I create a shell script script.sh containing: exit 4 and set assembleScript.set("exec sh script.sh"), Yarn fails with exit code 4, and Gradle fails the build as expected.

I think the problem comes from command exec sh -c 'exit X'. On WSL, the sh process does not exit as you might expect. I suggest you progress step by step from simple cases (e.g. exec ls for a passing example, exec ls -z for a failing example) to your target configuration.

Let me know your results here. BR

F43nd1r commented 10 months ago

Alright, I tried to give a generic example to a specific problem, here's the test for my specific failure:

package org.siouan.frontendgradleplugin.infrastructure.gradle;

import lombok.val;
import org.gradle.internal.impldep.org.junit.Rule;
import org.gradle.internal.impldep.org.junit.rules.TemporaryFolder;
import org.gradle.testkit.runner.GradleRunner;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.mockito.junit.jupiter.MockitoExtension;

import java.io.FileWriter;
import java.io.IOException;

@ExtendWith(MockitoExtension.class)
class AbstractRunCommandTaskTest {
    @Rule
    public TemporaryFolder dir = new TemporaryFolder();

    @Test
    void should_fail_if_script_returns_nonzero_exit_value() throws IOException {
        dir.create();
        val buildFile = dir.newFile("build.gradle.kts");
        try(val buildFileWriter = new FileWriter(buildFile)) {
            buildFileWriter.write("""
                    plugins {
                        id("org.siouan.frontend-jdk17")
                    }

                    frontend {
                        nodeInstallDirectory.set(file("build/nodejs"))
                        nodeVersion.set("16.20.0")
                        packageJsonDirectory.set(file("."))
                    }
                    """);
        }
        val packageFile = dir.newFile("package.json");
        try(val packageFileWriter = new FileWriter(packageFile)) {
            packageFileWriter.write("""
                    {
                      "packageManager": "yarn@4.0.1"
                    }
                    """);
        }
        GradleRunner.create()
                .withProjectDir(dir.getRoot())
                .withArguments("installFrontend", "--stacktrace")
                .withPluginClasspath()
                .buildAndFail();
    }

    @Test
    void should_succeed_if_script_returns_zero_exit_value() throws IOException {
        dir.create();
        val buildFile = dir.newFile("build.gradle.kts");
        try(val buildFileWriter = new FileWriter(buildFile)) {
            buildFileWriter.write("""
                    plugins {
                        id("org.siouan.frontend-jdk17")
                    }

                    frontend {
                        nodeInstallDirectory.set(file("build/nodejs"))
                        nodeVersion.set("16.20.0")
                        packageJsonDirectory.set(file("."))
                    }
                    """);
        }
        val packageFile = dir.newFile("package.json");
        try(val packageFileWriter = new FileWriter(packageFile)) {
            packageFileWriter.write("""
                    {
                      "packageManager": "yarn@3.6.3"
                    }
                    """);
        }
        GradleRunner.create()
                .withProjectDir(dir.getRoot())
                .withArguments("installFrontend", "--stacktrace")
                .withPluginClasspath()
                .build();
    }
}

Output:

> Task :installFrontend
Usage Error: This tool requires a Node version compatible with >=18.12.0 (got 16.20.0). Upgrade Node, or set `YARN_IGNORE_NODE=1` in your environment.

Yarn Package Manager - 4.0.1

  $ yarn <command>

You can also print more details about any of these commands by calling them with 
the `-h,--help` flag right after the command name.

BUILD SUCCESSFUL in 3s

And here it is happening in my pipeline: https://github.com/F43nd1r/zachtronics-leaderboard-bot/actions/runs/6811731302/job/18522761389#step:8:105

I would've hoped my pipeline would break for this update instead of the silent fail of frontend build I got instead.

v1nc3n4 commented 10 months ago

I don't understand why the plugin would not work as expected with the two examples you provided. Let me explain:

In the first test, command gradle installFrontend --stacktrace is executed which leads to command yarn install issued by the plugin. The output of this command tells you are using an incompatible release of Node.js with Yarn 4. At least Node.js 18.12.0 is expected. Then, as you noticed, the build is successful though yarn install failed. If you run yarn install in the exact same project by your own with a terminal, then run echo $?, you will see the exit code of this command is 0, even if it failed. That being said, the problem seems to be Yarn 4 is not exiting with a non-zero code when it fails installing dependencies with an incompatible release of Node.js (at least).

Do you confirm this analysis?

F43nd1r commented 10 months ago

If you run yarn install in the exact same project by your own with a terminal, then run echo $?, you will see the exit code of this command is 0, even if it failed.

$ yarn install
Usage Error: This tool requires a Node version compatible with >=18.12.0 (got 16.20.2). Upgrade Node, or set `YARN_IGNORE_NODE=1` in your environment.

━━━ Yarn Package Manager - 4.0.1 ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

  $ yarn <command>

You can also print more details about any of these commands by calling them with 
the `-h,--help` flag right after the command name.
$ echo $?
1

Seems like the exit code works correctly when I run the command by hand

v1nc3n4 commented 10 months ago

Strange 🤔. Exit code on my WSL Ubuntu is 0, and on Windows it's 1. It's difficult to help on this case because Yarn 4.0.1 is not compatible with Node.js 16.20.2. What if this problem with the exit code is due to this incompatibility? Let's upgrade Node and/or Yarn first, then let's try again and see if the issue is gone.

I don't see a reason why the plugin would not be able to forward the exit code of the command executed. The plugin just calls Gradle API that does it automatically, as you noticed in introduction. With my actual knowledge, the only potential causes are:

We are now focusing on another use case with an incompatible Node.js/Yarn pair. That was not the use case you introduced at the beginning of the discussion. Can you attach a sample project as a ZIP file with a valid Node.js/Yarn stack (not an automated test) so as I could reproduce the problem?

F43nd1r commented 10 months ago

Can you attach a sample project as a ZIP file with a valid Node.js/Yarn stack (not an automated test) so as I could reproduce the problem?

frontend-gradle-plugin-issue-215-reproducer.zip

Let's upgrade Node and/or Yarn first, then let's try again and see if the issue is gone.

Of course I've already updated node in my project. My issue is one of the versions (yarn) got automatically updated, which should've failed a test pipeline but didn't because of the problem here. I'm trying to prevent this from happening again (and already built a workaround, but would prefer a clean solution).

v1nc3n4 commented 10 months ago

Sorry but the project you provided still contains an incompatible stack of Node.js (16.20.1) and Yarn (4.0.1).

My issue is one of the versions (yarn) got automatically updated, which should've failed a test pipeline but didn't because of the problem here. I'm trying to prevent this from happening again (and already built a workaround, but would prefer a clean solution).

I'm trying to help, but to me, the problem is not the pipeline not failing due to incompatible versions of build tools. The problem is the "automatic" update of such a critical tool as Yarn that is not verified before and outside an automatic process.

F43nd1r commented 10 months ago

Sorry but the project you provided still contains an incompatible stack of Node.js (16.20.1) and Yarn (4.0.1).

I cannot provide a sample without that incompatibility because the incompatibility error is the point.


Regardless, seems like the issue is not with this plugin, at least not directly: The plugin calls ./node/bin/yarn, which uses corepack to call yarn, and the bundled corepack in node 16 seems to be swallowing the exit code. This behaviour is different from my global yarn installation because that doesn't use corepack. This difference explains the difference in exit codes.

Only fix I can see would be to call the installed yarn berry script directly (./.yarn/releases/yarn-4.0.1.cjs), but I'm guessing that would be out of the scope of this plugin?

v1nc3n4 commented 10 months ago

I cannot provide a sample without that incompatibility because the incompatibility error is the point.

You cannot expect Gradle or Yarn or the plugin to work as you expect if you use an invalid stack. As I said, considering the 2 use cases (install and assemble) we talk about, I don't see anything suggesting the plugin behaves abnormally because of the invalid stack first, then of the assemble script that did not do what you were expecting.

Again, it's wasted time to search the cause of an issue with an incompatible Node.js/Yarn combination. The plugin will not help you highlight such incompatibility in your pipeline because it is not its goal. I don't think Gradle or Yarn will for the same reason.

I may only help if you provide me a sample project to reproduce a problem where the plugin does not forward correctly the non-zero exit code of a command, with a valid stack.