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

Corepack Support #531

Open
RichiCoder1 opened this issue Jun 28, 2022 · 48 comments · May be fixed by #651 or #901
Open

Corepack Support #531

RichiCoder1 opened this issue Jun 28, 2022 · 48 comments · May be fixed by #651 or #901
Labels
feature request New feature or request to improve the current logic needs eyes

Comments

@RichiCoder1
Copy link

RichiCoder1 commented Jun 28, 2022

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 RichiCoder1 added feature request New feature or request to improve the current logic needs triage labels Jun 28, 2022
@dmitry-shibanov
Copy link
Contributor

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
Copy link
Author

RichiCoder1 commented Jun 28, 2022

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

@dsame dsame self-assigned this Aug 15, 2022
@tisonkun
Copy link

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

@brcrista
Copy link
Contributor

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
Copy link

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

@brcrista
Copy link
Contributor

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

@RichiCoder1
Copy link
Author

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
Copy link
Author

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

@dsame
Copy link
Contributor

dsame commented Aug 17, 2022

@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 dsame closed this as completed Aug 17, 2022
@jakebailey
Copy link

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
Copy link

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

@brcrista
Copy link
Contributor

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 brcrista reopened this Aug 17, 2022
@RichiCoder1
Copy link
Author

RichiCoder1 commented Aug 17, 2022

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
Copy link

simbo commented Aug 27, 2022

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
Copy link

simbo commented Aug 27, 2022

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 (Corepack Support #531, feature request: cache: 'auto' #306, Yarn is not installed #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.

@yarinsa
Copy link

yarinsa commented Sep 1, 2022

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

@RavenK1ll
Copy link

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
Copy link

tisonkun commented Sep 9, 2022

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
Copy link

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

@wmertens
Copy link

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

@janpe
Copy link

janpe commented Nov 20, 2023

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

Error: This project's package.json defines "packageManager": "[email protected]".
       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
Copy link

naXa777 commented Nov 21, 2023

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

@janpe
Copy link

janpe commented Nov 21, 2023

@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: #480 (comment)

@ildella
Copy link

ildella commented Dec 31, 2023

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
Copy link

styfle commented Mar 27, 2024

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
Copy link

rt-joe commented Apr 17, 2024

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.

xen0n added a commit to xen0n/areweloongyet that referenced this issue Apr 21, 2024
xen0n added a commit to xen0n/areweloongyet that referenced this issue Apr 21, 2024
@samwightt
Copy link

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

@linghengqian
Copy link

@karfau
Copy link

karfau commented Oct 9, 2024

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
Copy link

rt-joe commented Oct 9, 2024

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
Copy link

karfau commented Oct 9, 2024

Has anybody attempted to create a PR for this?

Yes: #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.

vscaiceanu-1a added a commit to AmadeusITGroup/otter that referenced this issue Nov 7, 2024
## Proposed change

In `node-setup` GitHub action, passing `cache: yarn` doesn't work well
with corepack (see issues below).

The goal is to do the cache manually for yarn and to let `node-setup` do
it for `npm`.

## Related issues

<!--
Please make sure to follow the [contribution
guidelines](https://github.com/amadeus-digital/Otter/blob/main/CONTRIBUTING.md)
-->

actions/setup-node#531

actions/setup-node#182

<!-- * 🐛 Fix #issue -->
<!-- * 🐛 Fix resolves #issue -->
<!-- * 🚀 Feature #issue -->
<!-- * 🚀 Feature resolves #issue -->
<!-- * :octocat: Pull Request #issue -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or request to improve the current logic needs eyes
Projects
None yet