Closed
Bug 1476467
Opened 6 years ago
Closed 6 years ago
`mach eslint --setup` should pin content hashes for all dependencies
Categories
(Developer Infrastructure :: Lint and Formatting, defect, P1)
Developer Infrastructure
Lint and Formatting
Tracking
(firefox-esr60 unaffected, firefox61 unaffected, firefox62 unaffected, firefox63 fixed)
RESOLVED
FIXED
mozilla63
Tracking | Status | |
---|---|---|
firefox-esr60 | --- | unaffected |
firefox61 | --- | unaffected |
firefox62 | --- | unaffected |
firefox63 | --- | fixed |
People
(Reporter: gps, Assigned: standard8)
References
Details
(Keywords: regression)
Attachments
(1 file)
My reading of https://hg.mozilla.org/mozilla-central/file/afa310dc89be/tools/lint/eslint/setup_helper.py#l95indicates that we don't use version or content hash pinning for ESLint and its dependencies. That's a security weakness and source of non-determinism for developer installs. We should add content hash pinning so behavior is deterministic and downloads are more secure.
Assignee | ||
Comment 1•6 years ago
|
||
Unfortunately I changed this in bug 1475504, the documentation said: "The --no-package-lock argument will prevent npm from creating a package-lock.json file." As has been pointed out elsewhere that isn't true. I'll see if there's another option we can take.
Assignee | ||
Comment 2•6 years ago
|
||
Bug 1475504 was about trying to avoid package-lock.json file updates when we automatically install when running one of the lint commands. Unfortunately due to some npm versions / caches bugs it appears it can happen. It looks like `npm ci` might be a better way to go: https://docs.npmjs.com/cli/ci#description We would only use this for `./mach lint` commands that trigger install of the npm modules, i.e. eslint setup. A couple of highlights: - It will never write to package.json or any of the package-locks: installs are essentially frozen. - If a node_modules is already present, it will be automatically removed before npm ci begins its install. So the first item gets us what we want. The second item is possible a minor downside, but would at least ensure a clean and correct installation for most developers. The downside to this is that we'd need to increase the minimum npm version to 5.8.0 (from 5.6.0), that might be a bit more of a pain as node 8.x (our min supported node version) currently ships with 5.6.0. The only other option I can think of is to revert bug 1475504 and do a change that reverts any changes to package-lock.json (maybe warn about them?) after install.
Comment 3•6 years ago
|
||
So basic NodeJS support for all tier-1 platforms is not too far from landing. After that happens, the next thing will be set up a top-level node_modules and start figuring out how our policies for vendoring in dependencies. I wonder if the first thing vendored in wants to be eslint. If we take this route, we'd no longer need eslint-setup at all, and eslint would be covered by the tree-wide package-lock.json (or perhaps more likely, yarn.lock); 'mach bootstrap' would be used to install node in ~/.mozbuild, much like clang-for-stylo is in some places today. What do you think?
Assignee | ||
Comment 4•6 years ago
|
||
(In reply to Dan Mosedale (:dmose) from comment #3) > So basic NodeJS support for all tier-1 platforms is not too far from > landing. After that happens, the next thing will be set up a top-level > node_modules and start figuring out how our policies for vendoring in > dependencies. I wonder if the first thing vendored in wants to be eslint. > If we take this route, we'd no longer need eslint-setup at all, and eslint > would be covered by the tree-wide package-lock.json (or perhaps more likely, > yarn.lock); 'mach bootstrap' would be used to install node in ~/.mozbuild, > much like clang-for-stylo is in some places today. > > What do you think? IMO it is a bug & regression that we're not covered package-lock.json at the moment. We were before, but that patch made it wrong. I'm happy for node_modules and ESLint to be vendored in-tree and to get rid of eslint-setup, though it sounds like it may still be a week or two before we get it. I'd rather fix this before then, just in case something similar happens again.
Comment 5•6 years ago
|
||
Sounds entirely reasonable.
Assignee | ||
Comment 6•6 years ago
|
||
For npm >= 5.8.0, we use 'npm ci' which automatically only uses package-lock.json and doesn't update it. For npm < 5.8.0, we use the existing 'npm install' and take a copy of package-lock.json and replace it afterwards. MozReview-Commit-ID: EO3GdVYYNDP
Comment 7•6 years ago
|
||
Comment on attachment 8998117 [details] Bug 1476467 - Make eslint setup use package-lock.json again and use a different method for preventing accidental changes to package-lock.json. Andrew Halberstadt [:ahal] has approved the revision.
Attachment #8998117 -
Flags: review+
Comment 8•6 years ago
|
||
Comment on attachment 8998117 [details] Bug 1476467 - Make eslint setup use package-lock.json again and use a different method for preventing accidental changes to package-lock.json. Andrew Halberstadt [:ahal] has requested changes to the revision.
Attachment #8998117 -
Flags: review+
Comment 9•6 years ago
|
||
Comment on attachment 8998117 [details] Bug 1476467 - Make eslint setup use package-lock.json again and use a different method for preventing accidental changes to package-lock.json. Andrew Halberstadt [:ahal] has approved the revision.
Attachment #8998117 -
Flags: review+
Comment 10•6 years ago
|
||
Pushed by mbanner@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/24337f8b8499 Make eslint setup use package-lock.json again and use a different method for preventing accidental changes to package-lock.json. r=ahal
Comment 11•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/24337f8b8499
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Updated•6 years ago
|
status-firefox61:
--- → unaffected
status-firefox62:
--- → unaffected
status-firefox-esr60:
--- → unaffected
Updated•2 years ago
|
Product: Firefox Build System → Developer Infrastructure
You need to log in
before you can comment on or make changes to this bug.
Description
•