Skip to content

build: remove the rest of corepack - #57763

Closed
ljharb wants to merge 1 commit into
nodejs:mainfrom
ljharb:corepack
Closed

build: remove the rest of corepack#57763
ljharb wants to merge 1 commit into
nodejs:mainfrom
ljharb:corepack

Conversation

@ljharb

@ljharbljharb commented Apr 5, 2025

Copy link
Copy Markdown
Member

Refs:

This fulfills the spirit of the TSC vote by removing the ability to build node with corepack at all, including removing the vendored dep. It should only land in node 25+, and should not be backported (which should also mean that corepack distribution in release lines < v25 are not disrupted).

It leaves behind corepack.md.

@ljharbljharb added semver-major PRs that contain breaking changes and should be released in the next major version. build Issues and PRs related to build files or the CI. dont-land-on-v18.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-v24.x PRs that should not land on the v24.x-staging branch and should not be released in v24.x. labels Apr 5, 2025
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/actions
  • @nodejs/performance
  • @nodejs/security-wg
  • @nodejs/startup
  • @nodejs/tsc

@nodejs-github-botnodejs-github-bot added dependencies Pull requests that update a dependency file. doc Issues and PRs related to the documentations. meta Issues and PRs related to the general management of the project. needs-ci PRs that need a full CI run. labels Apr 5, 2025

@aduh95aduh95 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, adding my block to make sure this doesn't land before v24.x is in Maintenance

@ljharb

Copy link
Copy Markdown
MemberAuthor

@aduh95 that's fine, but can you help me understand why main would affect v24, which this commit wouldn't land on at all?

@aduh95

Copy link
Copy Markdown
Contributor

Commits make their way to LTS lines by being first released onto a Current release for at least two weeks, and Current releases get their commits from main; so landing this before 24.x is in maintenance would add unnecessary maintenance burden, as already discussed in #57617.

@ljharb

Copy link
Copy Markdown
MemberAuthor

Unfortunately the previous discussion, and this one, didn't make it clear to me why this is a burden. I'm aware that commits will land on main before being backported to v24, but the things this PR touches are highly unlikely to conflict with anything destined for v24 or older, so I'm unclear on what maintenance burden there would be.

@codecov

codecovBot commented Apr 5, 2025

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.95%. Comparing base (6428e2e) to head (ef9eeb1).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@ Coverage Diff @@## main #57763 +/- ##
=======================================
Coverage 89.95% 89.95% =======================================
Files 667 667 Lines 196813 196813 Branches 38425 38430 +5 =======================================
+ Hits 177038 177039 +1 + Misses 12200 12193 -7 - Partials 7575 7581 +6 

see 30 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@targos

Copy link
Copy Markdown
Member

There's no point in releasing in a Current line that doesn't have the feature. I don't believe it will be more difficult to maintain if the code only lives on <=v24.x. We'll just need to change automation to open the update PR against v24.x-staging instead of main.

@rluvatonrluvaton changed the title build: remove the reset of corepackbuild: remove the rest of corepackApr 6, 2025
@aduh95

aduh95 commented Apr 6, 2025

Copy link
Copy Markdown
Contributor

I don't believe it will be more difficult to maintain if the code only lives on <=v24.x. We'll just need to change automation to open the update PR against v24.x-staging instead of main.

Of course it would be, PR targeting main can be landed by any collaborator (or even triaggers using CQ), PRs targeting a staging branch fall under the responsibility of the releaser.

There's no point in releasing in a Current line that doesn't have the feature

For end users, sure. For us maintainers, the point is to keep using the existing process and minimize manual interventions.

@ljharb

Copy link
Copy Markdown
MemberAuthor

I think I understand the issue - because of the policy that anything destined for Current or LTS has to sit on main for 2 weeks, commits updating corepack have to land on main, which means this PR can't land on main.

Either blocking this PR until 24 is in maintenance would work, or, altering the policy so that some kinds of commits don't have to hit main before landing in Current/LTS.

@aduh95

Copy link
Copy Markdown
Contributor

To clarify, the policy is not blocking, we could still manage pushing updates to specific branches, the Release WG would likely won't see any problem with that. However, as explained in my previous comment, it would make Corepack maintenance harder / give more work to the releasers, so I'm against it.

@MikeMcC399

This comment was marked as resolved.

ljharb added a commit to ljharb/node that referenced this pull request Sep 10, 2025
@ljharb
ljharb deleted the corepack branch September 12, 2025 06:41
pullBot pushed a commit to zys-contrib/node that referenced this pull request Sep 12, 2025
Closesnodejs#57763.
Refs:
- nodejs/TSC#1697 (comment)
- nodejs#57617
- nodejs#57763
PR-URL: nodejs#59835
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Ulises Gascón <ulisesgascongonzalez@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

buildIssues and PRs related to build files or the CI.dependenciesPull requests that update a dependency file.docIssues and PRs related to the documentations.dont-land-on-v22.xPRs that should not land on the v22.x-staging branch and should not be released in v22.x.dont-land-on-v24.xPRs that should not land on the v24.x-staging branch and should not be released in v24.x.metaIssues and PRs related to the general management of the project.needs-ciPRs that need a full CI run.semver-majorPRs 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.

5 participants

@ljharb@nodejs-github-bot@aduh95@targos@MikeMcC399