"mach eslint" fails due incompatible htmlparser2 / entities libraries, not detected/fixed by ./mach eslint --setup
Categories
(Developer Infrastructure :: Lint and Formatting, defect, P3)
Tracking
(firefox-esr102 unaffected, firefox108 wontfix, firefox109 wontfix, firefox110 wontfix, firefox114 fixed)
Tracking | Status | |
---|---|---|
firefox-esr102 | --- | unaffected |
firefox108 | --- | wontfix |
firefox109 | --- | wontfix |
firefox110 | --- | wontfix |
firefox114 | --- | fixed |
People
(Reporter: robwu, Assigned: standard8)
References
(Regression)
Details
(Keywords: regression)
Attachments
(1 file)
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-levelnode_modules
directory: https://searchfox.org/mozilla-central/rev/72d9bf2d4128990cd1f349d4e15003e515277982/tools/lint/eslint/setup_helper.py#245-291expected_eslint_modules()
has commented-out code foreslint-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.
Reporter | ||
Updated•1 year ago
|
Assignee | ||
Comment 1•1 year ago
|
||
(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-levelnode_modules
directory:
eslint_module_needs_setup()
only checks the top-levelnode_modules
directory: https://searchfox.org/mozilla-central/rev/72d9bf2d4128990cd1f349d4e15003e515277982/tools/lint/eslint/setup_helper.py#245-291expected_eslint_modules()
has commented-out code foreslint-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•1 year ago
|
||
Set release status flags based on info from the regressing bug 1792465
Reporter | ||
Comment 3•1 year ago
|
||
(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-levelnode_modules
directory:
eslint_module_needs_setup()
only checks the top-levelnode_modules
directory: https://searchfox.org/mozilla-central/rev/72d9bf2d4128990cd1f349d4e15003e515277982/tools/lint/eslint/setup_helper.py#245-291expected_eslint_modules()
has commented-out code foreslint-plugin-mozilla
referencing bug 1766659: https://searchfox.org/mozilla-central/rev/72d9bf2d4128990cd1f349d4e15003e515277982/tools/lint/eslint/setup_helper.py#199-207I 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 astools/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 runsnpm 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 fornpm 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 thenode_modules
+ package.json directories within e-p-m. npm ci
has to be called again afterchdir
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.
Assignee | ||
Comment 4•1 year ago
|
||
(In reply to Rob Wu [:robwu] from comment #3)
I wouldn't expect
npm ci
to recursively scan the filesystem fornode_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 thenode_modules
+ package.json directories within e-p-m.npm ci
has to be called again afterchdir
to e-p-mIf 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.
Updated•1 year ago
|
Updated•1 year ago
|
Comment 5•1 year ago
|
||
The severity field is not set for this bug.
:andi, could you have a look please?
For more information, please visit auto_nag documentation.
Updated•1 year ago
|
Assignee | ||
Comment 6•1 year ago
|
||
Updated•1 year ago
|
Pushed by mbanner@mozilla.com: 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•1 year ago
|
||
bugherder |
Updated•1 year ago
|
Updated•6 months ago
|
Description
•