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

build: enable yarn and pnpm Corepack binaries by default #51886

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Feb 26, 2024

Fixes: #50963

Last version of Corepack has addressed (some of) the major concerns that was discussed in the TSC meeting last time:

IMO having Corepack enabled by default will overall be a clear win for Yarn and PNPM users (and won't affect npm users). Having it enabled by default does not stop those who don't like the "jumper binary" pattern to install Yarn and PNPM as they see fit.

@aduh95 aduh95 added the semver-major PRs that contain breaking changes and should be released in the next major version. label Feb 26, 2024
@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. python PRs and issues that require attention from people who are familiar with Python. labels Feb 26, 2024
@aduh95 aduh95 added the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Feb 26, 2024
Copy link
Member

@MoLow MoLow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@richardlau
Copy link
Member

If we land this then we probably also need to add the additional shims to tools/msvs/msi/nodemsi/product.wxs.

Copy link
Member

@GeoffreyBooth GeoffreyBooth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Corepack needs to support npm in the same ways that it supports Yarn and pnpm, if Corepack is to remain included with Node’s distribution. While Corepack was experimental and disabled by default this incompatibility was something that could be ignored as a TODO to be solved later, but with the current proposal to enable it by default (for all intents and purposes, making Corepack stable) the time has come to reconcile its relationship with npm.

In particular, the members of the TSC who met on 2024-01-24 asked the Corepack champions to take specific steps to align design goals: #50963 (comment). This work has not yet been done. Until it has, I don’t think there should be any movement toward making Corepack stable or enabled by default.

@jasnell
Copy link
Member

jasnell commented Feb 26, 2024

#51888, if Corepack is to remain included with Node’s distribution

I disagree. Having npm and corepack work together is an orthogonal concern. There is no impact to npm users if corepack is enabled by default. Recognizing that there still may be work remaining to do, these things can be done incrementally I don't see them as blockers here.

@GeoffreyBooth
Copy link
Member

GeoffreyBooth commented Feb 26, 2024

There is no impact to npm users if corepack is enabled by default.

If Corepack is to ever support npm in the ways that npm’s maintainers desire, then Corepack itself will likely need to undergo significant changes. It may need to drop the version pinning or jumper binaries, for example, which would be a dramatic change that would break the majority of Corepack’s current users (pnpm users who are pinning precise versions to avoid corrupting lockfiles).

I see enabling Corepack by default as equivalent to dropping an experimental flag. It’s a sign that we’re moving an experimental feature into the “release candidate” stage, where we don’t anticipate any further major API changes. But unless we’re okay with simply never fully supporting npm, Corepack needs significant changes. And shipping major changes to a feature that doesn’t require opt-in is bad UX. We should sort out these things while Corepack is still experimental and still disabled by default.

@anonrig
Copy link
Member

anonrig commented Feb 26, 2024

There is, because if Corepack is to ever support npm in the ways that npm’s maintainers desire, then Corepack itself will likely need to undergo significant changes.

From what I've read on Github and from private discussions, NPM does not want to be shipped with Corepack. NPM wants to be shipped with Node.js itself. It was clearly communicated in previous Github issues.

We shouldn't block the progress of adding support for other package managers just because NPM isn't interested, or it's not in their roadmap or it's not in their benefit.

@GeoffreyBooth
Copy link
Member

NPM does not want to be shipped with Corepack

So maybe Corepack doesn’t need to ship package managers. Or maybe Corepack downloads Yarn and pnpm on demand but simply errors on the wrong version of npm. We don’t have defined goals of Corepack; what is a core feature of Corepack and what isn’t? If Corepack drops the downloading ability, would it still be achieving its goals? What if it drops the version pinning?

My point is still that we should work all these things out before making Corepack near-stable. There’s still major uncertainty around Corepack. The fact that we’re considering enabling this by default before even making the effort to define what Corepack’s goals are feels very wrong to me.

@mhdawson
Copy link
Member

mhdawson commented Feb 26, 2024

@aduh95 I know that a prompt was added in nodejs/corepack#360. What I'm not sure of is how "by default it's only shown when "not using the corepack binary" (i.e. when using the binaries created by corepack enable)" affects it's use in Node.js.

I want to be sure that if corepack is enabled by default that the user will see the prompt the first time they try to use yarn, pnpm etc.

Assuming that they will see the prompt, I think we need an addition somewhere in the Node.js documentation which explains that despite corepack being present to ease the installation of these tools, they are not part of the Node.js distribution and:

  • the version installed is the latest at the time of use
  • any required updates (related to security vulnerabilities or otherwise) are out of scope of the Node.js project. If necessary end users must figure out how to update on their own.

In addition we should have an addition to the https://github.com/nodejs/node/blob/main/SECURITY.md#the-nodejs-threat-model which indicates that any vulnerabilities in package managers installed through corepack are outside of the Node.js threat model and why.

@mcollina
Copy link
Member

I’m currently -1, pending a solution to nodejs/corepack#399. I think this can be solved quickly if there is agreement.

——

@GeoffreyBooth I think we can reconcile the conundrum by saying that npm will stay as it is, and it has a special relationship with Node.js. I agree that something in the corepack design docs has to change.

@GeoffreyBooth
Copy link
Member

I think we can reconcile the conundrum by saying that npm will stay as it is, and it has a special relationship with Node.js. I agree that something in the corepack design docs has to change.

Surely there should be consensus that we need updated design docs, both on the Node and Corepack sides, before we consider enabling Corepack by default. Even if the updated docs say that npm is special and nothing that Corepack does applies to it, that feels like something that we should spell out and land using the regular PR process. I don’t understand why we would be rushing toward stability when there’s still basic consensus-seeking work to be done.

Personally I don’t think that npm should be special. If we ship both npm and Corepack, as a user I expect them to work well together and provide the same functionality as is provided for other package managers. Inconsistency is bad UX. My preference would be that the people working on Corepack devote some effort into resolving this so that Corepack can work well for all of our users, not just Yarn and pnpm users.

@jasnell
Copy link
Member

jasnell commented Feb 27, 2024

If Corepack is to ever support npm in the ways that npm’s maintainers desire, then Corepack itself will likely need to undergo significant changes. It may need to drop the version pinning or jumper binaries, for example, which would be a dramatic change that would break the majority of Corepack’s current users (pnpm users who are pinning precise versions to avoid corrupting lockfiles)....

Many changes may be needed by lots of things over time. We don't need to solve everything before making progress. We can make breaking changes in semver-major commits at any time. The idea that we might need to make major changes at some point is no reason not to make progress now.

@GeoffreyBooth
Copy link
Member

Many changes may be needed by lots of things over time. We don’t need to solve everything before making progress. We can make breaking changes in semver-major commits at any time. The idea that we might need to make major changes at some point is no reason not to make progress now.

This argument makes no sense to me. Generally we don’t unflag features if we anticipate major changes. That’s what I see here: Corepack will need major changes to support npm; it should support npm; therefore it should remain disabled by default while it undergoes the significant churn required to support npm.

The lack of outreach to the npm team even after requests from the TSC leads me to suspect that if Corepack is unflagged without npm support, then npm support will never happen. Therefore I think it’s all the more vital that we hold the line here until the design doc work and npm support work is complete.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Marking my -1 explicit depending on the fix in nodejs/corepack#399. The lack of reproducible builds by default is too high of a risk to mark as enabled by default. It's also a clear defense against supply chain attacks etc.

(I'm positive on lgtm'ing this PR once that is solved, I think this should happen ASAP. I'm currently flagging a major issue for the "newbie" use case).

Copy link
Member

@ruyadorno ruyadorno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I mentioned in the last TSC call in which we discussed this, I believe it's important to have an "Alternatives Considered" writeup in the Corepack docs and/or readme in which the team documents the reason why the Node.js runtime should ship Corepack by default and why it should be enabled by default vs alternatives such as using asdf, Volta, Docker, Corepack as a separate binary or any other environment wrapper/manager solution.

@GeoffreyBooth
Copy link
Member

I reached out to the npm team. They are interested in finding a solution that can work for them as well as Yarn and pnpm: #51888 (comment)

Such a solution would probably mean changing or replacing the packageManager field, which seems to be under consideration anyway: nodejs/corepack#402 (comment)

I think we should let both efforts play out before we unflag Corepack.

@nickmccurdy
Copy link

nickmccurdy commented Feb 27, 2024

@GeoffreyBooth Couldn't npm just be another valid package manager in packageManager in this case?

For example:

"packageManager": "npm@10.4.0"

@GeoffreyBooth
Copy link
Member

Couldn’t npm just be another valid package manager in packageManager in this case?

It could if the npm team would be willing to support that syntax, but they’re not. They want something that defines version ranges, not a particular version; what should happen when validation fails; and other concerns. We’ve started sketching out an alternative configuration block in openjs-foundation/package-metadata-interoperability-collab-space#15.

@MylesBorins
Copy link
Member

One thing that was called out that still has not been clarified is

  1. How do new package managers get added to Corepack
  2. How do they get turned on by default in Node.js

The current documentation simply says open a PR and it will land and it can become part of the default list after a couple of months. There is no documentation at all regarding how those package managers end up shipping in Node.js

This doesn't seem like explicit enough governance for something that will result in jumper binaries being installed on every system that installs Node.js

@arcanis
Copy link
Contributor

arcanis commented Feb 28, 2024

Such a solution would probably mean changing or replacing the packageManager field, which seems to be under consideration anyway: nodejs/corepack#402 (comment)

I don't foresee the packageManager field changing. You're free to debate between yourselves of another syntax to add on top of it if you feel so strongly about it, but the current one has been proved to work well for the past years, has support from both package managers Corepack was initially intended for, and is already integrated in various places. I will strongly push back on removing or deprecating it. I trust you to know how to expand a design without breaking it.

The current documentation simply says open a PR and it will land and it can become part of the default list after a couple of months. There is no documentation at all regarding how those package managers end up shipping in Node.js

What do you suggest more than what is the current status for PRs to be merged in this repository? ie.

Two collaborators must approve a pull request before the pull request can land. (One collaborator approval is enough if the pull request has been open for more than 7 days.) Approving a pull request indicates that the collaborator accepts responsibility for the change. Approval must be from collaborators who are not authors of the change.

If a collaborator opposes a proposed change, then the change cannot land. The exception is if the TSC votes to approve the change despite the opposition. Usually, involving the TSC is unnecessary. Often, discussions or further changes result in collaborators removing their opposition.

@trivikr
Copy link
Member

trivikr commented Mar 5, 2024

I would appreciate augmenting or removing that statement unless you personally disagree with what I'm saying.

I've removed the statement from my comment, and also sent a self-report to Node.js moderation team.

@@ -20,10 +20,14 @@
<String Id="NodeRuntime_Description" Value="Install the core [ProductName] runtime (node.exe)."/>

<String Id="npm_Title" Value="npm package manager"/>
<String Id="npm_Description" Value="Install npm, the recommended package manager for [ProductName]."/>
<String Id="npm_Description" Value="Install npm, a package manager for [ProductName]."/>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change does not relate to enabling Corepack by default (ie. we can recommend npm even though Corepack is being distributed). I'd suggest making a separate PR so we can discuss this change independently.

Suggested change
<String Id="npm_Description" Value="Install npm, a package manager for [ProductName]."/>
<String Id="npm_Description" Value="Install npm, the recommended package manager for [ProductName]."/>

Copy link
Member

@wesleytodd wesleytodd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I strongly urge folks, the @nodejs/tsc members specifically, to consider how contentious this issue is and go slow on a decision. There a group of folks working to propose a concrete way forward which addresses both the needs from the corepack team and the other open feedback. I strongly believe that shipping this PR too soon is locking the ecosystem into supporting a subpar solution for a long time. I am not asking to unilaterally stop this, just not to vote on this too soon and with incomplete context. Also, I am not adding new technical context here, that has all been said, but I am appealing to your care for the ecosystem and the contributors who are deeply invested in this part of the ecosystem who are opposed to the shipping this proposal.

Copy link
Contributor

@Ethan-Arrowood Ethan-Arrowood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it is appropriate to enable corepack by default at this time. There are numerous concerns about it that have not been addressed; furthermore, it still breaks certain npm and pnpm patterns.

@arcanis
Copy link
Contributor

arcanis commented Mar 5, 2024

Maintaining Corepack isn’t just fixing bugs and adding features, it’s also treating the npm, Yarn and pnpm teams as valued stakeholders who require consultation. I’ve been doing that outreach over the last few days, but I’m not a member of the Corepack team and I don’t intend to act in this role for much longer.

I'll perhaps be a little more direct than I should, but I very rarely felt as invisible as during this past week. If you attempted to have an outreach role, I'm afraid you failed at that: I personally feel you've ignored signals from my team, Zoltan had to reiterate his positions despite stating he didn't wish to be dragged into this discussion again, and the npm team represented by Myles repeatedly had to restate they were fine with Corepack as well. Or do you think this is unclear?

We have explicitly supported the inclusion of yarn + pnpm, offering multiple ways to reach that goal. I've personally advocated for both directly vendoring as well as including corepack.

On the other hand we have you who decided to run a crusade noone in the three package manager teams understands, and gave yourself a mandate to propose an alternative that noone in the three package manager teams requested, or even supported. You say you want to treat us as valued stakeholders, and proceed to ignore our work. You talk about healthy relationships, and proceed to mischaracterize our words in the very same sentence. How is that coherent?

I don't understand how this thread could derail so much without noone intervening, and it seems to me a significant miss from a governance and/or moderation point of view.

@ovflowd
Copy link
Member

ovflowd commented Mar 5, 2024

After reading all the comments, and as a (relatively) outsider who hasn't been participating in these Corepack conversations, I feel that this PR is becoming unnecessarily emotional and throwing blows in directions that aren't called for or welcomed by our Community.

I've read many discussions and threads of Corepack (recent and old ones) before deciding to write this commentary here: we are making a rushed decision on enabling Corepack.

Listen, I do use Corepack and find it incredible. In most cases, it works out of the box, and as an average user who simply wants an efficient way of enabling PNPM or YARN without using third scripts, it is a great choice.

Yet, I do acknowledge Darcy's, Geoffrey's, Wes's, and many other concerns (which actually do make a lot of sense, at least in my humble opinion) that Corepack, in its current state, seems at least unstable and has a few gaps on its documentation/governance (package manager adoption, version resolution, installation path, integrity, etc)

These concerns raise the question: Should we enable Corepack by default now? Honestly, what is not working right now that would require us to proceed with this PR? As mentioned before, with Corepack disabled by default, you can still use it regularly, as it is enabled at the moment of invocation. And that's great since I only want to have Corepack install or download binaries upon my explicit request.

I agree that the community is asking for better ways to use diverse package managers with Node.js, and I agree with Matteo's statement, "It doesn't need necessarily to be Corepack." That statement is vital because it reverberates with a sentiment I share: that we, as core collaborators, are here to serve the community. Yet, at the same time, to better serve the community, we need to make well-thought technical decisions correctly, and sometimes, we wait for specific pieces of technology to reach maturity or the so-called consensus-seeking process. I doubt that Darcy and other people objecting to this are doing it for the sake of being against Corepack, and it saddens me that some collaborators go to the lengths of pointing fingers and escalating the situation unnecessarily.

Although we might differ on when Corepack should be enabled, we all agreed that we are seeking a long-term solution that solves the issue of "allowing Node.js to bundle or seamlessly provide package managers in a uniform approach. Be that Corepack or not.

I genuinely ask the collaborators involved in this PR to take a small step back, relax, and figure things out together. I believe we can reach a consensus and work together to release a stable "Corepack."

Having said that, I feel there's no immediate need for Corepack to become enabled by default. Again, it doesn't change the current status quo, but it forces the functionality into a "stable" status, which should honestly not be warranted at the moment.

I understand that many people put a lot of effort here. Trust me, I definitely understand what it means to take a reasonable amount of time to craft something desirable. But if Corepack has been in the works for a few years already, addressing these concerns in a few extra months... Wouldn't hurt, would it?

Peace 🤞

@ovflowd
Copy link
Member

ovflowd commented Mar 5, 2024

We have explicitly supported the inclusion of yarn + pnpm, offering multiple ways to reach that goal. I've personally advocated for both directly vendoring as well as including corepack.

On the other hand we have you who decided to run a crusade noone in the three package manager teams understands, and gave yourself a mandate to propose an alternative that noone in the three package manager teams requested, or even supported. You say you want to treat us as valued stakeholders, and proceed to ignore our work. You talk about healthy relationships, and proceed to mischaracterize our words in the very same sentence. How is that coherent?

I don't understand how this thread could derail so much without noone intervening, and it seems to me a significant miss from a governance and/or moderation point of view.

Hi @arcanis, could you please self-moderate? I understand you might feel frustrated at the moment, but this sort of commentary is not healthy.

Copy link
Member

@lukekarrys lukekarrys left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a member of the npm team and will be working on adding support for devEngines to npm.

I think more time should be given to allow implementations of the devEngines spec to be written. Progress has been made in the package metadata interop collab space and work on implementation is ready to begin. My goal here is to avoid any possible user confusion and I think marking Corepack as stable will have the effect of locking in a single solution to this problem while we are working on another.

As a Node.js collaborator, I also have concerns about a lack of documented governance and goals in Corepack.

@aduh95
Copy link
Contributor Author

aduh95 commented Mar 6, 2024

@Ethan-Arrowood I'm going to need you to be more specific, I don't understand what would be the actionable steps to address your feedback.

@lukekarrys @wesleytodd Requesting more time is certainly fine, note that this PR is labelled semver major, so it won't make it to a release of Node.js until the next major anyway.

@Ethan-Arrowood
Copy link
Contributor

Ethan-Arrowood commented Mar 6, 2024

My apologies @arcanis that you didn't receive the notification about yesterday's meeting discussing the devEngines field and how it relates to npm/corepack. I posted about it in a couple of the ongoing threads (1, 2), as well as tweeted about it. It was also included in the meetings agenda a couple days ahead of time, and mentioned in the groups slack channel. I'll be more direct next time to ensure everyone knows they are invited.

The meeting primarily discussed the "devEngines" property. Including what steps would be needed to consider it "adopted", as well as how it could impact the ongoing corepack/npm debates. You can see the meeting notes here: openjs-foundation/package-metadata-interoperability-collab-space#17

@Ethan-Arrowood
Copy link
Contributor

@aduh95 I am echoing other's request for changes to ensure my opinion is similarly heard. Unfortunately, a simple +1 on someone else's request-for-changes does not have the same effect as an individualized review.

When the group can reach consensus on the inclusion of corepack I am happy to approve necessary changes.

@ronag
Copy link
Member

ronag commented Mar 26, 2024

This is quite a long thread now. Can some summarise the current technical objections to enable corepack by default? @aduh95 @GeoffreyBooth? Is it mainly the security parts atm?

@GeoffreyBooth
Copy link
Member

Can some summarise the current technical objections to enable corepack by default?

I was asked this in the 2024-03-06 TSC meeting and I gave a list then, which is in the meeting notes: https://github.com/nodejs/TSC/blob/main/meetings/2024-03-06.md#nodejsnode. There have been objections raised since then, many of which are summarized in the top post of #52107.

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe enabling corepack is what we all want to achieve. I am not having a full picture of what measures were taken to guarantee security concerns are fully addressed. I would love to have a short writeup about all measures including all concerns in concise language. For now, I can only abstain.

@darcyclarke
Copy link
Member

darcyclarke commented Mar 28, 2024

@BridgeAR / @ronag / @GeoffreyBooth, I wrote a pretty succinct list of issues I'm aware of but I'll reiterate/link them here for posterity (others should feel free to do the same to save new readers time):

  1. Adds yarn-specific codepaths which must be constantly updated/maintained/monitored
    (ex. Hash validation failed for yarn when COREPACK_NPM_REGISTRY is set on one side corepack#435)
  2. Inconsistent Distributions & Versions
  3. Differing Artifacts
  4. Broken or Missing Versions
  5. Integrity Mismatches for the Same Artifact
  6. Creates a New Source-Of-Truth (config.json) for Distributions instead of Relying on a Canonical Registry (registry.npmjs.org) & Aligning with Existing Tools (ex. npm, yarn, pnpm, bun etc.)
  7. Changing the Recommended Package Manager should be Discussed Separately from Enabling Corepack by Default

Edit: it looks like the anchor links above won't work if you haven't expanded the conversation & #51886 (comment) is visible - if you click on a link & it doesn't work, it's likely nested in that comment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. python PRs and issues that require attention from people who are familiar with Python. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

enable corepack by default