fix(core): don't mark package as built if any of its scripts fails (#5176)

Author: merceyz

.yarn/versions/7d105aea.yml

@@ -0,0 +1,34 @@
+releases:
+  "@yarnpkg/cli": patch
+  "@yarnpkg/core": patch
+
+declined:
+  - "@yarnpkg/plugin-compat"
+  - "@yarnpkg/plugin-constraints"
+  - "@yarnpkg/plugin-dlx"
+  - "@yarnpkg/plugin-essentials"
+  - "@yarnpkg/plugin-exec"
+  - "@yarnpkg/plugin-file"
+  - "@yarnpkg/plugin-git"
+  - "@yarnpkg/plugin-github"
+  - "@yarnpkg/plugin-http"
+  - "@yarnpkg/plugin-init"
+  - "@yarnpkg/plugin-interactive-tools"
+  - "@yarnpkg/plugin-link"
+  - "@yarnpkg/plugin-nm"
+  - "@yarnpkg/plugin-npm"
+  - "@yarnpkg/plugin-npm-cli"
+  - "@yarnpkg/plugin-pack"
+  - "@yarnpkg/plugin-patch"
+  - "@yarnpkg/plugin-pnp"
+  - "@yarnpkg/plugin-pnpm"
+  - "@yarnpkg/plugin-stage"
+  - "@yarnpkg/plugin-typescript"
+  - "@yarnpkg/plugin-version"
+  - "@yarnpkg/plugin-workspace-tools"
+  - "@yarnpkg/builder"
+  - "@yarnpkg/doctor"
+  - "@yarnpkg/extensions"
+  - "@yarnpkg/nm"
+  - "@yarnpkg/pnpify"
+  - "@yarnpkg/sdks"

packages/acceptance-tests/pkg-tests-specs/sources/commands/install.test.js

@@ -318,6 +318,29 @@ describe(`Commands`, () => {
       ),
     );
 
+    test(
+      `should not mark package as built if any of its scripts fails`,
+      makeTemporaryEnv(
+        {
+          scripts: {
+            preinstall: `echo 'foo'`,
+            postinstall: `exit 1`,
+          },
+        },
+        async ({path, run, source}) => {
+          await expect(run(`install`, `--inline-builds`)).rejects.toMatchObject({
+            code: 1,
+            stdout: expect.stringContaining(`foo`),
+          });
+
+          await expect(run(`install`, `--inline-builds`)).rejects.toMatchObject({
+            code: 1,
+            stdout: expect.stringContaining(`foo`),
+          });
+        },
+      ),
+    );
+
     test(
       `should wait for direct dependencies to finish building`,
       makeTemporaryMonorepoEnv(

packages/yarnpkg-core/sources/Project.ts

@@ -1353,7 +1353,7 @@ export class Project {
 
     while (buildablePackages.size > 0) {
       const savedSize = buildablePackages.size;
-      const buildPromises = [];
+      const buildPromises: Array<Promise<unknown>> = [];
 
       for (const locatorHash of buildablePackages) {
         const pkg = this.storedPackages.get(locatorHash);
@@ -1436,18 +1436,15 @@ export class Project {
               stdout.end();
               stderr.end();
 
-              if (exitCode === 0) {
-                nextBState.set(pkg.locatorHash, buildHash);
+              if (exitCode === 0)
                 return true;
-              }
 
               xfs.detachTemp(logDir);
 
               const buildMessage = `${structUtils.prettyLocator(this.configuration, pkg)} couldn't be built successfully (exit code ${formatUtils.pretty(this.configuration, exitCode, formatUtils.Type.NUMBER)}, logs can be found here: ${formatUtils.pretty(this.configuration, logFile, formatUtils.Type.PATH)})`;
 
               if (this.optionalBuilds.has(pkg.locatorHash)) {
                 report.reportInfo(MessageName.BUILD_FAILED, buildMessage);
-                nextBState.set(pkg.locatorHash, buildHash);
                 return true;
               } else {
                 report.reportError(MessageName.BUILD_FAILED, buildMessage);
@@ -1456,15 +1453,21 @@ export class Project {
             });
 
             if (!wasBuildSuccessful) {
-              return;
+              return false;
             }
           }
+
+          return true;
         });
 
         buildPromises.push(
           ...pkgBuilds,
-          Promise.allSettled(pkgBuilds).then(() => {
+          Promise.allSettled(pkgBuilds).then(results => {
             buildablePackages.delete(locatorHash);
+
+            if (results.every(result => result.status === `fulfilled` && result.value === true)) {
+              nextBState.set(pkg.locatorHash, buildHash);
+            }
           }),
         );
       }