setup-node icon indicating copy to clipboard operation
setup-node copied to clipboard

Corepack Support

Open RichiCoder1 opened this issue 2 years ago • 44 comments

Description: Support using https://github.com/nodejs/corepack to manage non-NPM package managers.

Ideally in between installing node and bootstrapping the package manager cache, this action:

  • enables corepack (could be a no-op depending on the node version)
  • caches/restores the corepack root
  • runs corepack prepare --active

(after some research, that last one may not necessary but instead makes an implicit step explicit)

This behavior could either be trigged by the presence of the packageManager field in the root package.json (might be suprising), cache: 'auto' as possibly envisioned in #306, or corepack: true

Justification: Both pnpm and yarn support corepack, and yarn recommendeds corepack for package manager versioning. Currently using corepack with this action will break when using the cache key as this action assumes the appropriate version manager has already been installed. However, you don't necessarily want to bootstrap with corepack until the appropriate node version has been installed and configured.

Related: #530 #182

Are you willing to submit a PR? Yes

RichiCoder1 avatar Jun 28 '22 21:06 RichiCoder1

Hello @RichiCoder1. Thank you for your feature request. Actually we had similar proposal to enable corepack by default. I think we should reinvestigate adding corepack support. We'll ping you about its results.

dmitry-shibanov avatar Jun 28 '22 22:06 dmitry-shibanov

@dmitry-shibanov apologies for not seeing that closed ticket! I very much hope ya'll do. While technically experimental, it's being officially recommended and there's no (current) way to use both this action and corepack without some awkward contortions. Looking forward to the outcome!

RichiCoder1 avatar Jun 28 '22 22:06 RichiCoder1

I use corepack locally and hope we can integrate it with GitHub Actions :)

tisonkun avatar Aug 16 '22 07:08 tisonkun

If I'm understanding correctly, right now you can run setup-node, set up Corepack, and then run the standalone cache action. So is the benefit of this just to be able to do it all inside the setup-node action?

brcrista avatar Aug 16 '22 15:08 brcrista

Thank @brcrista ! Could you share a minimal config to achieve this? I may not understand how to set up corepack manually here.

tisonkun avatar Aug 16 '22 16:08 tisonkun

I don't know either, I was just summarizing my understanding from the issue description.

brcrista avatar Aug 16 '22 19:08 brcrista

The semi-simple way w/ caching and something like pnpm would be:

- run: corepack enable
- uses: actions/setup-node@v3
  with:
    node-version: 16
    cache: pnpm
- run: pnpm i

I think that may be faintly fragile as corepack enable operates on the built in node and not the potentially installed one. It does however fix an issue where setup-node will look for a pnpm exe/cache that doesn't exist yet when setting up cache I think.

RichiCoder1 avatar Aug 16 '22 19:08 RichiCoder1

@brcrista and your understanding is correct. Ideally with some extra smarts enabled by knowing exactly which packageManager is in use.

RichiCoder1 avatar Aug 16 '22 20:08 RichiCoder1

@RichiCoder1 thanks for you notification, it was a good point to review, but we should not go ahead of nodejs team and turn on corepack by default while it is not default for the nodejs itself. The consequences of having corepack enabled are risky for some builds and adding new input is not necessary complication of the action and its documentation. In the current state of the corepack to have an explicit simple step enabling the feature seems to be optimal solution.

I have to reject the feature request and close the issue, but please feel free to reopen it or create new one in case you still have some problems.

dsame avatar Aug 17 '22 06:08 dsame

That's unfortunate; I avoid using the cache feature of the action because I'm nervous of mixing different major npm versions' caches. Having a corepack opt-in would compose well there, but I guess I'll just keep using the 4 steps (!) of setup-node, enable corepack, get cache location, cache, or keep mixing the npm version into the cache key.

jakebailey avatar Aug 17 '22 06:08 jakebailey

@dsame perhaps you can close this issue as not planned instead of completed?

tisonkun avatar Aug 17 '22 06:08 tisonkun

We can leave this open to see if it continues to attract attention. We're not going to do anything about it right now for the reasons that @dsame shared. When Node makes Corepack the default, we can revisit it.

brcrista avatar Aug 17 '22 13:08 brcrista

There is the perfectly fine alternative for now to just have corepack be opt in. Lets people use it without conflicting with the action, and makes flipping the switch later easy.

Edit: I missed this statement:

adding new input is not necessary complication of the action and its documentation. In the current state of the corepack to have an explicit simple step enabling the feature seems to be optimal solution

I strongly disagree. As noted above, this doesn't account for this action installing an alternative Node (and possibly breaking the enablement) and it doesn't solve caching whatsoever.

RichiCoder1 avatar Aug 17 '22 15:08 RichiCoder1

I strongly agree with @RichiCoder1 and would even go for a fork of setup-node with corepack support to achieve this goal.

For yarn and pnpm users there seem to be no reliable workflow to use setup-node with proper caching at the moment.

simbo avatar Aug 27 '22 00:08 simbo

Please also keep in mind, that for GitHub Enterprise users there is in most cases neither node preinstalled on runners nor are they able to customize their runners because they are managed by company IT.

I just made a PR to fix the tests for the PR of BeeeQueue.

This would add optional corepack support by simply setting corepack: true in the action inputs.

The PR adds 3 little lines of code to the main.ts of setup-node and one new input which is easy to understand and should not overcomplicate things. The documentation and action manifest update is done and the new code is also covered in tests.

And as a bonus - as corepack is completely optional - this feature is not a breaking change and could be published right away as a minor version update for v3.

In regards to attention on this topic:

  • there are currently 2 open PRs who are trying to bring corepack support to setup-node.
  • there are 3 open issues regarding corepack support (https://github.com/actions/setup-node/issues/531, https://github.com/actions/setup-node/issues/306, https://github.com/actions/setup-node/issues/182) where 2 of them have ongoing and recent activity.
  • and you can be sure there are a lot of yarn and pnpm users out there as well as users of self-hosted runners with GitHub Enterprise who would be very thankful if this feature would be added.

simbo avatar Aug 27 '22 12:08 simbo

That would be great to have it as an input "corepack: true" As each of my actions have to do "corepack enable"

yarinsa avatar Sep 01 '22 12:09 yarinsa

Ok, bare with me everyone, I have been dealing with this for some time now I an just getting back to my device. I might need assistance

RavenK1ll avatar Sep 01 '22 12:09 RavenK1ll

I notice that I can achieve this mechanism simply with:

      - name: Enable Corepack
        run: corepack enable

Thus, I'm OK with current status and don't find this issue a blocker to me :)

See this workflow as an example.

tisonkun avatar Sep 09 '22 04:09 tisonkun

  • I am surprised that this is actually an issue, corepack is still in the experimental stage and of course needs to be manually enabled. I myself have been using corepack enable && corepack prepare [email protected] --activate.

  • This issue should actually be closed and wait for nodejs to enable corepack by default.

linghengqian avatar Sep 09 '22 09:09 linghengqian

@tisonkun you're not even using setup-node...

Ideally, setup-node optionally supports corepack, since Node optionally supports it. For example:

- uses: actions/setup-node@v4
  with:
    node-version: 16
    corepack: true
    cache: true
    install: true

This would install Node 16, run corepack enable, read the packageManager field, set up the cache for that, and then call the package manager in the correct way for CI (npm ci, pnpm i etc).

wmertens avatar Oct 21 '22 08:10 wmertens

Hi all. I faced the same issue today and seeing as how #546 is kinda abandoned, I decided to write this feature myself. I am opening up a new PR for the same. All tests are passing in this PR and the option to enable corepack is, well, optional.

@brcrista As mentioned, this PR will add the "option" to enable corepack. As such, I don't think we will go ahead of the NodeJs team. Yes, it is exeperimental but then again we aren't asking to enable this option by default. Just allow us to use it if we need it.

SayakMukhopadhyay avatar Dec 26 '22 17:12 SayakMukhopadhyay

I'm also chiming in with interest. For some CI tasks my org has some private runners that my team does not control and which do not have node or yarn installed by default. Because of this, we cannot use cache: yarn within actions/setup-node since the action will fail when running yarn cache dir to get the cache location. Instead we have to address this chicken/egg problem by manually caching with actions/cache like

      - name: Setup Node
        uses: actions/setup-node@v3
        with:
          node-version-file: .nvmrc
      - name: Setup Yarn
        run: npm i -g yarn
      - name: Get yarn cache directory path
        id: yarn-cache-dir-path
        run: echo "dir=$(yarn cache dir)" >> $GITHUB_OUTPUT
      - uses: actions/cache@v3
        with:
          path: ${{ steps.yarn-cache-dir-path.outputs.dir }}
          key: ${{ runner.os }}-yarn-${{ hashFiles('**/yarn.lock') }}
          restore-keys: |
            ${{ runner.os }}-yarn-

It would be preferable to instead be able to set up node with caching, and ensure the desired package manager is present just this action, which might look like (to borrow the syntax proposed in #651):

      - name: Setup Node
        uses: actions/setup-node@v3
        with:
          node-version-file: .nvmrc
          cache: yarn
          corepack: yarn

joequincy avatar Jan 13 '23 23:01 joequincy

@brcrista any chance that the PR #651 gets accepted? If there is a chance, I will fix the PR to handle the latest changes on main. As far as interest goes, imo there is a lot of it in here. This would anyway be an opt-in option so I don't see why there should be an issue with implementing the PR.

Also corepack is used by many other systems like Vercel and the corepack development team itself is very confident on the capability of corepack (see https://github.com/nodejs/corepack/issues/104).

SayakMukhopadhyay avatar Jan 31 '23 08:01 SayakMukhopadhyay

This workaround works for me (using yarn 3.x):

      - name: Install node
        uses: actions/setup-node@v3
        with:
          node-version: latest

      - name: Install Yarn
        run: corepack enable |
          corepack prepare yarn@stable --activate

      # Yarn dependencies cannot be cached until yarn is installed
      # WORKAROUND: https://github.com/actions/setup-node/issues/531
      - name: Extract cached dependencies
        uses: actions/setup-node@v3
        with:
          cache: yarn

      - name: Update dependencies
        run: corepack yarn --immutable

cowwoc avatar Feb 04 '23 18:02 cowwoc

This workaround works for me (using yarn 3.x):

      - name: Update dependencies
        run: corepack yarn --immutable

Looks interesting. So, if I get it right, the setup-node action needs to be invoked twice for the workaround to work. Still the last line seems odd. Is the command to run is corepack yarn --immutable or corepack yarn install --immutable? In either case, is it necessary to invoke yarn via corepack?

SayakMukhopadhyay avatar Feb 04 '23 19:02 SayakMukhopadhyay

if I get it right, the setup-node action needs to be invoked twice for the workaround to work.

Correct.

Still the last line seems odd. Is the command to run is corepack yarn --immutable or corepack yarn install --immutable? In either case, is it necessary to invoke yarn via corepack?

Per https://yarnpkg.com/getting-started/usage install is implied if the command name is omitted.

Looking at https://yarnpkg.com/getting-started/install, you are right, we can invoke yarn directly instead of corepack yarn. I just tested it and it works.

cowwoc avatar Feb 04 '23 19:02 cowwoc

I started to use corepack nearly everywhere. Would be nice to get direct support in setup-node ❤️

Dr-Electron avatar May 09 '23 21:05 Dr-Electron

Can we get more eyes on #651?

Its not clear why this hasn't been merged.

Perhaps we need a better "experimental" disclaimer in case there are breaking changes in the future? (or is that implied?)

styfle avatar Aug 01 '23 15:08 styfle

I was really surprised this wasn't already working, or at least supported through an option. I see a concern above about not making it the default until node.js does, but what is the objection to adding an opt-in option to enable corepack?

IanVS avatar Aug 23 '23 14:08 IanVS

Yarn's documentation shows corepack enable as the only way to install Yarn other than from git source (yarn set version from sources). Seems like this basically is required for Yarn, despite it being experimental. 🐔🥚

trusktr avatar Oct 21 '23 04:10 trusktr