fix(turborepo): Rationalize the install and execution process. #5695

nathanhammond · 2023-08-09T08:59:41Z

Previously we had a lot of complexity in our install scripts inherited from esbuild. This change removes all of the incidental complexity in favor of relying upon default ecosystem behaviors in most cases.

Instead of attempting correction of configuration at both installation and invocation, it handles everything at invocation. Early returns ensure that we perform the exact same amount of work as we did in the previous variation.

It can now also detect and provide instructions on how to repair a broken package-lock.json caused by npm/cli#4828. (Note: if the "just try installing" saving throw works, we won't provide this information to the user. I elected this ordering for startup time reasons.)

As an added bonus, turbo no longer has a postinstall script which is more end-user-friendly.

Material changes:

Does not attempt to extract turbo from a Yarn PnP without preferUnplugged support. This works with all stable releases of yarn (berry). This is a change for users on canary versions >2.0.0-rc.0 && <=2.0.0-rc.30 from September 2019 – March 2020—but we already don't support in other portions of our codebase so this only changes where the error is thrown from.
In the event of npm install download failure it does not attempt to directly download a tarball. If a user misconfigured their setup and we didn't succeed at npm install to bail them out? It's okay for you to get an error with lots of details on how to fix your configuration.

vercel · 2023-08-09T08:59:51Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
examples-designsystem-docs	🔄 Building (Inspect)	Visit Preview	💬 Add feedback	Aug 10, 2023 5:16am
examples-kitchensink-blog	🔄 Building (Inspect)	Visit Preview	💬 Add feedback	Aug 10, 2023 5:16am
examples-vite-web	🔄 Building (Inspect)	Visit Preview	💬 Add feedback	Aug 10, 2023 5:16am

8 Ignored Deployments

Name	Status	Preview	Updated (UTC)
examples-basic-web	⬜️ Ignored (Inspect)	Visit Preview	Aug 10, 2023 5:16am
examples-cra-web	⬜️ Ignored (Inspect)	Visit Preview	Aug 10, 2023 5:16am
examples-gatsby-web	⬜️ Ignored (Inspect)	Visit Preview	Aug 10, 2023 5:16am
examples-native-web	⬜️ Ignored (Inspect)	Visit Preview	Aug 10, 2023 5:16am
examples-nonmonorepo	⬜️ Ignored (Inspect)	Visit Preview	Aug 10, 2023 5:16am
examples-svelte-web	⬜️ Ignored (Inspect)	Visit Preview	Aug 10, 2023 5:16am
examples-tailwind-web	⬜️ Ignored (Inspect)	Visit Preview	Aug 10, 2023 5:16am
turbo-site	⬜️ Ignored (Inspect)	Visit Preview	Aug 10, 2023 5:16am

vercel · 2023-08-09T08:59:54Z

@nathanhammond is attempting to deploy a commit to the Vercel Team on Vercel.

A member of the Team first needs to authorize it.

nathanhammond

Reviewer's Guide!

nathanhammond · 2023-08-09T09:00:36Z

packages/turbo/bin/turbo

+// If we do not find the correct platform binary, should we attempt to install it?
+const SHOULD_INSTALL = true;
+
+// If we do not find the correct platform binary, should we trust calling an emulated variant?
+const SHOULD_ATTEMPT_EMULATED = true;


I left these in as configuration variables; but we can actually hardcode the decisions here.

nathanhammond · 2023-08-09T09:02:35Z

packages/turbo/bin/turbo

+  child_process.execSync(
+    `npm install --loglevel=error --prefer-offline --no-audit --progress=false`,
+    { cwd: turboPath, stdio: "pipe", env }
+  );


Previously this had to carefully track which package and version, clean up after itself, and more.

Now we just rely on npm to do the right thing when looking at the package.json for turbo, clean up in the event of failure, etc.

nathanhammond · 2023-08-09T09:03:37Z

packages/turbo/bin/turbo

