-
-
Notifications
You must be signed in to change notification settings - Fork 16
ci: run corepack enable before installing dependencies
#244
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
Conversation
1a1f680 to
975cb9f
Compare
…n v4 before deps install - makes nodejs version `lts/*` explicit in ci workflow
975cb9f to
69e2b16
Compare
corepack enable as part of preparecorepack enable before installing dependencies
Not sure I understand. We're using Yarn 3 and 4 in several repos without problems. |
| uses: actions/setup-node@v4 | ||
| with: | ||
| node-version-file: '.nvmrc' | ||
| node-version: 'lts/*' |
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.
Should we not prefer the version in .nvmrc?
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.
According to the PR description, we are now running this first before checking out, so we don't have access to .nvmrc anymore.
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.
Yeah, I figured if we can't use it consistently for a job in a workflow, it's preferred to be consistent within the workflow.
| - name: Use Node.js | ||
| uses: actions/setup-node@v4 | ||
| with: | ||
| node-version-file: '.nvmrc' | ||
| node-version: 'lts/*' | ||
| - name: Install Yarn | ||
| run: corepack enable | ||
| - uses: actions/checkout@v4 | ||
| - name: Use Node.js and install dependencies | ||
| uses: actions/setup-node@v4 | ||
| with: | ||
| node-version: 'lts/*' |
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 looks like corepack may be already installed on ubuntu-latest? The reason I say that is that I'm reviewing solutions others have posted in the actions/setup-node ticket, specifically actions/setup-node#480 (comment), and it seems like we could do this without having to install Node twice:
| - name: Use Node.js | |
| uses: actions/setup-node@v4 | |
| with: | |
| node-version-file: '.nvmrc' | |
| node-version: 'lts/*' | |
| - name: Install Yarn | |
| run: corepack enable | |
| - uses: actions/checkout@v4 | |
| - name: Use Node.js and install dependencies | |
| uses: actions/setup-node@v4 | |
| with: | |
| node-version: 'lts/*' | |
| - uses: actions/checkout@v4 | |
| - name: Install Yarn via Corepack | |
| run: corepack enable | |
| - name: Use Node.js and install dependencies | |
| uses: actions/setup-node@v4 | |
| with: | |
| node-version: '.nvmrc' |
Perhaps you tried this already, but, thoughts?
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 was also traversing the chain of various attempts at adding Corepack support to setup-node, and apparently according to this comment Yarn is already installed on GitHub runners somehow, so we may wish to remove it so that the shim that Corepack installs doesn't conflict:
| - name: Use Node.js | |
| uses: actions/setup-node@v4 | |
| with: | |
| node-version-file: '.nvmrc' | |
| node-version: 'lts/*' | |
| - name: Install Yarn | |
| run: corepack enable | |
| - uses: actions/checkout@v4 | |
| - name: Use Node.js and install dependencies | |
| uses: actions/setup-node@v4 | |
| with: | |
| node-version: 'lts/*' | |
| - uses: actions/checkout@v4 | |
| - name: Uninstall existing Yarn | |
| run: npm -g uninstall yarn | |
| - name: Install Yarn via Corepack | |
| run: corepack enable | |
| - name: Use Node.js and install dependencies | |
| uses: actions/setup-node@v4 | |
| with: | |
| node-version: '.nvmrc' |
Then again, this doesn't make any sense since it would imply that npm is always preinstalled on GitHub runners. Just a suggestion though in case it works.
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 don't think this is directly related to here, in any case? Recall this has always been the case and we rely on @setup/node to override anything present in the local env. I suggest addressing any removal of already installed stuff separately from this PR.
(I also suspect that the corepack enable in the previous step will be done uneffective by the setup-node replacing local nodejs. But you're welcome to try variations as well)
| needs: | ||
| - prepare | ||
| steps: | ||
| - name: Use Node.js |
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
|
Thanks! I'll go ahead and merge this into my branch. |
corepackis not currently supported in@actions/setup-nodeand this means installation of dependencies fail when using yarn >v1:This hacks around it by running
@actions/setup-nodetwice: once before checkout to install the node-version, so thatcorepack enablecan be run, after which the normal flow starting with checkout can proceed.Since
.nvmrcis not available before checkout, it also implies not using.nvmrcto specify node-version for this workflow anymore.An alternative to this would be forking
@actions/setup-nodewith the patch from the PR linked above but sounds not worth it.