Closed Bug 1270851 Opened 3 years ago Closed 3 years ago

mach eslint should install eslint and dependencies if not installed

Categories

(Testing :: General, defect, P2)

Version 3
defect

Tracking

(firefox49 fixed)

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: miker, Assigned: miker)

References

Details

Attachments

(1 file, 8 obsolete files)

When mach eslint ran we should check for eslint and it's dependencies. If they are not installed we should install them.
Attached patch eslint-auto-install.diff (obsolete) — Splinter Review
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.
Attachment #8749658 - Flags: review?(gps)
Attached patch eslint-auto-install.diff (obsolete) — Splinter Review
rebase
Attachment #8749658 - Attachment is obsolete: true
Attachment #8749658 - Flags: review?(gps)
Attachment #8749682 - Flags: review?(gps)
Attached patch eslint-auto-install.diff (obsolete) — Splinter Review
Rebase
Attachment #8749682 - Attachment is obsolete: true
Attachment #8749682 - Flags: review?(gps)
Attachment #8749801 - Flags: review?(gps)
Summary: mach eslint should install eslint and their dependencies if not installed → mach eslint should install eslint and dependencies if not installed
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.
Attachment #8749801 - Flags: review?(gps)
Attached patch eslint-auto-install.diff (obsolete) — Splinter Review
Done.
Attachment #8749801 - Attachment is obsolete: true
Attachment #8750271 - Flags: review?(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()
Attachment #8750271 - Flags: review?(gps)
Attached patch eslint-auto-install.diff (obsolete) — Splinter Review
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.
Attachment #8750271 - Attachment is obsolete: true
Attachment #8750827 - Flags: review?(gps)
Attached patch eslint-auto-install.diff (obsolete) — Splinter Review
Replaced WindowsError with OSError
Attachment #8750827 - Attachment is obsolete: true
Attachment #8750827 - Flags: review?(gps)
Attachment #8751254 - Flags: review?(gps)
Attachment #8751254 - Flags: review?(gps) → review+
Attached patch eslint-auto-install.diff (obsolete) — Splinter Review
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.
Attachment #8751254 - Attachment is obsolete: true
Attachment #8751795 - Flags: review?(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.
Attachment #8751795 - Flags: review?(gps)
Attached patch eslint-auto-install.diff (obsolete) — Splinter Review
(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.
Attachment #8751795 - Attachment is obsolete: true
Attachment #8752080 - Flags: review?(gps)
Rebased
Attachment #8752080 - Attachment is obsolete: true
Attachment #8752080 - Flags: review?(gps)
Attachment #8752188 - Flags: review?(gps)
Attachment #8752188 - Flags: review?(gps) → review+
https://hg.mozilla.org/integration/fx-team/rev/6e2add233b54766d44568f81aa207c624bbe4437
Bug 1270851 - mach eslint should install eslint and their dependencies if not installed r=gps
https://hg.mozilla.org/mozilla-central/rev/6e2add233b54
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.