Closed Bug 1288225 Opened 8 years ago Closed 8 years ago

Require all ESLint plugin commits to be running in automation

Categories

(Developer Infrastructure :: Lint and Formatting, defect)

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

Tracking

(firefox50 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla50
Tracking Flags:
Tracking Status
firefox50 --- fixed

People

(Reporter: jryans, Assigned: jryans)

Details

Attachments

(3 files)

Bug 1288225 - Tweak ESLint update text.
58 bytes, text/x-review-board-request
ahal
: review+
Details
Bug 1288225 - Use PEP 8 style for method names.
58 bytes, text/x-review-board-request
ahal
: review+
Details
Bug 1288225 - Exclude eslint-plugin-mozilla from tooltool.
58 bytes, text/x-review-board-request
ahal
: review+
Details 
At the moment, it is possible to commit changes to our ESLint modules, by which I mean:

* Version of ESLint used
* Version of external ESLint plugins used
* In-tree code for eslint-plugin-mozilla

without actually packaging it for automation.  If you do this, it means you aren't doing a try run to test the plugin changes, so you have no idea if it actually works outside of your local machine.  Additionally, the changes don't actually run for other developers' local machines either.

Currently, it's actually somewhat frustrating to package for automation, because you need to upload to tooltool, and only a small set of people have this access.

These problems have already come up in several bugs, such as bug 1280883.

We need to change this process such that any ESLint code changes are running in automation with the same commit that makes the change.

Some possible ideas:

A. Use a commit hook to ensure that ESLint plugin changes also modify the tooltool manifest which suggests it was uploaded
B. Move away from tooltool and use the in-tree files, making it easier for all developers to modify
I like option B. We already clone gecko at job runtime, so these files are available. We just need a script that packages everything properly. I guess it downloads a bunch of node_modules too?

What commands do you run to generate the current zip on tooltool?
(In reply to Andrew Halberstadt [:ahal] from comment #1)
> I like option B. We already clone gecko at job runtime, so these files are
> available. We just need a script that packages everything properly. I guess
> it downloads a bunch of node_modules too?
> 
> What commands do you run to generate the current zip on tooltool?

Something like option B definitely sounds nicer so that any developer can make rule changes.

At the moment, we run the script at https://dxr.mozilla.org/mozilla-central/source/tools/lint/eslint/update, which downloads all the Node modules and uploads them to tooltool.  This is actually a recently change in the last few months (bug 1265082) because the sheriffs wouldn't allow ESLint jobs to be tier 1 once they realized it downloads from NPM on every run.  So, things were changed to package into tooltool to avoid network requests when running the jobs, but this makes changes much harder.

One of the following approaches may work, but I am not sure how palatable they are to everyone:

B.1. We could vendor all the Node modules into m-c and get rid of tooltool.  Then there is still no network access, but perhaps a bit of bloat in the repo?  The tooltool manifest says the ESLint modules are currently 2.3 MB (size of .tar.gz), not sure if that's okay to accept as far as size growth of the m-c repo.

B.2. We could continue with tooltool for most of ESLint, but for the in-tree code of eslint-plugin-mozilla (which is what people change most frequently), we would leave that out of the tooltool package and just use the in-tree code directly.  With this change, we would only use tooltool when upgrading ESLint itself, which is less common.  There is a bit of complexity here for any Node modules that the in-tree plugin itself uses, which would need to end up in the tooltool archive still.  I don't think it's common to change those dependencies either, so perhaps it's okay.  (The most common change is adding or updating the code for a custom rule to the plugin code.)
I don't think 2.3MB is worth worrying about. We vendor in several python packages, some of which are over 2MB alone. That being said, option B.2 is likely easier to implement than option B.1 as it's probably just a small change to the update script and then re-running it. And it seems like B.2 would solve the majority of the problem anyway. If we go with B.1 we have to figure out where to store these vendored libraries and how to collect them (I assume we would want to make sure we don't start vendoring in the same library in multiple places)
Assignee: nobody → jryans
Status: NEW → ASSIGNED
This removes the in-tree plugin from the tooltool archive and uses that code
directly from the Gecko checkout instead.

For automation, we now get ESLint and external plugins from tooltool and then
symbolic link to the in-tree plugin.

For local development, we install ESLint and external plugins following the
shrinkwrap file created from the last change to the tooltool archive.  The local
plugin is then installed.

This change also removes the list of module versions from mach_commands.py, so
there is only one place to update module versions for the future.

Review commit: https://reviewboard.mozilla.org/r/66612/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66612/
Comment on attachment 8773980 [details]
Bug 1288225 - Exclude eslint-plugin-mozilla from tooltool.

Hmm, seems this version is not quite right when running on TaskCluster.  I'll try again next week.
Attachment #8773980 - Flags: review?(ahalberstadt)
Comment on attachment 8773980 [details]
Bug 1288225 - Exclude eslint-plugin-mozilla from tooltool.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66612/diff/1-2/
Attachment #8773980 - Flags: review?(ahalberstadt)
Comment on attachment 8773980 [details]
Bug 1288225 - Exclude eslint-plugin-mozilla from tooltool.

Haha, now it works on TC but not locally...
Attachment #8773980 - Flags: review?(ahalberstadt)
Comment on attachment 8773979 [details]
Bug 1288225 - Use PEP 8 style for method names.

https://reviewboard.mozilla.org/r/66610/#review64020

Thanks, I actually had a patch locally that does this too :). But land yours first as mine is part of a series that is still a ways away from being ready.

::: tools/lint/mach_commands.py:99
(Diff revision 1)
>          '''Run eslint.'''
>  
>          module_path = self.get_eslint_module_path()
>  
>          # eslint requires at least node 4.2.3
> -        nodePath = self.getNodeOrNpmPath("node", LooseVersion("4.2.3"))
> +        nodePath = self.get_node_or_npm_path("node", LooseVersion("4.2.3"))

Might as well do the same to the variable names while you're at it (though my patch will also do that, so feel free to close this issue instead)
Attachment #8773979 - Flags: review?(ahalberstadt) → review+
Attachment #8773978 - Flags: review?(ahalberstadt) → review+
Comment on attachment 8773980 [details]
Bug 1288225 - Exclude eslint-plugin-mozilla from tooltool.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66612/diff/2-3/
Attachment #8773980 - Flags: review?(ahalberstadt)
https://reviewboard.mozilla.org/r/66610/#review64020

> Might as well do the same to the variable names while you're at it (though my patch will also do that, so feel free to close this issue instead)

I believe I covered most of the variable name issues in the last patch (still to be reviewed) while I was reworking the code, so I'll drop this for now.
Comment on attachment 8773980 [details]
Bug 1288225 - Exclude eslint-plugin-mozilla from tooltool.

https://reviewboard.mozilla.org/r/66612/#review64456

This looks like a great cleanup, thanks! I think it's a good idea to wait for a more general solution to dealing with npm packages in-tree before vendoring everything in. Hopefully this will solve most of the problem.

::: tools/lint/eslint/npm-shrinkwrap.json:2
(Diff revision 3)
>  {
> +  "name": "mach-eslint",

I'm assuming this is tested and you know what you're doing in this file because I sure don't :)

::: tools/lint/mach_commands.py:219
(Diff revision 3)
>          return True
>  
> +    def expected_eslint_modules(self):
> +        # Read the expected version of ESLint and external modules
> +        expected_modules_path = os.path.join(self.get_eslint_module_path(), "package.json")
> +        expected_modules = json.load(open(expected_modules_path))["dependencies"]

Not super important in this case, but generally it's better to explicitly close files. I'm a fan of using context managers for this:

    with open(expected_modules_path, 'rb') as f:
        json.load(f)

::: tools/lint/mach_commands.py:224
(Diff revision 3)
> +        expected_modules = json.load(open(expected_modules_path))["dependencies"]
> +
> +        # Also read the in-tree ESLint plugin version
> +        mozilla_json_path = os.path.join(self.get_eslint_module_path(),
> +                                         "eslint-plugin-mozilla", "package.json")
> +        expected_modules["eslint-plugin-mozilla"] = json.load(open(mozilla_json_path))["version"]

Ditto.

::: tools/lint/mach_commands.py:248
(Diff revision 3)
> -                print("%s v%s should be v%s." % (name, data["version"], req_version))
> +                print("%s v%s should be v%s." % (name, data["version"], version_range))
>                  has_issues = True
>  
>          return has_issues
>  
> -    def node_package_installed(self, package_name="", globalInstall=False, cwd=None):
> +    def version_in_range(self, version, range):

nit: range is a keyword in python, maybe rename to crange to show it must be a caret range?

::: tools/lint/mach_commands.py:263
(Diff revision 3)
> +        # Caret ranges as specified by npm allow changes that do not modify the left-most non-zero
> +        # digit in the [major, minor, patch] tuple.  The code below assumes the major digit is
> +        # non-zero.
> +        range_match = CARET_VERSION_RANGE_RE.match(range)
> +        if range_match:
> +            range_major = int(range_match.group(1))
> +            range_minor = int(range_match.group(2))
> +            range_patch = int(range_match.group(3))
> +
> +            return (
> +                version_major == range_major and
> +                (
> +                    (version_minor == range_minor and version_patch >= range_patch) or
> +                    version_minor > range_minor
> +                )
> +            )

I think I understand this, but it might be more readable to first expand the caret range to an actual range, and then use `distutils.version.LooseVersion` to compare the versions. So assuming you convert `range` to `min_range` and `max_range`, you could do:

    return LooseVersion(min_range) <= LooseVersion(version) <= LooseVersion(max_range)

I'll call this optional, feel free to close.
Attachment #8773980 - Flags: review?(ahalberstadt) → review+
https://reviewboard.mozilla.org/r/66612/#review64456

> I'm assuming this is tested and you know what you're doing in this file because I sure don't :)

This file is autogenerated by the `npm shrinkwrap` command in the update script.  Checking in the changes here locks down all the package versions, so that `npm install` for other people should always grab the same tree of packages at the same versions.  But yes, I did also test it locally, including the upgrade case from what most people should have prior to this change. :)

> Not super important in this case, but generally it's better to explicitly close files. I'm a fan of using context managers for this:
> 
>     with open(expected_modules_path, 'rb') as f:
>         json.load(f)

Make sense, changed to use context manager style.

> Ditto.

Changed to use context manager style.

> nit: range is a keyword in python, maybe rename to crange to show it must be a caret range?

Okay, I changed to version_range.

> I think I understand this, but it might be more readable to first expand the caret range to an actual range, and then use `distutils.version.LooseVersion` to compare the versions. So assuming you convert `range` to `min_range` and `max_range`, you could do:
> 
>     return LooseVersion(min_range) <= LooseVersion(version) <= LooseVersion(max_range)
> 
> I'll call this optional, feel free to close.

Ah, this seems much nicer.  Didn't know Python supported chained comparisons like that, neat! :) Changed to use this style.
Pushed by jryans@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4956db508be7
Tweak ESLint update text. r=ahal
https://hg.mozilla.org/integration/mozilla-inbound/rev/d865632280ba
Use PEP 8 style for method names. r=ahal
https://hg.mozilla.org/integration/mozilla-inbound/rev/410cc90a90da
Exclude eslint-plugin-mozilla from tooltool. r=ahal
https://hg.mozilla.org/mozilla-central/rev/4956db508be7
https://hg.mozilla.org/mozilla-central/rev/d865632280ba
https://hg.mozilla.org/mozilla-central/rev/410cc90a90da
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
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: