-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Comments
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 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! |
I use corepack locally and hope we can integrate it with GitHub Actions :) |
If I'm understanding correctly, right now you can run |
Thank @brcrista ! Could you share a minimal config to achieve this? I may not understand how to set up corepack manually here. |
I don't know either, I was just summarizing my understanding from the issue description. |
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 |
@brcrista and your understanding is correct. Ideally with some extra smarts enabled by knowing exactly which packageManager is in use. |
@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. 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. |
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. |
@dsame perhaps you can close this issue as not planned instead of completed? |
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. |
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:
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. |
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. |
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 The PR adds 3 little lines of code to the 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:
|
That would be great to have it as an input |
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 |
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 you're not even using setup-node... Ideally, setup-node optionally supports corepack, since Node optionally supports it. For example:
This would install Node 16, run |
You should be able to fix this by adding the following env to your workflow |
@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
But hopefully in the future we'll see an option like proposed here added to |
I make it work with actions v4 and yarn 4.x. 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
|
We might not need to change
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. |
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:
|
Kinda shocked that it's 2024 and this doesn't work with |
|
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? If this action would take care of it, 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: |
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. |
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. |
## 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 --> <!-- * Pull Request #issue -->
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:
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 rootpackage.json
(might be suprising),cache: 'auto'
as possibly envisioned in #306, orcorepack: 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
The text was updated successfully, but these errors were encountered: