1270851 - mach eslint should install eslint and dependencies if not installed

Status: RESOLVED FIXED

(Testing :: General, defect, P2)

Description

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

When mach eslint ran we should check for eslint and it's dependencies. If they are not installed we should install them.

Comment 1

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: eslint-auto-install.diff

Pretty simple change.

eslint and it's plugins will be installed to a local node modules folder as part of bug 1265082.

This patch requests to install eslint and it's plugins locally if they are not already installed when mach eslint is ran.

We purposefully avoid using the global version of eslint where possible because we really only want local installs. You can still use the global version if you pass the binary argument.

Comment 2

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: eslint-auto-install.diff

rebase

Comment 3

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: eslint-auto-install.diff

Rebase

Comment 4

[Gregory Szorc [:gps]]

Comment on attachment 8749801 [details] [diff] [review]
eslint-auto-install.diff

Review of attachment 8749801 [details] [diff] [review]:
-----------------------------------------------------------------

Please refactor the package installation code in the same file to share the list of packages. We don't want these getting out of sync.

Comment 5

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: eslint-auto-install.diff

Done.

Comment 6

[Gregory Szorc [:gps]]

Comment on attachment 8750271 [details] [diff] [review]
eslint-auto-install.diff

Review of attachment 8750271 [details] [diff] [review]:
-----------------------------------------------------------------

::: python/mach_commands.py
@@ +221,5 @@
> +        missing = []
> +
> +        for pkg in ESLINT_PACKAGES:
> +            if not self.node_package_installed(pkg):
> +                missing.append(pkg)

You could write this as: missing = [pkg for pkg in ESLINT_PACKAGES if not self.node_package_installed(pkg)]

@@ +223,5 @@
> +        for pkg in ESLINT_PACKAGES:
> +            if not self.node_package_installed(pkg):
> +                missing.append(pkg)
> +
> +        if len(missing) > 0:

if missing:

@@ +422,5 @@
>          fullpath = os.path.abspath(sys.modules['__main__'].__file__)
>          return os.path.dirname(fullpath)
> +
> +    def _prompt_yn(self, msg):
> +        print('%s? [Y/n]' % msg)

We need a check for an interactive terminal here or this could hang in automated processes. This should return (probably False) if not sys.stdin.isatty()

Comment 7

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: eslint-auto-install.diff

We decided that rather than having a root node_modules folder we should use module-specific node_modules.

This means that the eslint node_modules folder is now in:
testing/eslint/node_modules/

And the locally installed eslint binary is in:
/testing/eslint/node_modules/.bin/eslint

I think that most of us are happy with this setup.

(In reply to Gregory Szorc [:gps] from comment #6)
> Comment on attachment 8750271 [details] [diff] [review]
> eslint-auto-install.diff
> 
> Review of attachment 8750271 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: python/mach_commands.py
> @@ +221,5 @@
> > +        missing = []
> > +
> > +        for pkg in ESLINT_PACKAGES:
> > +            if not self.node_package_installed(pkg):
> > +                missing.append(pkg)
> 
> You could write this as: missing = [pkg for pkg in ESLINT_PACKAGES if not
> self.node_package_installed(pkg)]
> 

Changed

> @@ +223,5 @@
> > +        for pkg in ESLINT_PACKAGES:
> > +            if not self.node_package_installed(pkg):
> > +                missing.append(pkg)
> > +
> > +        if len(missing) > 0:
> 
> if missing:
> 

Changed

> @@ +422,5 @@
> >          fullpath = os.path.abspath(sys.modules['__main__'].__file__)
> >          return os.path.dirname(fullpath)
> > +
> > +    def _prompt_yn(self, msg):
> > +        print('%s? [Y/n]' % msg)
> 
> We need a check for an interactive terminal here or this could hang in
> automated processes. This should return (probably False) if not
> sys.stdin.isatty()

Awesome, added.

Comment 8

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: eslint-auto-install.diff

Replaced WindowsError with OSError

Comment 9

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: eslint-auto-install.diff

Sorry to ask for another review but I couldn't land this many changes without asking for one.

In order to rebase on top of Alex's changes in bug 1271706 I have built a little on the module validation logic (modules need to be installed locally etc.)

Here is a list of important changes:

1. We either use the user provided path for eslint or the local node module installed in testing/eslint/node_modules/... ideally we wouldn't allow a user provided path but there will always be people that want to use their own setup.
2. Much improved module validation... a module is reinstalled if any of the following are true:
  - It is not the required version for the build.
  - It is not installed locally.
3. We warn about any modules that are installed globally and explain how to uninstall them although we do not uninstall them ourselves.
4. We have added a simple package.json to the eslint folder to silence warnings about a missing package.json file.
5. We ignore any globally installed eslint-plugin-mozilla because unlinking in npm has a tendency to corrupt node_module folders. Besides, if we install the local version the global version is ignored.
6. ./mach eslint --setup now informs the user about the path to their **local** eslint binary.

To be honest the logic seems quite a bit simpler, which is probably a good thing.

Overall this gives us a much more solid experience.

I will update the documentation in bug 1271923.

Comment 10

[Gregory Szorc [:gps]]

Comment on attachment 8751795 [details] [diff] [review]
eslint-auto-install.diff

Review of attachment 8751795 [details] [diff] [review]:
-----------------------------------------------------------------

::: python/mach_commands.py
@@ +350,5 @@
> +                    global_install = subprocess.check_output([npmPath, "ls", "--json", name, "-g"],
> +                                                             stderr=fnull)
> +                info = json.loads(global_install)
> +                global_version = info["dependencies"][name]["version"]
> +            except:

Don't use bareword except in Python: it catches *all* exceptions, including KeyboardInterrupt and SystemExit (which don't inherit from Exception). This means it effectively swallows ctrl+c and requests to exit. That's bad. Especially when around a potentially long running block of code like executing a process.

Use "except Exception" to catch all "normal" exceptions.

Comment 11

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: eslint-auto-install.diff

(In reply to Gregory Szorc [:gps] from comment #10)
> Comment on attachment 8751795 [details] [diff] [review]
> eslint-auto-install.diff
> 
> Review of attachment 8751795 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: python/mach_commands.py
> @@ +350,5 @@
> > +                    global_install = subprocess.check_output([npmPath, "ls", "--json", name, "-g"],
> > +                                                             stderr=fnull)
> > +                info = json.loads(global_install)
> > +                global_version = info["dependencies"][name]["version"]
> > +            except:
> 
> Don't use bareword except in Python: it catches *all* exceptions, including
> KeyboardInterrupt and SystemExit (which don't inherit from Exception). This
> means it effectively swallows ctrl+c and requests to exit. That's bad.
> Especially when around a potentially long running block of code like
> executing a process.
> 
> Use "except Exception" to catch all "normal" exceptions.

Didn't know that. No longer using bareword except. Uses `except subprocess.CalledProcessError` instead as that is the expected exception here.

Comment 12

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: eslint-auto-install.diff

Rebased

Comment 13

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

https://hg.mozilla.org/integration/fx-team/rev/6e2add233b54766d44568f81aa207c624bbe4437
Bug 1270851 - mach eslint should install eslint and their dependencies if not installed r=gps

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6e2add233b54