./mach eslint gives useless messages when you need to update nodejs

RESOLVED FIXED in Firefox 55

Status

Testing
Lint
RESOLVED FIXED
9 months ago
7 months ago

People

(Reporter: Gijs, Assigned: rajk, Mentored)

Tracking

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [lang=py])

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

9 months ago
Gijs Kruitbosch@DESKTOP-MB3M8BI /d/mozilla-central
$ which node
/c/program files/nodejs/node.exe

Gijs Kruitbosch@DESKTOP-MB3M8BI /d/mozilla-central
$ ./mach eslint toolkit/components/places/
nodejs v6.9.1 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
  - C:\Program Files (x86)\nodejs
A failure occured in the eslint linter.
? 1 problem (0 errors, 0 warnings, 1 failure)

Well, that's puzzling. Node is right where it's expected to be! Oh, wait:

Gijs Kruitbosch@DESKTOP-MB3M8BI /d/mozilla-central
$ node --version
v4.6.0


This error message would be much more useful if it actually showed where it did find node executables and what versions they were. Then I would not waste 10 minutes cursing the build system and assuming this was just another one of those times where we broke stuff on Windows and didn't notice.
I'm happy to mentor this. The file to look at is tools/lint/eslint.lint and it looks like the get_node_or_npm_path() function is the one that needs fixing.
Mentor: standard8@mozilla.com
Whiteboard: [lang=python]
Component: Build Config → Lint
Product: Firefox → Testing
Whiteboard: [lang=python] → [lang=py]

Comment 2

9 months ago
I would like to try to fix this problem.
Hi Sasa, thank you for the offer. I'm assigning you to the bug.

In case you need it, there's also some information here about how we run ESLint https://developer.mozilla.org/docs/ESLint
Assignee: nobody → sasacocic

Comment 4

