Closed Bug 1228761 Opened 9 years ago Closed 8 years ago

"mach eslint --setup" tells me nodejs is not installed at expected path, but it is

Categories

(Developer Infrastructure :: Lint and Formatting, defect)

Product:
Developer Infrastructure
Component:
Lint and Formatting
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox45 affected, firefox53 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox45 --- affected
firefox53 --- fixed

People

(Reporter: dholbert, Assigned: standard8)

Details

Attachments

(2 files)

hackaround: look for nodejs instead of node
769 bytes, patch
Details | Diff | Splinter Review
Bug 1228761 - 'mach eslint' should look for 'nodejs' as well as 'node' executables to better support linux.
58 bytes, text/x-review-board-request
ahal
: review+
Details 
When I try to set up eslint, I get the following output:
> $ ./mach eslint --setup
> 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:
>   - /usr/bin/nodejs

But, I do actually have nodejs installed (I just now installed it), and it's installed precisely where mach says it's expecting it to be:
> $ which nodejs
> /usr/bin/nodejs

So, seems like something's broken here. Maybe mach is tripping over some other problem and misreporting what the problem is.
Summary: mach eslint --setup" → "mach eslint --setup" tells me nodejs is not installed at expected path, but it is
I'm using 64-bit Ubuntu 15.10 on a Lenovo laptop, btw, and I'm using an up-to-date mozilla-inbound build. (cset c33072613b5e)
Perhaps /usr/bin/nodejs is a symlink on your system and `./mach eslint` is not taking these into account?
Good idea, but sadly nope, it's not a symlink -- it's a normal file.
Thanks! If I change that line to look for "nodejs" instead of "node", then I get past that part.  So we should presumably be more flexible & look for either node or nodejs as the binary name here?
For reference, here's a local hackaround that works for me (not indended as an actual patch), to just hardcode in "nodejs" instead of "node".

(Presumably an actual patch would do something more graceful.)
Looks like the "nodejs-legacy" ubuntu package provides a "node" executable.  Maybe we just need to be more clearly suggesting to people that they need to install the "nodejs-legacy" package? (not sure)
Incidentally, even with the hackaround here, "./mach eslint" doesn't work for me.  The next thing I hit is bug 1229194, and then (after I worked around *that*) I hit bug 1229199.
Perhaps we could make the nodejs-legacy package part of the mach bootstrapping process?

Although looking for the nodejs binary as well means there’s no chance of a package manager install of Node.js interfering with a source installation, which you need to write code for Gaia.
Not sure if this is still an issue, but moving to the Lint component anyway.
Component: Build Config → Lint
Product: Core → Testing
Assignee: nobody → standard8
Comment on attachment 8809879 [details]
Bug 1228761 - 'mach eslint' should look for 'nodejs' as well as 'node' executables to better support linux.

Daniel, can you try this to see if it works for you?

I've only tested on Mac by adjusting a few things, but it seems to do the right thing with respect to falling back to nodejs if it can't find node.
Attachment #8809879 - Flags: feedback?(dholbert)
I've done "apt-get install nodejs" on my Ubuntu 16.04 desktop machine (and "nodejs --version" reports "v4.2.6"). With that, and without any patches, I get this output:
===
$ ./mach eslint --setup
nodejs v4.2.3 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:
  - /usr/bin/nodejs
✖ 0 problems (0 errors, 0 warnings)
===

If I apply standard8's patch from comment 13, the first part of the error changes to npm, but the "Valid installation paths" output remains unchanged:
===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:
  - /usr/bin/nodejs
✖ 0 problems (0 errors, 0 warnings)
===

If I then "apt-get install npm", then everything seems to work.
===
Installing eslint for mach using "/usr/bin/npm install"...
Installing eslint-plugin-mozilla using "/usr/bin/npm install /scratch/work/builds/mozilla-inbound/mozilla/tools/lint/eslint/eslint-plugin-mozilla"...

ESLint and approved plugins installed successfully!

NOTE: Your local eslint binary is at /scratch/work/builds/mozilla-inbound/mozilla/tools/lint/eslint/node_modules/.bin/eslint

