Skip to content
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

ci: run corepack enable before installing dependencies #244

Merged
merged 2 commits into from
Apr 29, 2024

Conversation

legobeat
Copy link
Contributor

@legobeat legobeat commented Apr 25, 2024

corepack is not currently supported in @actions/setup-node and this means installation of dependencies fail when using yarn >v1:

This hacks around it by running @actions/setup-node twice: once before checkout to install the node-version, so that corepack enable can be run, after which the normal flow starting with checkout can proceed.

Since .nvmrc is not available before checkout, it also implies not using .nvmrc to specify node-version for this workflow anymore.

An alternative to this would be forking @actions/setup-node with the patch from the PR linked above but sounds not worth it.

@legobeat legobeat closed this Apr 25, 2024
@legobeat legobeat reopened this Apr 26, 2024
@legobeat legobeat force-pushed the corepack-enable branch 2 times, most recently from 1a1f680 to 975cb9f Compare April 26, 2024 00:09
@legobeat legobeat requested a review from mcmire April 26, 2024 00:12
@legobeat legobeat marked this pull request as ready for review April 26, 2024 00:12
@legobeat legobeat requested a review from a team as a code owner April 26, 2024 00:12
…n v4 before deps install

- makes nodejs version `lts/*` explicit in ci workflow
@legobeat legobeat changed the title ci: run corepack enable as part of prepare ci: run corepack enable before installing dependencies Apr 26, 2024
@Mrtenz
Copy link
Member

Mrtenz commented Apr 26, 2024

this means installation of dependencies fail when using yarn >v1:

Not sure I understand. We're using Yarn 3 and 4 in several repos without problems.

- name: Use Node.js
uses: actions/setup-node@v4
with:
node-version-file: '.nvmrc'
node-version: 'lts/*'
Copy link
Member

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?

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Comment on lines 11 to +21
- 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/*'
Copy link
Contributor

@mcmire mcmire Apr 26, 2024

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:

Suggested change
- 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?

Copy link
Contributor

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:

Suggested change
- 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.

Copy link
Contributor Author

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)

@@ -23,11 +29,17 @@ jobs:
needs:
- prepare
steps:
- name: Use Node.js

This comment was marked as duplicate.

@legobeat legobeat requested review from mcmire and Mrtenz April 26, 2024 18:10
@mcmire mcmire merged commit 2cd37d1 into MetaMask:remove-yarn-binary Apr 29, 2024
14 checks passed
@mcmire
Copy link
Contributor

mcmire commented Apr 29, 2024

Thanks! I'll go ahead and merge this into my branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants