-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Attempt at an alternative corepack implementation. #1656
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Attempt at an alternative corepack implementation. #1656
Conversation
@nathanhammond is attempting to deploy a commit to the Vercel Team on Vercel. A member of the Team first needs to authorize it. |
Hrm, this would handle CI, but doesn't handle running E2E locally. Then again, our local E2E already doesn't do the right thing so this is maybe "same same"? |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
function setupCorepack(): string { | ||
execa.sync("corepack", ["enable"]); | ||
const corepack_path = execa.sync("which", ["corepack"]).stdout; | ||
return path.dirname(corepack_path); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're getting rid of this, then we should add a section to CONTRIBUTING.md
that alerts users that they need to make sure their $PATH is correctly setup.
For example, with my setup this change causes make e2e
to hard fail (I do have corepack enabled):
olszewski@chriss-mbp cli % echo $PATH
/Users/olszewski/Library/pnpm:/opt/homebrew/bin:/opt/homebrew/sbin:/Users/olszewski/.nvm/versions/node/v16.16.0/bin:/opt/homebrew/bin:/opt/homebrew/sbin:/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:/opt/homebrew/bin:/opt/homebrew/sbin:/Users/olszewski/.nvm/versions/node/v16.16.0/bin:/usr/local/go/bin:/Users/olszewski/go/bin:/usr/local/go/bin:/Users/olszewski/go/bin
olszewski@chriss-mbp cli % make e2e
npm install -g corepack@latest
npm WARN config global `--global`, `--local` are deprecated. Use `--location=global` instead.
changed 1 package, and audited 2 packages in 440ms
found 0 vulnerabilities
corepack enable
pnpm install --filter=cli
.. | +5 +
.. | Progress: resolved 5, reused 5, downloaded 0, added 0, done
node -r esbuild-register scripts/e2e/e2e.ts
Error: Command failed with exit code 1: yarn install
error Couldn't find any versions for "b" that matches "workspace:*"
yarn install v1.22.19
[1/4] Resolving packages...
info Visit https://yarnpkg.com/en/docs/cli/install for documentation about this command.
at makeError (/Users/olszewski/code/vercel/turborepo/node_modules/.pnpm/execa@5.1.1/node_modules/execa/lib/error.js:60:11)
at Function.module.exports.sync (/Users/olszewski/code/vercel/turborepo/node_modules/.pnpm/execa@5.1.1/node_modules/execa/index.js:194:17)
at Monorepo.linkPackages (/Users/olszewski/code/vercel/turborepo/cli/scripts/monorepo.ts:162:17)
at Object.<anonymous> (/Users/olszewski/code/vercel/turborepo/cli/scripts/e2e/e2e.ts:52:8)
at Module._compile (node:internal/modules/cjs/loader:1105:14)
at Module._compile (/Users/olszewski/code/vercel/turborepo/node_modules/.pnpm/esbuild-register@3.3.3_esbuild@0.14.49/node_modules/esbuild-register/dist/node.js:2258:26)
at Module._extensions..js (node:internal/modules/cjs/loader:1159:10)
at Object.newLoader [as .ts] (/Users/olszewski/code/vercel/turborepo/node_modules/.pnpm/esbuild-register@3.3.3_esbuild@0.14.49/node_modules/esbuild-register/dist/node.js:2262:9)
at Module.load (node:internal/modules/cjs/loader:981:32)
at Function.Module._load (node:internal/modules/cjs/loader:822:12) {
shortMessage: 'Command failed with exit code 1: yarn install',
command: 'yarn install',
escapedCommand: 'yarn install',
exitCode: 1,
signal: undefined,
signalDescription: undefined,
stdout: 'yarn install v1.22.19\n' +
'[1/4] Resolving packages...\n' +
'info Visit https://yarnpkg.com/en/docs/cli/install for documentation about this command.',
stderr: `error Couldn't find any versions for "b" that matches "workspace:*"`,
failed: true,
timedOut: false,
isCanceled: false,
killed: false
}
make: *** [e2e] Error 1
olszewski@chriss-mbp cli % which yarn
/opt/homebrew/bin/yarn
olszewski@chriss-mbp cli % which pnpm
/opt/homebrew/bin/pnpm
olszewski@chriss-mbp cli % pnpm -v
7.8.0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary of a conversation that happened in Slack:
We decided that we should stay ahead of where things are going with corepack
and begin adoption of it. There exist weird $PATH
precedence issues that are not fun and it's still behind corepack enable
, but we believe that those issues will fade as adoption of corepack
increases and it moves toward being the default for node
dev environments.
It's imperfect, it will make things a little weird for contributors during the transition, but we believe that it's worth the churn and add or name to the "adopters of corepack
" list.
@@ -66,7 +66,11 @@ fmt-go: $(GO_FILES) go.mod go.sum | |||
install: | ./package.json | |||
pnpm install --filter=cli | |||
|
|||
e2e: install turbo | |||
corepack: | |||
npm install -g corepack@latest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line isn't needed since corepack ships with Node.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The version shipped with Node.js currently includes this bug which causes the first run of the e2e tests on a machine to fail
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So much cleaner and assuming devs have a good corepack setup seems reasonable.
corepack enable | ||
|
||
- name: Install dependencies | ||
run: pnpm install |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be cool if this was corepack -- install
. That way a change to packageManager
wouldn't require another change here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've got a secondary pnpm caching setup above this in the file that is also dependent upon this. This is what happens when we're semi-early adopters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(corepack
is not (yet?) in the business of delegating to package managers for certain functionality.)
npm install -g corepack@latest | ||
corepack enable | ||
|
||
e2e: corepack install turbo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm sure i missed this in a slack thread somewhere, but is this essentially pnpm install turbo
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope! It reads hilariously close to that; but it's actually the enumeration of tasks that need to be run in advance of running the e2e
task.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh... TIL. this is how Makefiles declare dependencies?
I don't actually know if this will work, but I was trying to make the system itself responsible for maintaining the executable paths so that we didn't need to account for it in our e2e scripts.