build: don't store eslint locally#54231

avivkeller · 2024-08-06T18:09:08Z

This PR removes eslint as a local dependency and switches to downloading it from npm. It will still be updated regularly via @dependabot.

Why?

eslint is the only linter stored locally (besides cpplint, which has patches, unlike ESLint).
There are multiple issues related to file paths being too long and tarballs failing to lint properly (see: Missing ./tools/node_modules/eslint/eslint.js on make test? #39709, make -s test-doc fails with Error: Cannot find module '/node-v14.4.0/tools/doc/../node_modules/eslint/node_modules/js-yaml' #34005, Erbium: test regression in tarball #32765, and others).

Why was eslint set up like this initially?

When eslint was first added to the repository in #1539 around 10 years, it replaced a (not as large, but still large) library, closure-lint. Since then, the library has almost 4x its size. At the time, there was no discussion (AFAICT) about keeping or removing the directory—it was simply included as it was.

Size-wise, this'll save ~40MB, which is a decent amount.

(I'll add more fixes as I find more issues)
Fixes#39709
Fixes#54231

nodejs-github-bot · 2024-08-06T18:09:13Z

Review requested:

@nodejs/actions
@nodejs/security-wg
@nodejs/tsc

avivkeller · 2024-08-06T18:09:26Z

CC @nodejs/build @nodejs/linting

avivkeller · 2024-08-06T18:12:12Z

Reviewers, please review the commit '(review changes)'. The first commit removes the node_modules directory and may be hard to review in the github UI.

avivkeller · 2024-08-06T19:51:26Z

Everything is passing! The failure is because of #54229. The original PR didn't see any objections, is this good to land in the coming days?

codecov · 2024-08-06T19:54:56Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.33%. Comparing base (1bcdba2) to head (6b95bab).
Report is 289 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #54231 +/- ## ========================================== + Coverage 87.10% 87.33% +0.23%  ========================================== Files 647 648 +1 Lines 181693 182321 +628 Branches 34869 34972 +103 ========================================== + Hits 158256 159225 +969 + Misses 16733 16371 -362 - Partials 6704 6725 +21

see 90 files with indirect coverage changes

bnb · 2024-08-07T17:06:58Z

I'm +1 to this approach, but feel like there's probably folks who have more domain knowledge about this who should actually be the ones to approve it.

silverwind · 2024-08-07T17:36:23Z

I don't think there was any specific reason why those linters were checked into the repo besides maybe offline functionality, but I think that's likely no longer needed.

avivkeller · 2024-08-07T18:05:07Z

Great! I'm glad y'all are on board! Seeing as the linting is passing (except for the unrelated issue), all looks good in terms of land-ability.

Although, I assume this needs a CI to land.

nodejs-github-bot · 2024-08-07T18:50:30Z

CI: https://ci.nodejs.org/job/node-test-pull-request/60949/

avivkeller · 2024-08-07T21:29:00Z

CI is passing. 🎉
Linting is passing except for an unrelated failured that was patched in 7327e44.

nodejs-github-bot · 2024-08-07T22:19:42Z

CI: https://ci.nodejs.org/job/node-test-pull-request/60955/

Makefile

aduh95

Everytime I run e.g. make lint-js on this branch, it runs npm. Can you make it sure npm is run only when there were changes in the package-lock.json?

silverwind · 2024-08-09T10:54:05Z

Everytime I run e.g. make lint-js on this branch, it runs npm. Can you make it sure npm is run only when there were changes in the package-lock.json?

FWIW, it's possible to achieve on-demand node_modules installation with Make by tracking the folder timestamp, but it's not a 100% thing as Make timestamp tracking works best with files. Here is how I do it in my projects:

node_modules: package-lock.json npm install --no-save @touch node_modules

.github/dependabot.yml

avivkeller · 2024-08-18T00:36:29Z

Any more changes needed?

avivkeller · 2024-08-23T18:55:30Z

Thanks for approving! Does anyone else have any more feedback? It's been a while since any reviews.

avivkeller · 2024-09-01T17:44:13Z

No objections in a week, is this author ready?

aduh95 · 2024-09-01T18:13:19Z

node/doc/contributing/collaborator-guide.md

Lines 900 to 908 in 27f1306

*`author-ready` - A pull request is _author ready_ when: 
* There is a CI run in progress or completed. 
* There is at least one collaborator approval (or two TSC approvals for 
 semver-major pull requests). 
* There are no outstanding review comments. 
 Please always add the `author ready` label to pull requests that qualify. 
 Please always remove it again as soon as the conditions are not met anymore, 
 such as if the CI run fails or a new outstanding review comment is posted. 

so this is not author ready because there are no recent Jenkins CI run. If you start one, it would be.

nodejs-github-bot · 2024-09-01T18:22:05Z

CI: https://ci.nodejs.org/job/node-test-pull-request/61796/

avivkeller · 2024-09-01T20:08:05Z

Per @aduh95's comment:

There is a CI run in progress or completed. 🟢
There is at least one collaborator approval.
There are no outstanding review comments.

nodejs-github-bot · 2024-09-03T20:17:37Z

CI: https://ci.nodejs.org/job/node-test-pull-request/61882/

avivkeller · 2024-09-04T00:15:58Z

The MacOS CI appears to have failed to start, could someone restart it?

As for the linked linux OpenSSL one, see #54737

nodejs-github-bot · 2024-09-04T07:15:12Z

CI: https://ci.nodejs.org/job/node-test-pull-request/61895/

avivkeller · 2024-09-04T16:42:52Z

FWIW CI is 🟢 (Yay!)

nodejs-github-bot · 2024-09-06T17:31:47Z

Landed in 437e168

PR-URL: #54231 Reviewed-By: Antoine du Hamel <[email protected]>

PR-URL: nodejs#54231 Reviewed-By: Antoine du Hamel <[email protected]>

nodejs-github-bot added build Issues and PRs related to build files or the CI. meta Issues and PRs related to the general management of the project. needs-ci PRs that need a full CI run. tools Issues and PRs related to the tools directory. labels Aug 6, 2024

avivkeller requested review from anonrig and targos August 6, 2024 18:09

avivkeller changed the title ~~build: don't sure eslint locally~~build: don't store eslint locallyAug 6, 2024

build: don't store eslint locally
1216ae5

avivkeller force-pushed the eslint-not-local branch from 986840b to e8d9745Compare August 6, 2024 18:11

avivkeller added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Aug 6, 2024

avivkeller force-pushed the eslint-not-local branch from e8d9745 to 1e378c0Compare August 6, 2024 18:16

avivkeller mentioned this pull request Aug 6, 2024
build: don't store eslint locally #53974
Closed

benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 7, 2024

github-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 7, 2024

aduh95 reviewed Aug 8, 2024
View reviewed changes

MakefileShow resolvedHide resolved

aduh95 added dont-land-on-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. labels Aug 9, 2024

aduh95 previously requested changes Aug 9, 2024
View reviewed changes

targos reviewed Aug 11, 2024
View reviewed changes

.github/dependabot.ymlShow resolvedHide resolved

avivkeller force-pushed the eslint-not-local branch 2 times, most recently from b003520 to d9eb162Compare August 12, 2024 15:54

avivkeller requested a review from targos August 12, 2024 22:07

avivkeller force-pushed the eslint-not-local branch from d9eb162 to d3fa8b0Compare August 15, 2024 20:44

(review changes)
6b95bab

avivkeller force-pushed the eslint-not-local branch from d3fa8b0 to 6b95babCompare August 18, 2024 14:56

aduh95 approved these changes Aug 18, 2024
View reviewed changes

avivkeller added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 1, 2024

github-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 1, 2024

avivkeller added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 1, 2024

aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 6, 2024

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 6, 2024

nodejs-github-bot merged commit 437e168 into nodejs:mainSep 6, 2024

aduh95 pushed a commit that referenced this pull request Sep 12, 2024
build: don't store eslint locally
6200cf4
PR-URL: #54231 Reviewed-By: Antoine du Hamel <[email protected]>

RafaelGSS mentioned this pull request Sep 16, 2024
v22.9.0 proposal #54966
Merged

avivkeller mentioned this pull request Oct 4, 2024
build: stop pre-compiling lint-md#55266
Merged

louwers pushed a commit to louwers/node that referenced this pull request Nov 2, 2024
build: don't store eslint locally
969449f
PR-URL: nodejs#54231 Reviewed-By: Antoine du Hamel <[email protected]>

tpoisseau pushed a commit to tpoisseau/node that referenced this pull request Nov 21, 2024
build: don't store eslint locally
1636c30
PR-URL: nodejs#54231 Reviewed-By: Antoine du Hamel <[email protected]>

Uh oh!

build: don't store eslint locally#54231

build: don't store eslint locally #54231

Uh oh!

Conversation

avivkeller commented Aug 6, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Aug 6, 2024

Uh oh!

avivkeller commented Aug 6, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

avivkeller commented Aug 6, 2024

Uh oh!

avivkeller commented Aug 6, 2024

Uh oh!

codecovbot commented Aug 6, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

bnb commented Aug 7, 2024

Uh oh!

silverwind commented Aug 7, 2024

Uh oh!

avivkeller commented Aug 7, 2024

Uh oh!

nodejs-github-bot commented Aug 7, 2024

Uh oh!

avivkeller commented Aug 7, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Aug 7, 2024

Uh oh!

Uh oh!

aduh95 left a comment

Choose a reason for hiding this comment

Uh oh!

silverwind commented Aug 9, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

avivkeller commented Aug 18, 2024

Uh oh!

avivkeller commented Aug 23, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

avivkeller commented Sep 1, 2024

Uh oh!

aduh95 commented Sep 1, 2024

Uh oh!

nodejs-github-bot commented Sep 1, 2024

Uh oh!

avivkeller commented Sep 1, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Sep 3, 2024

Uh oh!

avivkeller commented Sep 4, 2024

Uh oh!

nodejs-github-bot commented Sep 4, 2024

Uh oh!

avivkeller commented Sep 4, 2024

Uh oh!

nodejs-github-bot commented Sep 6, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

avivkeller commented Aug 6, 2024•
edited
Loading

avivkeller commented Aug 6, 2024•
edited
Loading

codecovbot commented Aug 6, 2024•
edited
Loading

avivkeller commented Aug 7, 2024•
edited
Loading

silverwind commented Aug 9, 2024•
edited
Loading

avivkeller commented Aug 23, 2024•
edited
Loading

avivkeller commented Sep 1, 2024•
edited
Loading