+// This provides logging messages as it progresses towards calculating the binary path.
+function getBinaryPath() {
+  // First we see if the user has configured a particular binary path.
+  const TURBO_BINARY_PATH = process.env.TURBO_BINARY_PATH;


We still need to allow the user to specify a particular binary path.

nathanhammond · 2023-08-09T09:05:38Z

packages/turbo/bin/turbo

+  const correctBinary = availablePlatforms.includes(platform) && availableArchs.includes(resolvedArch) ? `turbo-${platform}-${resolvedArch}/bin/turbo${ext}` : null;
+  if (correctBinary !== null) {
+    try {
+      return require.resolve(`${correctBinary}`);


By rule we should be able to require.resolve() the platform-specific package from within the turbo directory, so we do that. (1:1 with the existing behavior.)

nathanhammond · 2023-08-09T09:06:52Z

packages/turbo/bin/turbo

+      installUsingNPM();
+      const resolvedPath = require.resolve(`${correctBinary}`);
+      console.warn('Installation has succeeded.');


We don't succeed unless we can resolve it, so this gets split across lines.

packages/turbo/bin/turbo

nathanhammond · 2023-08-09T09:37:50Z

packages/turbo/bin/turbo

+
+            console.error();
+            console.error('To resolve this issue for your repository, run:');
+            console.error(`npm install turbo${version} --package-lock-only${environment} && npm install`);


@chris-olszewski This is the magic command that fixes things without rm -rf.

nathanhammond · 2023-08-09T09:38:43Z

packages/turbo/bin/turbo

+        break;
+      } catch (e) {}
+
+      let next = path.join(current, '..', '..', 'package-lock.json');


No-dependencies find-up.

nathanhammond · 2023-08-09T09:39:04Z

packages/turbo/package.json

@@ -9,15 +9,12 @@
  "main": "./bin/turbo",
  "scripts": {
    "postversion": "node bump-version.js",
-    "postinstall": "node install.js"


gsoltis

Just a couple of notes on errors / warnings, otherwise LGTM.

Are there any concerns about running this when the repo is using package managers besides npm?

packages/turbo/bin/turbo

chris-olszewski

This is so much more straight forward!

I think we can get away with not building out this level of support for package-lock.json version 1. The last version of npm that used this version by default was 6 and node 16 (EOL 9/11) was the last one to ship with npm 6.

packages/turbo/bin/turbo

chris-olszewski · 2023-08-09T20:54:55Z

packages/turbo/bin/turbo

+
+            console.error();
+            console.error('To resolve this issue for your repository, run:');
+            console.error(`npm install turbo${version} --package-lock-only${environment} && npm install`);


gsoltis · 2023-08-10T02:53:04Z

Ah, right, the package name should match, even if the architecture doesn't. Ok, sgtm

…

On Wed, Aug 9, 2023, 6:51 PM Nathan Hammond ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In packages/turbo/bin/turbo <#5695 (comment)>: > + if (SHOULD_INSTALL && correctBinary !== null) { + console.warn('Turborepo did not find the correct binary for your platform.'); + console.warn('We will attempt to install it now.'); + + try { + installUsingNPM(); + const resolvedPath = require.resolve(`${correctBinary}`); + console.warn('Installation has succeeded.'); + return resolvedPath; + } catch (e) { + console.warn('Installation has failed.'); + } + } + + // 3. Both Windows and macOS ARM boxes can run x64 binaries. Attempt to run under emulation. + const alternateBinary = (arch === "arm64" && ['darwin', 'windows'].includes(platform)) ? `turbo-${platform}-64/bin/turbo${ext}` : null; I believe this is still worth warning on. Even if you are running under emulation you don't by default get the differently-named binary. So getting the correctly-named binary is still important. Phrased differently: if you run an install on windows arm, you should get the windows arm binary. If you don't have the windows arm binary it is indicative of an error in configuration (probably the npm issue) every time. Also, to get this warning at all you have to fail the install saving throw where we would drop the correct binary on your box. I'm going to keep this as is. — Reply to this email directly, view it on GitHub <#5695 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADGD7ODNAKRC3T3WWA7HWTXUQ5DZANCNFSM6AAAAAA3JVMNAY> . You are receiving this because your review was requested.Message ID: ***@***.***>

nathanhammond · 2023-08-10T02:57:30Z

Are there any concerns about running this when the repo is using package managers besides npm? - @gsoltis

Nope! There are two things happening here that do npm-related nonsense:

An npm install inside of the <PROJECT>/node_modules/turbo folder.

Writing to this destination (no matter how it is implemented underneath the hood with linking) results in safe behavior—even if it pollutes the cache, even if that cache is later restored from a random cache, even if that cache comes from a different platform, even if the base project is a different package manager. It works because it leverages npm install to handle the behavior, which on rerun always puts things into the expected state for the dependencies, and the resolve algorithm.

The material difference is that, in a misconfigured repository, if we restore from a polluted cache, on the same platform (worst-case scenario), the correct binary will be found inside the node_modules/turbo/node_modules/turbo-<platform>-<arch> folder instead of node_modules/turbo-<platform>-arch. This is a harmless distinction since those two things should be identical in order to work anyway.

Looking for package-lock.json.

This does not directly invoke or mutate npm, it just looks on the file system for evidence of the package-lock.json error and gives you a command to fix it. Even failing the command generation just gives you a slightly more-permissive turbo version.

Rationalize the install and execution process.

8d4a52b

nathanhammond requested review from gsoltis, tknickman and chris-olszewski August 9, 2023 08:59

nathanhammond requested a review from a team as a code owner August 9, 2023 08:59

turbo-orchestrator bot added the team: turborepo label Aug 9, 2023

nathanhammond commented Aug 9, 2023

View reviewed changes

This was linked to issues Aug 9, 2023

[turborepo] Unable to find Go binary - go-turbo-bin is missing #5650

Closed

[turborepo] Unable to find Go binary on Apple Silicon #5692

Closed

vercel bot deployed to Preview – turbo-site August 9, 2023 10:02 View deployment

Off by one on starting position.

c5b5658

gsoltis approved these changes Aug 9, 2023

View reviewed changes

packages/turbo/bin/turbo Outdated Show resolved Hide resolved

packages/turbo/bin/turbo Show resolved Hide resolved

chris-olszewski approved these changes Aug 9, 2023

View reviewed changes

nathanhammond added 3 commits August 10, 2023 11:29

Hard error for TURBO_BINARY_PATH.

aa8e7c2

Formatting.

1d5e4d5

Support npm 9.

c4f0709

nathanhammond added the pr: automerge Kodiak will merge these automatically after checks pass label Aug 10, 2023

kodiakhq bot merged commit ad313e6 into vercel:main Aug 10, 2023
34 checks passed

nathanhammond deleted the no-postinstall branch August 10, 2023 05:53

tknickman mentioned this pull request Aug 15, 2023

fix(release): correct version script #5735

Merged

This was referenced Aug 21, 2023

[turborepo] #5496

Closed

[turborepo] Vercel deployment error #5512

Closed

Strange behaviour after git hook is run (Error: Cannot find module 'turbo') #2147

Closed

mehulkar mentioned this pull request Oct 19, 2023

[turborepo] Better error reporting in postinstall script #5205

Closed

renovate bot mentioned this pull request Nov 13, 2023

fix(deps): Update dependency eslint-config-turbo to v1 stipsan/groqz#47

Open

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(turborepo): Rationalize the install and execution process. #5695

fix(turborepo): Rationalize the install and execution process. #5695

nathanhammond commented Aug 9, 2023 •

edited

Loading

vercel bot commented Aug 9, 2023 •

edited

Loading

vercel bot commented Aug 9, 2023

nathanhammond left a comment

nathanhammond Aug 9, 2023

nathanhammond Aug 9, 2023

nathanhammond Aug 9, 2023

nathanhammond Aug 9, 2023

nathanhammond Aug 9, 2023

nathanhammond Aug 9, 2023

chris-olszewski Aug 9, 2023

nathanhammond Aug 9, 2023

nathanhammond Aug 9, 2023

gsoltis left a comment

chris-olszewski left a comment

chris-olszewski Aug 9, 2023

gsoltis commented Aug 10, 2023 via email

nathanhammond commented Aug 10, 2023

fix(turborepo): Rationalize the install and execution process. #5695

fix(turborepo): Rationalize the install and execution process. #5695

Conversation

nathanhammond commented Aug 9, 2023 • edited Loading

vercel bot commented Aug 9, 2023 • edited Loading

vercel bot commented Aug 9, 2023

nathanhammond left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gsoltis left a comment

Choose a reason for hiding this comment

chris-olszewski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gsoltis commented Aug 10, 2023 via email

nathanhammond commented Aug 10, 2023

nathanhammond commented Aug 9, 2023 •

edited

Loading

vercel bot commented Aug 9, 2023 •

edited

Loading