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: stop distributing Corepack #57617

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

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Mar 25, 2025

@aduh95 aduh95 added dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. dont-land-on-v22.x PRs that should not land on the v22.x-staging branch and should not be released in v22.x. dont-land-on-v23.x PRs that should not land on the v23.x-staging branch and should not be released in v23.x. dont-land-on-v24.x PRs that should not land on the v18.x-staging branch and should not be released in v24.x. labels Mar 25, 2025
@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. labels Mar 25, 2025
@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Mar 25, 2025
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Is there a reason to keep the ability to build node with corepack? that doesn't seem in line with the TSC decision.

configure.py Outdated
default=None,
help='do not install the bundled Corepack')
help='do install the bundled Corepack')
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
help='do install the bundled Corepack')
help='bundle Corepack')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
help='do install the bundled Corepack')
help='do install the bundled Corepack (experimental, will be removed without notice)')

Copy link
Member

Choose a reason for hiding this comment

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

"do install" at the front of the sentence doesn't read like proper english to me, and there is no "bundled corepack" anymore to install - this flag causes it to be bundled.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that "bundled Corepack" is weird, maybe "vendored" would be more appropriate?

Copy link
Member

Choose a reason for hiding this comment

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

the problem is the "the" - there is no "bundled corepack" anymore. the flag causes corepack to BE bundled, and "bundle" is a verb, which is why "bundle corepack" seems like the best/proper phrasing to me (with or without the experimental note)

@aduh95
Copy link
Contributor Author

aduh95 commented Mar 25, 2025

that doesn't seem in line with the TSC decision.

What makes you think that?

Is there a reason to keep the ability to build node with corepack?

Yes, so we can keep it up-to-date across the release lines

@ljharb
Copy link
Member

ljharb commented Mar 25, 2025

My read of the TSC decision is to not include corepack in any way whatsoever in 25+, not to make it optionally bundled and just change the default. iow, it's meant to be removed entirely.

Yes, so we can keep it up-to-date across the release lines

This commit will only be landing in v25+, though. Won't existing release lines lack this commit, and continue to build it with corepack by default?

@aduh95
Copy link
Contributor Author

aduh95 commented Mar 25, 2025

iow, it's meant to be removed entirely.

No, it was very explicit that the decision was about including (or not) a corepack executable in the distribution. Removing the vendored version from the repo would not be reasonable until 24.x is EOL. Anyway, does it matter?

Won't existing release lines lack this commit, and continue to build it with corepack by default?

That's correct, and in line with the TSC decision linked in the OP.

@ljharb
Copy link
Member

ljharb commented Mar 25, 2025

Right - node < 25 should continue to bundle corepack unchanged. node 25+ shouldn't even offer the option to build it with corepack, because the TSC decision was to "not include the corepack executable" - not to "not include the corepack executable by default".

If the option to build node with corepack remains in 25+, more people may be encouraged to compile node themselves rather than using the official binaries, which would magnify support burden for the project, and proliferate variants of node which could cause compatibility concerns down the line.

@targos
Copy link
Member

targos commented Mar 25, 2025

It's true that there's a risk to see unofficial distributions like the one from homebrew to keep including corepack. But this PR can be followed up by another one that removes it completely.

@ljharb
Copy link
Member

ljharb commented Mar 25, 2025

That's true, it just seems like a silly unnecessary step :-)

@aduh95
Copy link
Contributor Author

aduh95 commented Mar 25, 2025

Commits need to land on main first, so there's no way we remove the vendor version for the time being, not without giving us a lot more work. If folks go as far as to compile node themself rather than downloading Corepack in any other possible way, I don't think we can do anything for them, they might also patch this repo to re-add Corepack you know.

@ljharb
Copy link
Member

ljharb commented Mar 25, 2025

main won't ever touch < 25 though in its entirety - the vendored version can be removed from main and it shouldn't affect any of the other release lines - or is there some complex interaction that i'm missing?

@marco-ippolito
Copy link
Member

marco-ippolito commented Mar 25, 2025

I mean we could keep corepack in the deps folder but remove the ability to build it. This allows the automation to keep updating it and just cherry-pick commits into old release lines and at some point we remove it completely.
But it should not be possible to build node with corepack otherwise redistributors will enable it, so I think we should remove completely the ability to build.
Also isn't this semver major?

@ljharb ljharb added the semver-major PRs that contain breaking changes and should be released in the next major version. label Mar 25, 2025
@aduh95
Copy link
Contributor Author

aduh95 commented Mar 25, 2025

otherwise redistributors will enable it

Should we care?

@marco-ippolito
Copy link
Member

otherwise redistributors will enable it

Should we care?

I think we should

@ljharb
Copy link
Member

ljharb commented Mar 25, 2025

I also already explained above why we should care.

@aduh95
Copy link
Contributor Author

aduh95 commented Mar 26, 2025

I'm not convinced. If we want folks to stop using Corepack, we should spend time on building an alternative, not spend time on making their life harder. Also I'm not even convinced it would be more effective: I'm a maintainer of Nixpkgs Node.js packages, and I can tell you it is as easy to pass a configure flag than it is to revert a specific commit for redistributors, so I'm afraid your concern is not addressable.
TL;DR, I'm not going to do it, it's not worth my time, let's please move on.

@marco-ippolito
Copy link
Member

marco-ippolito commented Mar 26, 2025

I'll ping @nodejs/tsc for visibility as I think it's worth being aligned on this decision

Copy link
Member

@RaisinTen RaisinTen left a comment

Choose a reason for hiding this comment

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

Whether to remove the ability to build corepack is a separate decision. This change is already a step in the right direction and is aligned with the vote results, so LGTM.

@mhdawson
Copy link
Member

Whether to remove the ability to build corepack is a separate decision. This change is already a step in the right direction and is aligned with the vote results, so LGTM.

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@ljharb
Copy link
Member

ljharb commented Mar 26, 2025

It's fine to land this as an incremental step, but I don't think it's a separate decision - the ability to build node with corepack is part of what the TSC vote decided to remove.

@MikeMcC399
Copy link

  • probably PR deps: remove corepack #51981 should be closed as it would be superseded by this PR I believe. It's locked so I can't comment there

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.

lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. build Issues and PRs related to build files or the CI. dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. dont-land-on-v22.x PRs that should not land on the v22.x-staging branch and should not be released in v22.x. dont-land-on-v23.x PRs that should not land on the v23.x-staging branch and should not be released in v23.x. dont-land-on-v24.x PRs that should not land on the v18.x-staging branch and should not be released in v24.x. needs-ci PRs that need a full CI run. request-ci Add this label to start a Jenkins CI on a PR. 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.