✖ 0 problems (0 errors, 0 warnings)
===
("everything seems to work" only with standard8's patch, that is. Without that patch, we still fail to find my nodejs binary.)
So I think standard8's patch is good, though there are two outstanding issues here:
 (1) the "✖ 0 problems (0 errors, 0 warnings)" is very misleading. It sounds like "Everything was fine", which contradicts the error that was just displayed.

 (2) The "Valid installation paths" error message is wrong, when we fail to find npm.  It should be printing out some useful information about where it's expecting npm to be, rather than where it expects nodejs to be (since in my case, nodejs was in fact there).
Attachment #8809879 - Flags: feedback?(dholbert) → feedback+
I filed bug 1316925 about the "0 errors, 0 warnings" text. The other issue in comment 16 might be under the scope of this bug here; not sure.
(In reply to Daniel Holbert [:dholbert] from comment #16)
> So I think standard8's patch is good, though there are two outstanding
> issues here:
...
>  (2) The "Valid installation paths" error message is wrong, when we fail to
> find npm.  It should be printing out some useful information about where
> it's expecting npm to be, rather than where it expects nodejs to be (since
> in my case, nodejs was in fact there).

The updated patch I just pushed & requested review on should fix this as well.
Comment on attachment 8809879 [details]
Bug 1228761 - 'mach eslint' should look for 'nodejs' as well as 'node' executables to better support linux.

https://reviewboard.mozilla.org/r/92376/#review92756

Thanks for making this more clear! Mostly just have some minor cleanup suggestions, though r- because this will break the f8 lint task.

::: tools/lint/eslint.lint:216
(Diff revision 2)
> +            if filename == "node":
> +              # Retry it with "nodejs" as Linux tends to prefer nodejs rather than node.
> +              return get_node_or_npm_path("nodejs", minversion)

4 space indent required. Fyi, you can run `./mach lint tools/lint/eslint.lint` to catch python lint errors like this.

::: tools/lint/eslint.lint:219
(Diff revision 2)
> +            else:
> -            pass
> +              pass

Just drop this else clause

::: tools/lint/eslint.lint:222
(Diff revision 2)
> +              # Retry it with "nodejs" as Linux tends to prefer nodejs rather than node.
> +              return get_node_or_npm_path("nodejs", minversion)
> +            else:
> -            pass
> +              pass
>  
> -    if filename == "node":
> +    if filename == "node" or filename == "nodejs":

nit: if filename in ('node', 'nodejs'):

::: tools/lint/eslint.lint:234
(Diff revision 2)
> +        if filename == "node":
> -        print("  - /usr/local/bin/node")
> +            print("  - /usr/local/bin/node")
> +        else
> +            print("  - /usr/local/bin/npm")

Instead of  this if/else, you can replace the old version of this line with:

    print("  - /use/local/bin/{}".format(filename))

::: tools/lint/eslint.lint:239
(Diff revision 2)
> +        if filename == "node":
> -        print("  - /usr/bin/nodejs")
> +            print("  - /usr/bin/nodejs")
> +        else
> +            print("  - /usr/bin/npm")

Ditto, except with `"/usr/bin/{}".format(filename)`. You don't need to convert "node" to "nodejs" here either because filename will be set to "nodejs" by your recursive call above.
Attachment #8809879 - Flags: review?(ahalberstadt) → review-
Thanks for the comments, they're addressed now.
Comment on attachment 8809879 [details]
Bug 1228761 - 'mach eslint' should look for 'nodejs' as well as 'node' executables to better support linux.

https://reviewboard.mozilla.org/r/92376/#review92784

::: tools/lint/eslint.lint:231
(Diff revision 3)
> -        print("  - /usr/local/bin/node")
> +        print("  - /use/local/bin/{}".format(filename))
>      elif platform.system() == "Linux":
> -        print("  - /usr/bin/nodejs")
> +        print("  - /use/bin/{}".format(filename))

Heh, oops.. I made a typo in my last review comment which I guess you copy/pasted.. "/usr"
Attachment #8809879 - Flags: review?(ahalberstadt) → review+
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1eb522ebe098
'mach eslint' should look for 'nodejs' as well as 'node' executables to better support linux. r=ahal
https://hg.mozilla.org/mozilla-central/rev/1eb522ebe098
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Product: Testing → Firefox Build System
Product: Firefox Build System → Developer Infrastructure
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: