Closed Bug 1265082 Opened 8 years ago Closed 8 years ago

ESLint jobs are apparently hitting the network?

Categories

(Developer Infrastructure :: Lint and Formatting, defect, P1)

48 Branch
defect

Tracking

(firefox48 affected, firefox49 fixed)

RESOLVED FIXED
mozilla49
Tracking Status
firefox48 --- affected
firefox49 --- fixed

People

(Reporter: KWierso, Assigned: miker)

References

Details

Attachments

(1 file, 2 obsolete files)

Ryan just pushed some patches to fx-team and the eslint job that ran on his push failed with https://public-artifacts.taskcluster.net/L6wu3c8KT-avDvoOcJZQmg/0/public/logs/live_backing.log

No errors got parsed out of the log, but it looks like there were some errors installing things, and it looks like those things were installing from npmjs.org.

Why are these jobs hitting the outside network?
Flags: needinfo?(dtownsend)
It's linking the in-tree eslint plugin to the npm modules which in turn requires some modules to be installed.
Flags: needinfo?(dtownsend)
Can that be done in-house? Hitting external infra is a pretty big no-no for Tier 1 jobs (and this bug was filed after npmjs.org issues burned a job, so it's not theoretical).
I imagine it is possible but I don't have any time to work on this.
ESLint demoted to tier-2 for touching the network.
I requested the creation of a new ESLint component on bugzilla, so we have a place to move bugs like this to.
Component: General → ESLint
I also won't have time to work on this now, but demoting the eslint job to tier 2 is sad because it opens the door to JS inconsistencies creeping back in the tree without people realizing. And before long, the eslint job will be broken beyond saving ...

If I'm understanding this well, the task is defined here:
https://dxr.mozilla.org/mozilla-central/source/testing/taskcluster/tasks/tests/eslint-gecko.yml

As :mossop said, at some stage, it links our custom mozilla eslint rules module:
npm link testing/eslint-plugin-mozilla

this module is in the tree rather than on the npm server to avoid sync issues and having to download it, and npm link is the equivalent of npm install but for locally hosted modules.

So when that happens, npm reads the eslint-plugin-mozilla's package.json file:
https://dxr.mozilla.org/mozilla-central/source/testing/eslint-plugin-mozilla/package.json

this file defines the following dependencies (which, of course, in turn probably depend on other packages):
  "dependencies": {
    "escope": "^3.2.0",
    "espree": "^2.2.4",
    "estraverse": "^4.1.1",
    "sax": "^1.1.4"
  },

In order to avoid having to download these from the network, we would need to store them in the tree and let npm find them there (in a node_modules directory).
It looks like the nom shrinkwrap command might be useful:
https://nodejs.org/en/blog/npm/managing-node-js-dependencies-with-shrinkwrap/

This would also have the added advantage of fixing the versions of all the dependencies used here. But of course would make updates harder.

Mike: any chance you'd have some time to look into this?
Flags: needinfo?(mratcliffe)
Yes, I will take a look... we can install a .tar.gz using npm install and include the dependencies that way. The problem is that we would need to formalize that whereas updating a node_modules folder would also work with less red tape.

Lets go for installing eslint locally (in an in tree node_modules folder) along with all it's dependencies. That would mean that people just need to configure their editors to use it and wouldn't need to worry about installing it.
Assignee: nobody → mratcliffe
Flags: needinfo?(mratcliffe)
We should note that these modules have a payload of 9.6MB.

We also need to ensure that all of the ./mach commands still work.
tooltool is also an option for jobs that run in automation
On the plus side having them in tree also means that we can make use of modules in the node_modules folder in other areas of our codebase.
(In reply to Ryan VanderMeulen [:RyanVM] from comment #9)
> tooltool is also an option for jobs that run in automation

We have been playing with the idea of using local node modules so we may as well ship it in tree... it will cause less problems if eslint is the exact version we used with the currently checked out Firefox version.
So a few changes here:
- node_modules is now in tree so we are no longer hitting the network.
- We have a npm-shrinkwrap.json file that version locks all of our node packages.
- eslint, eslint-plugin-mozilla etc. are now all installed locally.

In reality this means that we don't hit the network and we don't force users into installing global packages.

./mach eslint --setup has also been improved. We install packages locally and display the path of the user's eslint binary (useful for configuring editors).

Review commit: https://reviewboard.mozilla.org/r/49933/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49933/
Attachment #8747563 - Flags: review?(pbrosset)
Comment on attachment 8747563 [details]
MozReview Request: Bug 1265082 - ESLint jobs are apparently hitting the network r?dustin

@pbrosset: Not as big as it seems... added node_modules into the tree, shrinkwrapped them and made eslint and our plugins install locally.

The real question is do we really want node_modules in the tree or shall we use an alternate method to create it in the testing environment?

I would have pinged Mossop for review but he is off the grid as far as reviews etc. at the moment.
(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #13)
> The real question is do we really want node_modules in the tree or shall we
> use an alternate method to create it in the testing environment?

I should point out that we need our versions of firefox and eslint to stay in sync to prevent errors so I believe shipping with the node_modules in tree is the only way to do this.
Comment on attachment 8747563 [details]
MozReview Request: Bug 1265082 - ESLint jobs are apparently hitting the network r?dustin

https://reviewboard.mozilla.org/r/49933/#review46703

I think this is the right approach, but I don't feel comfortable reviewing this change and would prefer someone else do it.
Adding ~3000 new files in node_modules isn't anecdotal, and there are some mach python changes that I have never reviewed before.
I think someone working on treeherder could weigh in maybe? Sorry I really don't know who to ask.
Attachment #8747563 - Flags: review?(pbrosset)
Comment on attachment 8747563 [details]
MozReview Request: Bug 1265082 - ESLint jobs are apparently hitting the network r?dustin

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49933/diff/1-2/
Attachment #8747563 - Attachment description: MozReview Request: Bug 1265082 - ESLint jobs are apparently hitting the network r?mossop → MozReview Request: Bug 1265082 - ESLint jobs are apparently hitting the network r?dustin
Attachment #8747563 - Flags: review?(dustin)
Priority: -- → P1
@dustin:
- python/mach_commands.py: Install all node modules locally (we were going to make this change anyway).
- We use shrinkwrap to lock package versions.
- The big question is... our node packages need to be the versions used for version x of Firefox. The obvious solution is to include the node_modules folder in the tree so that they are always in sync. We could use ToolTool to install the node modules but then the versions would not be synchronized.

@dcamp:
Dave, what do you think? We need the node packages to be in sync but having the modules in the tree is a political issue and I guess that is your stomping ground. You know the infrastructure well so can you think of another workaround? Maybe as part of creating a new release we could zip the node modules and then use ToolTool to install them. Not sure how we could match the versions then though.
Flags: needinfo?(dcamp)
Yeah, I'm definitely not someone who can green-light a top-level node_modules directory.

If you put that in tooltool, then the versions can correspond, it just means you'd need to upload a new blob to tooltool (with a new hash) when you wanted to upgrade them.

Another option is to build the node_modules tarball on-demand in a prerequisite task.  We would basically look up that task by a hash of npm-shrinkwrap.json, and if not found start a new task to create it first.  That task would still hit the network, but only when npm-shrinkwrap.json changes.  We're already OK with this compromise for a number of other cases like docker images.  Using npm shrinkwrap helps, too, and it'd be even better if that had verifiable package hashes in it the way `peep` does for Python.  Sadly, this option isn't quite available yet -- I'm still working on the support for such dependent tasks in the taskgraph.  So probably tooltool is the best option at the moment.
Attachment #8747563 - Flags: review?(dustin)
tooltool it is then.
Flags: needinfo?(dcamp)
Comment on attachment 8747563 [details]
MozReview Request: Bug 1265082 - ESLint jobs are apparently hitting the network r?dustin

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49933/diff/2-3/
Attachment #8747563 - Flags: review?(dustin)
@dustin: I am not sure if you are the right person to review this so feel free to give it somebody else if you don't have time.
Incidentally, I don't know how to test the linting task, it should work but I don't know how to force it to run... it is hard to find a combination on try that runs it.

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=66d402983b22
(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #23)
> Incidentally, I don't know how to test the linting task, it should work but
> I don't know how to force it to run... it is hard to find a combination on
> try that runs it.
This works for me:
try: -b do -p linux64,macosx64,win64,eslint-gecko -u xpcshell,mochitest-dt,mochitest-o,mochitest-e10s-dt -t none
(of course there's a lot of other tasks in there that you don't need, the important part is the eslint-gecko platform).
It is worth mentioning that testing/eslint-plugin-mozilla/update is a simple update script that updates eslint and it's minions and ups the archive of node_modules with tooltool if you have the tokens. If this script is run and a patch containing changes the script made is landed it should just work.
Comment on attachment 8747563 [details]
MozReview Request: Bug 1265082 - ESLint jobs are apparently hitting the network r?dustin

https://reviewboard.mozilla.org/r/49933/#review47759

::: python/mach_commands.py:276
(Diff revision 3)
> +
>          npmPath = self.getNodeOrNpmPath("npm")
>          if not npmPath:
>              return 1
>  
> +        # eslint-plugin-mozilla **never** be installed (linked) globally.

should

::: testing/docker/lint/Dockerfile
(Diff revision 3)
>  
>  # install necessary npm packages
>  RUN           npm install -g taskcluster-vcs@2.3.12
> -RUN           npm install -g eslint@2.8.0
> -RUN           npm install -g eslint-plugin-html@1.4.0
> -RUN           npm install -g eslint-plugin-react@4.2.3

Huh, I didn't realize these tasks were running in a purpose-built docker image, rather than in desktop-test.  It might be even *easier* to just do the `npm install` in the docker image and have it ready and waiting.  There's already support for automatically re-generating the docker image when the Dockerfile (or anything in its directory) changes.

::: testing/eslint-plugin-mozilla/manifest.tt:4
(Diff revision 3)
> +[
> +{
> +"size": 2903832,
> +"visibility": "internal",

I think you want "public" here, unless there's something in this tarball that we can't redistribute.  The downside of "internal" is that it requires tooltool credentials to download.  Those are present in CI, but not on developer's systems.

I know you've been hacking on this for a while, so I don't mind landing this particular form, if you'd rather not re-re-factor to do the `npm install` in the Dockerfile.  I should have seen that possibility earlier.

The visibility is worth fixing, as 'internal' will annoy people down the road.  Tooltool doesn't have an API for changing visbiilities, so you can either upload a new file with a different sha512 hash, or ask someone in releng to edit the row in the tooltool DB directly.

I don't think I'm qualified to r+ the top-level npm-shrinkwrap.json, but the rest of this looks good.
Attachment #8747563 - Flags: review?(dustin)
Comment on attachment 8747563 [details]
MozReview Request: Bug 1265082 - ESLint jobs are apparently hitting the network r?dustin

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49933/diff/3-4/
Attachment #8747563 - Flags: review?(dustin)
Comment on attachment 8747563 [details]
MozReview Request: Bug 1265082 - ESLint jobs are apparently hitting the network r?dustin

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49933/diff/4-5/
Attachment #8747563 - Flags: review?(dustin) → review?(pbrosset)
@pbrosset: You don't need to review everything, dustin doesn't want to interfere with the idea of using npm shrinkwrap which I think makes sense for us.

So you really only need to review the "idea" of using npm shrinkwrap and maybe my update script... it was your idea so that should be pretty easy.

I won't land anything until the task is running cleanly.
Looks like it works fine... I guess I need to fix the eslint failures that have accumulated now though.

I will upload them as a separate patch.
..and in particular a toplevel shrinkwrap file -- will this be the only npm install?
Please note that bug 1270596 has just landed on fx-team, which upgrades to ESLint 2.9.0.  Please carry over this upgrade to the work here.
Attachment #8747563 - Attachment is obsolete: true
Attachment #8747563 - Flags: review?(pbrosset)
So a few changes here:
- node_modules is downloaded using tooltool so that we dont need to rely on external infrastructure.
- We have a npm-shrinkwrap.json file that version locks all of our node packages.
- eslint, eslint-plugin-mozilla etc. are now all installed locally.

In reality this means that we don't hit the network and we don't force users into installing global packages.

./mach eslint --setup has also been improved. We install packages locally and display the path of the user's eslint binary (useful for configuring editors).

Review commit: https://reviewboard.mozilla.org/r/51101/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51101/
Attachment #8749698 - Flags: review?(pbrosset)
Setting ni? so you don't forget ESLint 2.9.0 as mentioned in comment 34.
Flags: needinfo?(mratcliffe)
Comment on attachment 8749698 [details]
MozReview Request: Bug 1265082 - ESLint jobs are apparently hitting the network r=me,dustin,pbro,jryans

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51101/diff/1-2/
Attachment #8749698 - Attachment description: MozReview Request: Bug 1265082 - ESLint jobs are apparently hitting the network r?dustin → MozReview Request: Bug 1265082 - ESLint jobs are apparently hitting the network r?pbro
Attachment #8749694 - Attachment is obsolete: true
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #38)
> Setting ni? so you don't forget ESLint 2.9.0 as mentioned in comment 34.

$ npm ls | grep eslint
    eslint@2.9.0
    eslint-plugin-html@1.4.0
    eslint-plugin-mozilla@0.0.3
    eslint-plugin-react@4.2.3
Flags: needinfo?(mratcliffe)
Comment on attachment 8749698 [details]
MozReview Request: Bug 1265082 - ESLint jobs are apparently hitting the network r=me,dustin,pbro,jryans

pbrosset is busy and all that still needs reviewing here is the testing/eslint-plugin-mozilla/update script, which is as simple as it gets.
Attachment #8749698 - Flags: review?(pbrosset) → review?(jryans)
Comment on attachment 8749698 [details]
MozReview Request: Bug 1265082 - ESLint jobs are apparently hitting the network r=me,dustin,pbro,jryans

https://reviewboard.mozilla.org/r/51101/#review48103

This approach could work, but do we have permission to create a top level node_modules directory?  It looks like this was discussed, but I don't see approval for it in the bug.  I am also not sure who you ask anymore to be honest (it used to be Brendan).

Also, I am somewhat worried about this process with a top level node_modules directory.  If someone else wants to start using the top level node_modules directory also for other modules, then it would compete with the modules you are installing here and then we'd have several different people who need to update the global node_modules bundle in tooltool... it sounds kind of messy.  We could decide to accept this and deal with it, but I think we may need a broader discussion first if we want to go that route.

Since the goal of this bug is _not_ "Create a global node_modules folder", what if you use basically this approach, but you keep the node_modules limited to inside testing somewhere?  Then we can leave discussion of a global node_modules as a follow up thing, since we probably have to hash it out on dev-platform or somewhere first.

Another alternative is to keep using the Docker image instead of tooltool like Dustin mentions in comment 26, though you may still want a limited location for the modules that is not the top level directory, since you want it to match for local installs as well.

::: python/mach_commands.py:279
(Diff revision 2)
>              return 1
>  
> +        # eslint-plugin-mozilla should **never** be installed (linked) globally.
> +        # Let's unlink it just in case.
> +        self.callProcess("remove-eslint-plugin-mozilla",
> +                                   [npmPath, "unlink", "eslint-plugin-mozilla"])

Nit: this line seems indented incorrectly

::: python/mach_commands.py:308
(Diff revision 2)
> -                                   [npmPath, "install", "eslint-plugin-react@4.2.3", "-g"])
> +                                   [npmPath, "install", "eslint-plugin-react@4.2.3"], root)
>          if not success:
>              return 1
>  
>          print("\nESLint and approved plugins installed successfully!")
> +        print("\nNOTE: YOUR LOCAL ESLINT BINARY IS IN %s/node_modules/.bin/eslint\n" % root)

Nit: I don't think it needs to be ALL CAPS.

::: testing/eslint-plugin-mozilla/update:56
(Diff revision 2)
> +echo "Adding eslint.tar.gz to tooltool..."
> +rm testing/eslint-plugin-mozilla/manifest.tt
> +./tooltool.py add --visibility public eslint.tar.gz
> +
> +echo "Uploading eslint.tar.gz to tooltool..."
> +./tooltool.py upload --authentication-file=~/.tooltool-token --message "node_modules folder update"

These steps seem logical, but I don't have tooltool upload access so it's hard for me to be sure.  Should Dustin or someone check it as well?
Attachment #8749698 - Flags: review?(jryans)
After discussing this on IRC we have decided to move the node_modules install to testing/eslint/node_modules.

Using this method multiple modules can have their own node_modules folder and dependencies.

I guess that the plugin should also be moved into this folder.
Comment on attachment 8749698 [details]
MozReview Request: Bug 1265082 - ESLint jobs are apparently hitting the network r=me,dustin,pbro,jryans

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51101/diff/2-3/
Attachment #8749698 - Attachment description: MozReview Request: Bug 1265082 - ESLint jobs are apparently hitting the network r?pbro → MozReview Request: Bug 1265082 - ESLint jobs are apparently hitting the network r?jryans
Attachment #8749698 - Flags: review?(jryans)
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #43)
> Comment on attachment 8749698 [details]
> MozReview Request: Bug 1265082 - ESLint jobs are apparently hitting the
> network r?pbro
> 
> https://reviewboard.mozilla.org/r/51101/#review48103
> 
> This approach could work, but do we have permission to create a top level
> node_modules directory?  It looks like this was discussed, but I don't see
> approval for it in the bug.  I am also not sure who you ask anymore to be
> honest (it used to be Brendan).
> 
> Also, I am somewhat worried about this process with a top level node_modules
> directory.  If someone else wants to start using the top level node_modules
> directory also for other modules, then it would compete with the modules you
> are installing here and then we'd have several different people who need to
> update the global node_modules bundle in tooltool... it sounds kind of
> messy.  We could decide to accept this and deal with it, but I think we may
> need a broader discussion first if we want to go that route.
> 
> Since the goal of this bug is _not_ "Create a global node_modules folder",
> what if you use basically this approach, but you keep the node_modules
> limited to inside testing somewhere?  Then we can leave discussion of a
> global node_modules as a follow up thing, since we probably have to hash it
> out on dev-platform or somewhere first.
> 
> Another alternative is to keep using the Docker image instead of tooltool
> like Dustin mentions in comment 26, though you may still want a limited
> location for the modules that is not the top level directory, since you want
> it to match for local installs as well.
> 

We have been planning on installing our node modules locally... it is a very highly requested feature. Anyhow, after discussing this on IRC it seems clear that the way to go is to install it in the component it is designed to target e.g. testing/eslint/node_modules. This way multiple modules can have their own private node_modules folder.

> ::: python/mach_commands.py:279
> (Diff revision 2)
> >              return 1
> >  
> > +        # eslint-plugin-mozilla should **never** be installed (linked) globally.
> > +        # Let's unlink it just in case.
> > +        self.callProcess("remove-eslint-plugin-mozilla",
> > +                                   [npmPath, "unlink", "eslint-plugin-mozilla"])
> 
> Nit: this line seems indented incorrectly
> 

Fixed

> ::: python/mach_commands.py:308
> (Diff revision 2)
> > -                                   [npmPath, "install", "eslint-plugin-react@4.2.3", "-g"])
> > +                                   [npmPath, "install", "eslint-plugin-react@4.2.3"], root)
> >          if not success:
> >              return 1
> >  
> >          print("\nESLint and approved plugins installed successfully!")
> > +        print("\nNOTE: YOUR LOCAL ESLINT BINARY IS IN %s/node_modules/.bin/eslint\n" % root)
> 
> Nit: I don't think it needs to be ALL CAPS.
> 

Agreed and fixed.

> ::: testing/eslint-plugin-mozilla/update:56
> (Diff revision 2)
> > +echo "Adding eslint.tar.gz to tooltool..."
> > +rm testing/eslint-plugin-mozilla/manifest.tt
> > +./tooltool.py add --visibility public eslint.tar.gz
> > +
> > +echo "Uploading eslint.tar.gz to tooltool..."
> > +./tooltool.py upload --authentication-file=~/.tooltool-token --message "node_modules folder update"
> 
> These steps seem logical, but I don't have tooltool upload access so it's
> hard for me to be sure.  Should Dustin or someone check it as well?

The fact that the task passes on try is proof that the tooltool process is working just fine.
Comment on attachment 8749698 [details]
MozReview Request: Bug 1265082 - ESLint jobs are apparently hitting the network r=me,dustin,pbro,jryans

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51101/diff/3-4/
Comment on attachment 8749698 [details]
MozReview Request: Bug 1265082 - ESLint jobs are apparently hitting the network r=me,dustin,pbro,jryans

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51101/diff/4-5/
Comment on attachment 8749698 [details]
MozReview Request: Bug 1265082 - ESLint jobs are apparently hitting the network r=me,dustin,pbro,jryans

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51101/diff/5-6/
Attachment #8749698 - Attachment description: MozReview Request: Bug 1265082 - ESLint jobs are apparently hitting the network r?jryans → MozReview Request: Bug 1265082 - ESLint jobs are apparently hitting the network r=me,dustin,pbro,jryans
Attachment #8749698 - Flags: review?(pbrosset)
Attachment #8749698 - Flags: review?(mratcliffe)
Attachment #8749698 - Flags: review?(jryans)
Attachment #8749698 - Flags: review?(dustin)
Attachment #8749698 - Flags: review?(pbrosset)
Attachment #8749698 - Flags: review?(jryans)
Attachment #8749698 - Flags: review?(dustin)
Comment on attachment 8749698 [details]
MozReview Request: Bug 1265082 - ESLint jobs are apparently hitting the network r=me,dustin,pbro,jryans

dustin, jryans and pbro have all reviewed different parts of this bug so marking as r+.
Attachment #8749698 - Flags: review?(mratcliffe) → review+
Comment on attachment 8749698 [details]
MozReview Request: Bug 1265082 - ESLint jobs are apparently hitting the network r=me,dustin,pbro,jryans

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51101/diff/6-7/
Attachment #8749698 - Flags: review?(pbrosset)
Attachment #8749698 - Flags: review?(jryans)
Attachment #8749698 - Flags: review?(dustin)
Attachment #8749698 - Flags: review+
Attachment #8749698 - Flags: review?(pbrosset)
Attachment #8749698 - Flags: review?(jryans)
Attachment #8749698 - Flags: review?(dustin)
Attachment #8749698 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/b4564c057f93
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Can ESLint become tier 1 again now, or are there other steps first?
Flags: needinfo?(wkocher)
We'll need to either get this uplifted to all still-used branches, or we'll have to just make it tier-1 for trunk, and then mark other trees as tier-1 as the release cycle progresses.
Flags: needinfo?(wkocher)
I've made ESLint tier-1 on trunk trees for now. Feel free to attempt uplifts if you'd like to get aurora/beta/etc tier-1'd as well. :)
Blocks: 1229194
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.