fix(berry arg parsing) #1612

chris-olszewski · 2022-08-01T22:01:20Z

This PR adds:

A new e2e test that verifies that package scripts aren't getting extra -- passed to them. e.g. NPM, Yarn, PNPM<7 all expect a -- to separate package manager args from script args, but other do not.
A fix for berry where turbo was passing -- as an arg separator. (See Turbo is sending arg -- (ignore-option) as first argument to script. #1477)
Add pnpm6 as a package manager that we run e2e tests against
Move pnpm6/pnpm7 behavior split to the standard package manager abstraction

I'm not happy with how I'm going about getting pnpm@6.22.2 for the e2e tests which involves installing it to a tempdir and then adding that to $PATH whenever we shell out, but it's what I got working that didn't mess with any global installs.

vercel · 2022-08-01T22:01:25Z

@chris-olszewski is attempting to deploy a commit to the Vercel Team on Vercel.

A member of the Team first needs to authorize it.

jaredpalmer · 2022-08-01T22:45:46Z

Failures are not due to this. Have some kind of config mishap with versioning

nathanhammond

Mostly: lots and lots more test scenarios!

nathanhammond · 2022-08-02T17:03:06Z

cli/scripts/monorepo.ts

@@ -184,6 +184,7 @@ importers:
          test: `${turboPath} run test`,
          lint: `${turboPath} run lint`,
          special: "echo root task",
+          args: 'node -e "console.log(process.argv)" --',


Can you help me understand why we should include -- here? I'm assuming that it's there for for a reason and I'd like to have it be a bit more fleshed-out to help with detection of issues.

node -e "console.log(process.argv)" --before-separator=thing -- --after-separator=thing

We can also add additional scripts for "has separator" and "doesn't have separator."

I had the trailing -- so that we could detect when turbo was passing an unnecessary separator since Node swallows the first --:

olszewski@chriss-mbp cli % node -e "console.log(JSON.stringify(process.argv))" -- foo ["/Users/olszewski/.nvm/versions/node/v16.16.0/bin/node","foo"] olszewski@chriss-mbp cli % node -e "console.log(JSON.stringify(process.argv))" -- -- foo ["/Users/olszewski/.nvm/versions/node/v16.16.0/bin/node","--","foo"]

Wow, it's turtles all the way down. Can you add this information in as a comment?

I still would like to have a before and after sentinel argument here, just to monitor for behavior changes.

Can you elaborate on having a before/after sentinel arguments for this script?

Unsure if it relates but, once node encounters an arg it doesn't recognize it treats that and every arg after it as a pass through:

olszewski@chriss-mbp cli % node -e "console.log(JSON.stringify(process.argv))" before -- after ["/Users/olszewski/.nvm/versions/node/v16.16.0/bin/node","before","--","after"] olszewski@chriss-mbp cli % node -e "console.log(JSON.stringify(process.argv))" --zero-fill-buffers before ["/Users/olszewski/.nvm/versions/node/v16.16.0/bin/node","before"] olszewski@chriss-mbp cli % node -e "console.log(JSON.stringify(process.argv))" before --zero-fill-buffers ["/Users/olszewski/.nvm/versions/node/v16.16.0/bin/node","before","--zero-fill-buffers"]

(Read in thread context.)
Good callout that node itself blows up here. Thanks for that! This is fine as is.

nathanhammond · 2022-08-02T17:04:16Z

cli/internal/packagemanager/packagemanager.go

@@ -36,6 +36,10 @@ type PackageManager struct {
 	// The directory in which package assets are stored by the Package Manager.
 	PackageDir string

+	// The separator that the Package Manger uses to identify arguments that
+	// should be passed through to the underlying script.
+	ArgSeparator []string


I know this maps to the previous, but can this be changed to not be an array of strings?

What's the most Go-like way to represent this? The comment on the pnpm 6/7 split:

// We are allowed to use nil here because argSeparator already has a type, so it's a typed nil,
// vs if we tried to call "args = append(args, nil...)", which would not work. This could
// just as easily be []string{}, but the style guide says to prefer nil for empty slices.

Would it be best practice/common to use the empty string as the nil value and check it before appending to the arg slice?

The existing solution is too clever by half, but I understand now why it exists.

String types can't be nil (only empty string)

[]string{}'s "zero value" is nil.

I'm ... comfortable leaving it the way it is. 🤷‍♂️ The only other reasonable options are:

check for "" (not ideal since it's an in-band sentinel)

far more-invasive (a whole new struct just for arg separator, and a method to get it, guards at the append, and then it's all just a mess)

So, yeah, let's add more to the comment and call it done.

nathanhammond · 2022-08-02T17:06:13Z

cli/scripts/e2e/e2e.ts

+      const result = getCommandOutputAsArray(
+        repo.turbo(
+          "run",
+          ["args", "--filter=//", "--", "--script-arg=42"],


Good call on choosing filter as a name.

nathanhammond · 2022-08-02T17:13:13Z

cli/scripts/e2e/e2e.ts

+      const result = getCommandOutputAsArray(
+        repo.turbo(
+          "run",
+          ["args", "--filter=//", "--", "--script-arg=42"],


Can we do quite a few more variations on this with varying argument patterns? That'll likely make the rest of the stuff here need to become utility functions of some sort.

I'm thinking mostly edge cases:

-- -- --

``

-f --f ---f ----f

--same -- --same

--zero -- --one -- --two -- --three

-------------------------

In terms of test understandability, I'd love to be able to spec the input like this and then the output expected adjacent to it. (e.g. Go table tests)

We might should also create multiple scripts in package.json that log out argv with and without their own separator.

We might should also create multiple scripts in package.json that log out argv with and without their own separator.

Could you elaborate on this?

(Read in thread context.)

This is really an unnecessary, overly-defensive idea. Basically, we have a script called argv in the root package.json now that is just an argv echo service.

We could create variations on that script that had other -- setups to detect if node itself changes the way it works.

But that'll realistically break way more than us and I can't see that being something we truly need to test.

nathanhammond

This is a fixship!

The only open things remaining are something trivial. (e.g. all comments, naming, light refactoring, or just ... unimportant)
So, once you're happy with it, ship it!

nathanhammond · 2022-08-05T15:46:43Z

cli/internal/packagemanager/pnpm.go

+	// pnpm v7+ changed their handling of '--'. We no longer need to pass it to pass args to
+	// the script being run, and in fact doing so will cause the '--' to be passed through verbatim,
+	// potentially breaking scripts that aren't expecting it.
+	ArgSeparator: nil,


Let's add information about this being a typed nil ([]string{}) here.

nathanhammond · 2022-08-05T15:48:02Z

cli/internal/run/run.go

@@ -929,7 +914,7 @@ func (e *execContext) exec(ctx gocontext.Context, pt *nodes.PackageTask, deps da
 	argsactual := append([]string{"run"}, pt.Task)
 	if len(passThroughArgs) > 0 {
 		// This will be either '--' or a typed nil
-		argsactual = append(argsactual, e.argSeparator...)
+		argsactual = append(argsactual, e.packageManager.ArgSeparator...)


I love that the entire change is this single line.

nathanhammond · 2022-08-05T15:48:57Z

cli/scripts/e2e/e2e.ts

+        assert.equal(args, passedArgs);
+      };
+
+      const tests = [


This is perfect, thank you. ❤️

nathanhammond · 2022-08-05T15:50:19Z

cli/scripts/monorepo.ts

+    if (this.npmClient === "pnpm6") {
+      fs.mkdirSync(path.join(this.binDir, "node_modules"), { recursive: true });
+      execa.sync("npm", ["install", "--no-save", "pnpm@6.22.2"], {
+        cwd: this.binDir,
+      });
+    }


You're right, corepack is a better story.

nathanhammond · 2022-08-05T15:54:07Z

cli/scripts/e2e/e2e.ts

+      const result = getCommandOutputAsArray(
+        repo.turbo(
+          "run",
+          ["args", "--filter=//", "--", "--script-arg=42"],


(Read in thread context.)

This is really an unnecessary, overly-defensive idea. Basically, we have a script called argv in the root package.json now that is just an argv echo service.

We could create variations on that script that had other -- setups to detect if node itself changes the way it works.

But that'll realistically break way more than us and I can't see that being something we truly need to test.

nathanhammond · 2022-08-05T15:55:18Z

cli/scripts/monorepo.ts

@@ -184,6 +184,7 @@ importers:
          test: `${turboPath} run test`,
          lint: `${turboPath} run lint`,
          special: "echo root task",
+          args: 'node -e "console.log(process.argv)" --',


(Read in thread context.)
Good callout that node itself blows up here. Thanks for that! This is fine as is.

A quick and dirty switch to use corepack for managing package managers for our e2e tests. A few things to note: - Running these tests on a local machine will enable corepack on that machine, this could be undesired for some devs. - I found that it is necessary to forcibly add the corepack install dir to the top of $PATH in case a dev has a package manager installed elsewhere that would take priority. At the moment this is only useful for the Yarn/Berry split, but it would simplify #1612 which adds pnpm6 to e2e tests quite a bit.

The initial testing script had a hidden dependency on the length of the node path which would trigger the output to be spread over multiple lines. This lead to confusing logic around handling the case when it all fit onto one line. This commit changes the script to now always output all information on a single line.

chris-olszewski requested review from jaredpalmer, gsoltis, gaspar09, nathanhammond and tknickman as code owners August 1, 2022 22:01

chris-olszewski mentioned this pull request Aug 1, 2022

fix(Arg passing) #1608

Closed

chris-olszewski force-pushed the arg_passing_e2e_berry branch from 638729d to 12f6679 Compare August 1, 2022 22:04

chris-olszewski force-pushed the arg_passing_e2e_berry branch from e1d20ea to dcb919a Compare August 2, 2022 16:21

chris-olszewski requested a review from mehulkar as a code owner August 2, 2022 16:21

chris-olszewski force-pushed the arg_passing_e2e_berry branch from dcb919a to 09b0e06 Compare August 2, 2022 17:00

nathanhammond suggested changes Aug 2, 2022

View reviewed changes

chris-olszewski force-pushed the arg_passing_e2e_berry branch 4 times, most recently from 9a08855 to 69a0d66 Compare August 3, 2022 23:42

chris-olszewski mentioned this pull request Aug 4, 2022

switch e2e tests to use corepack #1632

Merged

nathanhammond added the pr: fixship A PR which is approved with notes and does not require re-review. label Aug 5, 2022

nathanhammond approved these changes Aug 5, 2022

View reviewed changes

chris-olszewski added 8 commits August 8, 2022 09:38

remove debug action from CI

0c6322a

add e2e test for arg parsing

efdae2a

add fix for berry arg passing

964d4a4

add additional test cases for arg passing

58001ed

add pnpm6 to e2e tests

1e0dd79

move pnpm 6/7 behavior split into new package manager

323f912

Add comments and delete unused code

a782a5f

chris-olszewski force-pushed the arg_passing_e2e_berry branch from 69a0d66 to a782a5f Compare August 8, 2022 17:19

Merge branch 'main' into arg_passing_e2e_berry

3fef1c3

chris-olszewski merged commit db0611d into vercel:main Aug 8, 2022

This was referenced Aug 9, 2022

Turbo is sending arg -- (ignore-option) as first argument to script. #1477

Closed

fix: refactor arg separator strategy #1507

Closed

renovate bot mentioned this pull request Aug 18, 2022

chore(deps) Update all non-major dependencies Arclight-Labs/Roe2#209

Merged

1 task

nathanhammond mentioned this pull request Aug 30, 2022

When parsing some client configs, Turbo is not aware of -- (ignore-option) #1476

Closed

fix(berry arg parsing) #1612

fix(berry arg parsing) #1612

Uh oh!

Conversation

chris-olszewski commented Aug 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vercel bot commented Aug 1, 2022

Uh oh!

jaredpalmer commented Aug 1, 2022

Uh oh!

nathanhammond left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nathanhammond Aug 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nathanhammond Aug 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nathanhammond left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nathanhammond Aug 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nathanhammond Aug 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chris-olszewski commented Aug 1, 2022 •

edited

Loading

nathanhammond Aug 5, 2022 •

edited

Loading

nathanhammond Aug 5, 2022 •

edited

Loading

nathanhammond Aug 5, 2022 •

edited

Loading

nathanhammond Aug 5, 2022 •

edited

Loading