1809036 - "mach eslint" fails due incompatible htmlparser2 / entities libraries, not detected/fixed by ./mach eslint --setup

Status: RESOLVED FIXED

(Developer Infrastructure :: Lint and Formatting, defect, P3)

Description

[Rob Wu [:robwu]]

Despite having the latest checkout and the latest tools (via ./mach bootstrap and ./mach eslint --setup), ./mach eslint was failing for me. The log and additional information is pasted below, but apparently this was caused by the existence of an incompatible htmlparser2 library in tools/lint/eslint/eslint-plugin-mozilla/node_modules/. I had a very old version that was not getting updated despite running ./mach eslint --setup. After looking at the source, it's not too surprising because ./mach eslint --setup only checks the top-level node_modules directory:

• eslint_module_needs_setup() only checks the top-level node_modules directory: https://searchfox.org/mozilla-central/rev/72d9bf2d4128990cd1f349d4e15003e515277982/tools/lint/eslint/setup_helper.py#245-291
• expected_eslint_modules() has commented-out code for eslint-plugin-mozilla referencing bug 1766659: https://searchfox.org/mozilla-central/rev/72d9bf2d4128990cd1f349d4e15003e515277982/tools/lint/eslint/setup_helper.py#199-207

While this is an issue in ./mach, I'm marking this as a regression of bug 1792465, because the specific error & issue is caused by the top-level upgrade of the entities dependency (3.x -> 4.x) of that bug. If the dependency wasn't updated or if ./mach eslint --setup worked correctly, then I would not have encountered this issue.

$  ./mach eslint browser/components/distribution.js
An error occurred running eslint. Please check the following error messages:


Oops! Something went wrong! :(

ESLint: 8.29.0

Error [ERR_PACKAGE_PATH_NOT_EXPORTED]: Failed to load plugin 'mozilla' declared in '.eslintrc.js': Package subpath './lib/decode_codepoint.js' is not defined by "exports" in /path/to/gecko/node_modules/entities/package.json
Referenced from: /path/to/gecko/.eslintrc.js
    at new NodeError (node:internal/errors:387:5)
    at throwExportsNotFound (node:internal/modules/esm/resolve:365:9)
    at packageExportsResolve (node:internal/modules/esm/resolve:649:3)
    at resolveExports (node:internal/modules/cjs/loader:529:36)
    at Function.Module._findPath (node:internal/modules/cjs/loader:569:31)
    at Function.Module._resolveFilename (node:internal/modules/cjs/loader:981:27)
    at Function.Module._load (node:internal/modules/cjs/loader:841:27)
    at Module.require (node:internal/modules/cjs/loader:1067:19)
    at require (node:internal/modules/cjs/helpers:103:18)
    at Object.<anonymous> (/path/to/gecko/tools/lint/eslint/eslint-plugin-mozilla/node_modules/htmlparser2/lib/Tokenizer.js:3:23)

A failure occurred in the eslint linter.
✖ 1 problem (0 errors, 0 warnings, 1 failure, 0 fixed)

The eslint-plugin-mozilla/node_modules/ directory contains the following, with htmlparser2 having been created in 2019:

$ grep '"version":' tools/lint/eslint/eslint-plugin-mozilla/node_modules/*/package.json
tools/lint/eslint/eslint-plugin-mozilla/node_modules/eslint-scope/package.json:  "version": "7.1.1",
tools/lint/eslint/eslint-plugin-mozilla/node_modules/eslint-visitor-keys/package.json:  "version": "3.3.0",
tools/lint/eslint/eslint-plugin-mozilla/node_modules/estraverse/package.json:  "version": "5.3.0",
tools/lint/eslint/eslint-plugin-mozilla/node_modules/htmlparser2/package.json:  "version": "3.10.1"

The node_modules directory at the root does contain the latest version of htmlparser2:

$ grep -H '"version":' node_modules/htmlparser2/package.json
node_modules/htmlparser2/package.json:    "version": "8.0.1",

I got ./mach eslint to work after deleting htmlparser2@3.10.1 from tools/lint/eslint/eslint-plugin-mozilla/node_modules/ because Node.js will then find the top-level htmlparser2 library. What should ideally have happened here is for ./mach eslint --setup to have detected the incomplete or incompatible contents of tools/lint/eslint/eslint-plugin-mozilla/node_modules/ and updated accordingly.

Comment 1

[Mark Banner (:standard8)]

(In reply to Rob Wu [:robwu] from comment #0)

After looking at the source, it's not too surprising because ./mach eslint --setup only checks the top-level node_modules directory:

• eslint_module_needs_setup() only checks the top-level node_modules directory: https://searchfox.org/mozilla-central/rev/72d9bf2d4128990cd1f349d4e15003e515277982/tools/lint/eslint/setup_helper.py#245-291
• expected_eslint_modules() has commented-out code for eslint-plugin-mozilla referencing bug 1766659: https://searchfox.org/mozilla-central/rev/72d9bf2d4128990cd1f349d4e15003e515277982/tools/lint/eslint/setup_helper.py#199-207

I don't think this is the issue here. The reason I let that commenting out happen as we generally update the top-level package.json at the same time as tools/lint/eslint/eslint-plugin-mozilla/package.json. It would be very rare that we'd think about only updating the e-p-m dependencies, and indeed, bug 1792465 updated both.

Given the top-level upgrade included in bug 1792465, ./mach eslint should have automatically detected changes at the top level. Behind the scenes, this runs npm ci.

The documentation for that command states "If a node_modules is already present, it will be automatically removed before npm ci begins its install.".

However, I've just tested and this does not appear to apply to packages in sub-directories. The ./mach eslint --setup code only force clears out the top-level node_modules.

Hence we should probably clobber tools/lint/eslint/eslint-plugin-mozilla/node_modules there as well, to stop obsolete mismatches happening and compensate for npm ci not quite working the way we expect.

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1792465

Comment 3

[Rob Wu [:robwu]]

(In reply to Mark Banner (:standard8) from comment #1)

(In reply to Rob Wu [:robwu] from comment #0)

After looking at the source, it's not too surprising because ./mach eslint --setup only checks the top-level node_modules directory:

• eslint_module_needs_setup() only checks the top-level node_modules directory: https://searchfox.org/mozilla-central/rev/72d9bf2d4128990cd1f349d4e15003e515277982/tools/lint/eslint/setup_helper.py#245-291
• expected_eslint_modules() has commented-out code for eslint-plugin-mozilla referencing bug 1766659: https://searchfox.org/mozilla-central/rev/72d9bf2d4128990cd1f349d4e15003e515277982/tools/lint/eslint/setup_helper.py#199-207

I don't think this is the issue here. The reason I let that commenting out happen as we generally update the top-level package.json at the same time as tools/lint/eslint/eslint-plugin-mozilla/package.json. It would be very rare that we'd think about only updating the e-p-m dependencies, and indeed, bug 1792465 updated both.

The commented-out expected_eslint_modules() on its own is not an issue indeed: even if uncommented, it would not do anything useful because its caller (eslint_module_needs_setup() - the first bullet point) only checks the top-level node_modules/.

Given the top-level upgrade included in bug 1792465, ./mach eslint should have automatically detected changes at the top level. Behind the scenes, this runs npm ci.

The documentation for that command states "If a node_modules is already present, it will be automatically removed before npm ci begins its install.".

However, I've just tested and this does not appear to apply to packages in sub-directories. The ./mach eslint --setup code only force clears out the top-level node_modules.

Hence we should probably clobber tools/lint/eslint/eslint-plugin-mozilla/node_modules there as well, to stop obsolete mismatches happening and compensate for npm ci not quite working the way we expect.

I wouldn't expect npm ci to recursively scan the filesystem for node_modules/ subdirectories. If there is a desire to continue using the e-p-m node_modules AND to keep it updated, then two things are needed:

• The eslint setup logic (eslint_module_needs_setup() I suppose) would have to also scan the node_modules + package.json directories within e-p-m.
• npm ci has to be called again after chdir to e-p-m

If node_modules/ of the top level (via package.json / package-lock.json) are always going to be compatible with e-p-m (in-tree), then a simpler solution may be to unconditionally remove node_modules/ from e-p-m.

Comment 4

[Mark Banner (:standard8)]

(In reply to Rob Wu [:robwu] from comment #3)

I wouldn't expect npm ci to recursively scan the filesystem for node_modules/ subdirectories.

I think I've figured out what's happening. We use file: references in the package manifest, which (on at least Mac & Linux) uses symlinks in the top-level node_modules to point to those directories.

npm ci and npm install expect to be able to install sub-modules in the top-level node_modules under the various package directories. However, they obviously aren't looking at/caring about symlinks, as they think it all part of the top-level node_modules.

So this is probably a bug in assumptions within npm, since I can't see any warnings in the documentation about it.

If there is a desire to continue using the e-p-m node_modules...

We need to keep the current structure as it gives us various benefits (publishing, easy development).

...AND to keep it updated, then two things are needed:

• The eslint setup logic (eslint_module_needs_setup() I suppose) would have to also scan the node_modules + package.json directories within e-p-m.
• npm ci has to be called again after chdir to e-p-m

If node_modules/ of the top level (via package.json / package-lock.json) are always going to be compatible with e-p-m (in-tree), then a simpler solution may be to unconditionally remove node_modules/ from e-p-m.

Until we can resolve the current package differences issues (which won't happen until babel 8 releases, and that's an unknown eta at the moment), I think we need to take the simpler option of clobbering the e-p-m node_modules when we do setup updates. It won't be perfect but it should do.

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:andi, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 6

[Mark Banner (:standard8)]

Attachment: Bug 1809036 - When updating the top-level node_modules, always clobber the eslint-plugin-mozilla node_modules as well. r?#linter-reviewers!

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/711166be89ed
When updating the top-level node_modules, always clobber the eslint-plugin-mozilla node_modules as well. r=linter-reviewers,andi

Comment 8

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/711166be89ed