9 months ago
I have the start of a solution and i'd like you to take a look at the changes i've made, but i'm having some trouble uploading a patch file.
(In reply to sasa cocic from comment #4)
> I have the start of a solution and i'd like you to take a look at the
> changes i've made, but i'm having some trouble uploading a patch file.

How are you trying to uplift the patch? There's some instructions here: https://developer.mozilla.org/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Submitting_the_patch

However, if you can't get that to work and need to attach a patch as a file, then feel free to do so - a unified diff with some context is normally best (and a commit message with author).
Blocks: 1354521
Unfortunately I've not heard any more from Sasa, so I'm opening this up again.
Assignee: sasacocic → nobody
Assignee: nobody → rajesh.kathiriya507
(Assignee)

Comment 7

7 months ago
what should be the msg to display in case of new version of node is required and system has old one installed? because in current msg it is clearly says specified required version to run eslint and user case anyway get current version using --version.
(Reporter)

Comment 8

7 months ago
(In reply to Rajesh Kathiriya(:rajk) from comment #7)
> what should be the msg to display in case of new version of node is required
> and system has old one installed? because in current msg it is clearly says
> specified required version to run eslint and user case anyway get current
> version using --version.

It should explicitly say if it detected other nodes and what versions they were, and not just suggest that the requisite version is "not installed or installed to a non-standard path" - it should suggest updating the node you have (if it found any).
To expand on Gijs comment, I'd expect something like one of two messages:

--------------------------------
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:
...
--------------------------------

or

--------------------------------
nodejs is out of date. You currently have node v6.0 but v6.9.1 is required.
Please update nodejs from https://nodejs.org and try again.
--------------------------------
Comment hidden (mozreview-request)

Comment 11

7 months ago
mozreview-review
Comment on attachment 8868667 [details]
bug 1346994 - Updated nodejs not found message

https://reviewboard.mozilla.org/r/140262/#review143976

Unfortunately we need to do a bit more than just update the existing message - we need to change `get_node_or_npm_path` so that it prints one of two messages:

-> Can't find node/npm if they aren't located
-> Out of date version of node/npm found
Attachment #8868667 - Flags: review?(standard8) → review-
Comment hidden (mozreview-request)

Comment 13

7 months ago
mozreview-review
Comment on attachment 8868667 [details]
bug 1346994 - Updated nodejs not found message

https://reviewboard.mozilla.org/r/140262/#review145918

Thank you for the update. This is heading in the right direction. Having looked into it a bit more, I think we do need to restructure this function a bit, as it is getting too complicated with the separate platform checking and the end results.

I've proposed a possible structure below, feel free to improve on it or ask if you're not sure.

::: tools/lint/eslint/setup_helper.py:262
(Diff revision 2)
>                  node_or_npm_path = which.which(filename + ext,
>                                                 path=get_possible_node_paths_win())
>                  if is_valid(node_or_npm_path, minversion):
>                      return node_or_npm_path
> +                else:
> +                    version_str = subprocess.check_output([node_or_npm_path, "--version"],

Unfortunately, this section is of the if statement starting on line 254 is windows-only, so we need to update the Linux section as well.

Rather than duplicating the code, I think we need to rewrite this function a bit. How about something like (this is not proper code):

```
def which_path(filename):
    path = None
    if platform.system() == Win:
        for ext in ...:
            try:
                path = which.which(filename + ext, ...
            except which.whichError:
                pass
    else:
        try:
            path = which.which(filename)
        except which.whichError:
            if (filename == "node":
                return which_path(nodejs)

def get_node_or_npm_path(filename, minversion=None):
    node_or_npm_path = which_path(filename)
    
    if not node_or_npm_path:
        # Display existing NODE_NOT_FOUND_MESSAGE or NPM_NOT_FOUND_MESSAGE
        # and the paths
        return None
       
    if not minversion:
        return node_or_npm_path
        
    version_str = get_version(node_or_npm_path)
    
    version = LooseVersion(version_str.lstrip('v'))

    if version > minversion:
        return node_or_npm_path

    # Display the new NODE_MACHING_VERSION_NOT_FOUND_MESSAGE message

    return None
```

The get_version function would be similiar to the is_valid function (and replaces it), but with just getting the version number from the executable.

::: tools/lint/eslint/setup_helper.py:264
(Diff revision 2)
>                  if is_valid(node_or_npm_path, minversion):
>                      return node_or_npm_path
> +                else:
> +                    version_str = subprocess.check_output([node_or_npm_path, "--version"],
> +                                              stderr=subprocess.STDOUT)
> +                    print(NODE_MACHING_VERSION_NOT_FOUND_MESSAGE % version_str)

It would be slightly better with `version_str.strip()` here, rather than just `version_str` as then the newline gets stripped off which makes the error message read nicer.
Attachment #8868667 - Flags: review?(standard8) → review-
Comment hidden (mozreview-request)

Comment 15

7 months ago
mozreview-review
Comment on attachment 8868667 [details]
bug 1346994 - Updated nodejs not found message

https://reviewboard.mozilla.org/r/140262/#review146340

This is great and is looking a lot better. There's just a few issues raised by the flake8 linter that need fixing.

::: tools/lint/eslint/setup_helper.py:247
(Diff revision 3)
>          "%s\\nodejs" % os.environ.get("SystemDrive"),
>          os.path.join(os.environ.get("ProgramFiles"), "nodejs"),
>          os.path.join(os.environ.get("PROGRAMW6432"), "nodejs"),
>          os.path.join(os.environ.get("PROGRAMFILES"), "nodejs")
>      })
> +    

If you run flake8:

./mach lint -l flake8

you'll notice there's various issues raised about extra whitespace & other things. Please fix those.
Attachment #8868667 - Flags: review?(standard8)
Comment hidden (mozreview-request)

Comment 17

7 months ago
mozreview-review
Comment on attachment 8868667 [details]
bug 1346994 - Updated nodejs not found message

https://reviewboard.mozilla.org/r/140262/#review146886

Thanks for the update. r=Standard8 with the final item below fixed.

Thank you for sticking with this, it'll be a great improvement to have (and cleaner code!).

::: tools/lint/eslint/setup_helper.py:291
(Diff revision 4)
> -    elif platform.system() == "Darwin":
> +        elif platform.system() == "Darwin":
> -        print("  - /usr/local/bin/{}".format(filename))
> +            print("  - /usr/local/bin/{}".format(filename))
> -    elif platform.system() == "Linux":
> +        elif platform.system() == "Linux":
> -        print("  - /usr/bin/{}".format(filename))
> +            print("  - /usr/bin/{}".format(filename))
>  
>      return None

This should be 4 spaces further indented, otherwise running eslint fails as it doesn't get to check the version part.
Attachment #8868667 - Flags: review?(standard8) → review+
Comment hidden (mozreview-request)
Thank you, I've just pushed it for you :-)

Comment 20

7 months ago
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a709c2aec00c
Updated nodejs not found message r=standard8

Comment 21

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a709c2aec00c
Status: NEW → RESOLVED
Last Resolved: 7 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1368656
You need to log in before you can comment on or make changes to this bug.