search

actions / setup-node

Set up your GitHub Actions workflow with a specific version of node.js
MIT License
3.93k stars 1.3k forks source link

Corepack Support #531

Open RichiCoder1 opened 2 years ago

RichiCoder1 commented 2 years ago

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:

(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

dmitry-shibanov commented 2 years ago

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.

RichiCoder1 commented 2 years ago

@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!

tisonkun commented 2 years ago

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

brcrista commented 2 years ago

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?

tisonkun commented 2 years ago

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

brcrista commented 2 years ago

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

RichiCoder1 commented 2 years ago

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 commented 2 years ago

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

dsame commented 2 years ago

@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.

jakebailey commented 2 years ago

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.

tisonkun commented 2 years ago

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

brcrista commented 2 years ago

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.

RichiCoder1 commented 2 years ago

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.

simbo commented 2 years ago

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 commented 2 years ago

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:

yarinsa commented 2 years ago

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

RavenK1ll commented 2 years ago

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

tisonkun commented 2 years ago

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.

linghengqian commented 2 years ago
wmertens commented 2 years ago

@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).

SayakMukhopadhyay commented 1 year ago

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.

joequincy commented 1 year ago

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
SayakMukhopadhyay commented 1 year ago

@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).

cowwoc commented 1 year ago

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
SayakMukhopadhyay commented 1 year ago

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?

cowwoc commented 1 year ago

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.

Dr-Electron commented 1 year ago

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

styfle commented 1 year ago

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?)

IanVS commented 1 year ago

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?

trusktr commented 1 year ago

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. 🐔🥚

Methuselah96 commented 1 year ago

This is even more important now that Yarn no longer sets yarnPath by default in Yarn 4. When yarnPath is set it "just works" since the Yarn Classic binary that comes pre-installed on the GitHub runner images consults the yarnPath setting.

Mrquinn4 commented 1 year ago

Hello my name is Curtis was wondering if you could help me out with a problem on my code if you would get back in touch withme thank you

On Mon, Oct 23, 2023, 10:55 AM Nathan Bierema @.***> wrote:

This is even more important now that Yarn no longer sets yarnPath by default in Yarn 4 https://yarnpkg.com/blog/release/4.0#installing-yarn. When yarnPath is set it "just works" since the Yarn Classic binary that comes pre-installed on the GitHub runner images consults the yarnPath setting.

— Reply to this email directly, view it on GitHub https://github.com/actions/setup-node/issues/531#issuecomment-1775510622, or unsubscribe https://github.com/notifications/unsubscribe-auth/BAESIE4HXC4VBTD3W4PC3I3YA2HN3AVCNFSM52DT2FL2U5DIOJSWCZC7NNSXTN2JONZXKZKDN5WW2ZLOOQ5TCNZXGU2TCMBWGIZA . You are receiving this because you are subscribed to this thread.Message ID: @.***>

phiresky commented 1 year ago

Curtis,

Hello yes this is tech support how can we help you?

Regards Tech Support

koistya commented 12 months ago

actions/setup-node@v3 stopped working today, throwing the following error:

Error: This project's package.json defines "packageManager": "yarn@4.0.2".
       However the current global version of Yarn is 1.22.21.
nikolaik commented 12 months ago

actions/setup-node@v3 stopped working today, throwing the following error:

Error: This project's package.json defines "packageManager": "yarn@4.0.2".
       However the current global version of Yarn is 1.22.21.

Started seeing the same error today with cache: yarn too

Kubessandra commented 12 months ago

actions/setup-node@v3 stopped working today, throwing the following error:

Error: This project's package.json defines "packageManager": "yarn@4.0.2".
       However the current global version of Yarn is 1.22.21.

Same with cache: yarn

benquarmby commented 12 months ago

actions/setup-node@v3 stopped working today, throwing the following error...

@koistya: Please raise a separate GitHub issue for that error. It is not the same as the important feature request which is the subject of this issue. Please do not overload this discussion any more other than to post a link to that separate error report.

janpe commented 11 months ago

actions/setup-node@v3 stopped working today, throwing the following error:

Error: This project's package.json defines "packageManager": "yarn@4.0.2".
       However the current global version of Yarn is 1.22.21.

Started seeing the same error today with cache: yarn too

You should be able to fix this by adding the following env to your workflow SKIP_YARN_COREPACK_CHECK: true. But it's more of a workaround since having corepack in setup-node would be preferable.

naXa777 commented 11 months ago

@janpe so what's the correct and direct fix?

janpe commented 11 months ago

@janpe so what's the correct and direct fix?

I don't really have anything to add to my previous comment, but I'll repeat if I was unclear earlier.

So currently you have to add the following to fix your failing workflow

env:
  SKIP_YARN_COREPACK_CHECK: true

But hopefully in the future we'll see an option like proposed here added to setup-node: https://github.com/actions/setup-node/issues/480#issuecomment-1119303515

ildella commented 10 months ago

I make it work with actions v4 and yarn 4.x. The trick should be to run corepack enable before everything else. 

    steps:
    - uses: actions/checkout@v4
    - run: corepack enable
    - uses: actions/setup-node@v4
      with:
        node-version: ${{ matrix.node-version }}
        cache: 'yarn'
    - run: yarn install --immutable

The only issue I have is the cache is never found. This is my log

actions/setup-node@v4

node: v20.10.0
npm: 10.2.3
yarn: 4.0.2

/usr/local/bin/yarn --version
4.0.2
/usr/local/bin/yarn config get cacheFolder
/home/runner/.yarn/berry/cache
/usr/local/bin/yarn config get enableGlobalCache
true
yarn cache is not found
styfle commented 7 months ago

We might not need to change @actions/setup-node if this other PR gets merged:

But there is also a scenario where Corepack may stop working with future versions of Node.js if this PR gets merged:

Feel free to add 👍 or 👎 to the linked PRs.

rt-joe commented 6 months ago

Regarding the comment made above by @styfle with the PRs listed about node enabling corepack by default or removing corepack from node, I'm copy and pasting my reply from PR #901 that is complete and ready for review:

My 2 cents: those do seem far away from reaching social consensus (there's still ongoing debate as of hours ago), let alone from being integrated into node. With the latest active LTS (v20) having maintenance until April 2026 (and v22 active LTS just around the corner), I think corepack with node will be here to stay for a while.

I don't know if your comment was to imply that this PR should be held off on being merged, but if so, I think it solves a popular usecase for many users that won't go away anytime soon and can be properly addressed if/when the decision to include/exclude corepack in node is finalized.

samwightt commented 6 months ago

Kinda shocked that it's 2024 and this doesn't work with yarn anymore.

linghengqian commented 1 month ago
karfau commented 1 month ago

Even if corepack is no longer supported:

In order to support properly installing/caching the dependencies when the packageManager field is set in package.json, the package manager with the right version needs to be installed, right? All information required to do that is already present.

If this action would take care of it, it would reduce a lot of boilerplate from workflows that is currently required to use this action as soon as this field is configured to something else than the npm version coming with the given node version.

For the node versions that support corepack, it could be as easy as using it, for other node versions, or maybe even in general it would require the action to know about the install steps of the package managers on ubuntu.

So I think the main question is: Is supporting this feature out of scope for this action or not, independent of whether it is called corepack or not.

rt-joe commented 1 month ago

the current issue is trying to introduce a feature that is about to be removed, which is unreasonable.

Even if it were removed today, node v20 is supported until April 2026 and soon to be LTS v22 into 2027/2028 (idk exact date) and they both include corepack. There's been arguments for/against corepack for a long while, many of which were already mentioned in this issue. We've already wasted 2+ years (since at least this issue was opened, although corepack has been around much longer) without proper support despite the high popularity of this "experimental" feature. We can still get 2-3 more years of usage and easily remove it if/when it is finally decided to put corepack to rest which I'm guessing would be v24 at the earliest.

That being said, I agree with what @karfau said as well.

karfau commented 1 month ago

Has anybody attempted to create a PR for this?

Yes: https://github.com/actions/setup-node/pull/901

I think it should be fine to use the PR as is or with an "experimental feature" / "only works with node versions x-?" in the readme, and deal with the topic of extending the support in a different way, when the related node version is released.