Update ESLint to use node finding routines from configure

RESOLVED FIXED in Firefox 64

Status

enhancement
RESOLVED FIXED
9 months ago
6 months ago

People

(Reporter: dmose, Assigned: standard8)

Tracking

(Blocks 5 bugs)

Trunk
mozilla64
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)

Details

Attachments

(1 attachment)

Comment hidden (empty)
ESLint on Windows definitely doesn't work if node isn't in the MozillaBuild path.

$ mach lint -l eslint
nodejs is either not installed or is installed to a non-standard path.
Please install nodejs from https://nodejs.org and try again.

Valid installation paths:
  - C:\Program Files\nodejs
  - C:\nodejs
Node Package Manager (npm) is either not installed or installed to a
non-standard path. Please install npm from https://nodejs.org (it comes as an
option in the node installation) and try again.

Valid installation paths:
  - C:\Program Files\nodejs
  - C:\nodejs
IIRC `mach lint` has its own bespoke logic for finding Node. We will want it to look in self.substs (assuming the presence of a MozbuildObject class) to resolve the path to Node.

Or we should extract the Python code for finding Node to a Python module usable outside of configure and have both configure and random mach commands use that to find Node. This is the approach we use for locating a Python 3 executable.

Updated

7 months ago
Blocks: 1476980
(Assignee)

Updated

7 months ago
Blocks: 1491028
(Assignee)

Comment 3

7 months ago
The speedy tests no longer exist, so as far as we know node is only used by ESLint at the moment.

I'm currently thinking that extracting the code to a python module might be the easiest option and avoids ESLint depending on configure (for the time being at least).
Summary: audit and update any in-tree uses of Node (eg eslint, speedy tests, etc.) to use version found by configure → Update ESLint to use node finding routines from configure
(Reporter)

Comment 4

7 months ago
Sounds good to me!
(Assignee)

Comment 5

7 months ago
I have a partial WIP for this that works for ESLint, just got to hook up the configure changes, and then do some tidy-up.
Assignee: nobody → standard8
Status: NEW → ASSIGNED
(Assignee)

Comment 6

7 months ago
This extracts the current logic for finding nodejs into its own module in mozbuild. Configure and ESLint then use it.

For ESLint, this will change the first location it looks for nodejs to be the .mozbuild directory.
(Assignee)

Comment 7

7 months ago
The initial patch here isn't quite working on try, I need to handle the NODEJS env variable properly. I'll look at that tomorrow.
(Assignee)

Comment 8

7 months ago
I've tried getting ESLint to use the linux64-node toolchain in taskcluster, but it appears there's more than just a few configuration changes - somewhere along the line we need to get something to download the toolchain.

That seems more complicated than I want to cover in this bug, so for now, I've just gone with updating the ESLint builder to the matching version of node, and it'll find that from the PATH.

We can file a follow-up to make that nicer I think.
(Assignee)

Comment 9

7 months ago
I've now requested review, I think this is all working ok on the different platforms & for different cases.
(Assignee)

Updated

7 months ago
Blocks: 1493610
Comment on attachment 9010756 [details]
Bug 1482435 - Separate out nodejs finding logic from configure and use it for ESLint.

Gregory Szorc [:gps] has approved the revision.
Attachment #9010756 - Flags: review+

Comment 11

7 months ago
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/951f04d1bb51
Separate out nodejs finding logic from configure and use it for ESLint. r=firefox-build-system-reviewers,gps

Comment 12

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/951f04d1bb51
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
(Assignee)

Updated

7 months ago
Depends on: 1496080
You need to log in before you can comment on or make changes to this bug.