Closed Bug 1239139 Opened 4 years ago Closed 4 years ago

mach eslint should sanity check the user's environment

Categories

(Testing :: General, defect)

defect
Not set

Tracking

(firefox47 fixed)

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: mossop, Assigned: mossop)

References

Details

Attachments

(1 file)

People get confused when they have an old node or don't have the right npm packages installed. It should be easy for mach eslint to do a quick check, maybe only if eslint returns failure, to make sure everything is right and if not recommend running mach eslint --setup.
The most common issue I'm hearing with eslint is people who have an outdated
node installed. This does a quick check to verify the version is high enough
before linting.

Review commit: https://reviewboard.mozilla.org/r/34261/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/34261/
Attachment #8717677 - Flags: review?(gps)
Comment on attachment 8717677 [details]
MozReview Request: Bug 1239139: Verify that a high enough node version is available before running eslint. r?gps

https://reviewboard.mozilla.org/r/34261/#review30983

::: python/mach_commands.py:295
(Diff revision 1)
> +                if (self.is_valid(nodeOrNpmPath, minversion)):

Nit: you don't need parens for `if` in Python.

::: python/mach_commands.py:319
(Diff revision 1)
> -            with open(os.devnull, "w") as fnull:
> +            version_str = subprocess.check_output([path, "--version"])

I /think/ check_output will send stderr to sys.stderr by default. You probably don't want that. Use `stderr=subprocess.STDOUT` to redirect to stdout or use `stderr=os.devnull` to send stderr to a black hole.

::: python/mach_commands.py:320
(Diff revision 1)
> -                subprocess.check_call([path, "--version"], stdout=fnull)
> +            if (minversion):

No need for parens.

::: python/mach_commands.py:324
(Diff revision 1)
> +                version = StrictVersion(version_str)

Please use LooseVersion. We've historically had all kinds of problems with StrictVersion and inevitably switch to LooseVersion. For example, someone will be running a dev build and the version string won't conform to the strict standards, causing StrictVersion to barf.

You'll need to change everything to LooseVersion, because comparing LooseVersion to a StrictVersion raises a TypeError IIRC.
Attachment #8717677 - Flags: review?(gps)
Attachment #8717677 - Flags: review?(gps)
Comment on attachment 8717677 [details]
MozReview Request: Bug 1239139: Verify that a high enough node version is available before running eslint. r?gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34261/diff/1-2/
Assignee: nobody → dtownsend
Status: NEW → ASSIGNED
Comment on attachment 8717677 [details]
MozReview Request: Bug 1239139: Verify that a high enough node version is available before running eslint. r?gps

https://reviewboard.mozilla.org/r/34261/#review31119

::: python/mach_commands.py:279
(Diff revision 2)
> -    def getNodeOrNpmPath(self, filename):
> +    def getNodeOrNpmPath(self, filename, minversion = None):

Nit: don't put spaces around '=' in argument default values.

::: python/mach_commands.py:295
(Diff revision 2)
> +                if self.is_valid(nodeOrNpmPath, minversion):
> +                    return nodeOrNpmPath
>              except which.WhichError:
>                  pass
>  
>          if filename == "node":
>              print(NODE_NOT_FOUND_MESSAGE)

It seems to me that an old version will fall back to printing the NODE_NOT_FOUND_MESSAGE. There is follow-up work to print a NODE_TOO_OLD error to help people triage failures.

::: python/mach_commands.py:323
(Diff revision 2)
> +                if version_str[0] == "v":
> +                    version_str = version_str[1:]

`version_str = version_str.lstrip('v')` is more Pythonic. Plus it won't raise an IndexError if version_str is ''.

This will strip all leading 'v' chars, but that's probably OK.
Attachment #8717677 - Flags: review?(gps) → review+
Comment on attachment 8717677 [details]
MozReview Request: Bug 1239139: Verify that a high enough node version is available before running eslint. r?gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34261/diff/2-3/
https://hg.mozilla.org/mozilla-central/rev/433ccd116471
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.