Repository: eslint/eslint
Pull request #9217 (2017-09-03 05:44:40)

Build: avoid process leak when generating website
<!--
    ESLint adheres to the [JS Foundation Code of Conduct](https://js.foundation/community/code-of-conduct).
-->

**What is the purpose of this pull request? (put an "X" next to item)**

[x] Bug fix

**Tell us about your environment**

* **ESLint Version:** master
* **Node Version:** 8.4.0
* **npm Version:** 5.3.0

**What parser (default, Babel-ESLint, etc.) are you using?**

N/A

**Please show your full configuration:**

N/A

**What did you do? Please include the actual source code causing the issue.**

I cloned the ESLint repo and ran

```bash
$ node Makefile.js gensite
```

**What did you expect to happen?**

I expected the website to generate in a reasonable amount of time, and I expected the build script to not spawn hundreds of concurrent processes.

**What actually happened? Please include the actual, raw output from ESLint.**

The build script froze after generating about 300/350 docs files. Running `ps` in a separate terminal tab indicated that hundreds of processes had been created.

**What changes did you make? (Give an overview)**

This updates `Makefile.js` to use the `shelljs` functions as variables, rather than relying on `shelljs` to set its own globals.

We rely on [shelljs-nodecli](https://github.com/nzakas/shelljs-nodecli), which has a dependency on `shelljs@~0.2`. `shelljs-nodecli` is also overwriting all the `shelljs` globals (see https://github.com/nzakas/shelljs-nodecli/pull/5), so the `exec` global available from `Makefile.js` is actually the `exec` function from `shelljs@0.2.6`, even though we have `shelljs@0.7.8` as a direct dependency. The `exec` function from `shelljs@0.2.6` appears have an issue where it doesn't shut down the processes it creates, which causes the `gensite` script to freeze as it exceeds the process limit.

**Is there anything you'd like reviewers to focus on?**

Nothing in particular


eslintbot: LGTM

Patch:
diff --git a/Makefile.js b/Makefile.js
index 074739f3c..7e22731d2 100644
--- a/Makefile.js
+++ b/Makefile.js
@@ -3,7 +3,7 @@
  * @author nzakas
  */
 
-/* global cat, cd, cp, echo, exec, exit, find, ls, mkdir, pwd, rm, target, test*/
+/* global target */
 /* eslint no-use-before-define: "off", no-console: "off" */
 "use strict";
 
@@ -24,10 +24,24 @@ const lodash = require("lodash"),
     os = require("os"),
     path = require("path"),
     semver = require("semver"),
+    shell = require("shelljs"),
     ejs = require("ejs"),
     loadPerf = require("load-perf"),
     yaml = require("js-yaml");
 
+const cat = shell.cat;
+const cd = shell.cd;
+const cp = shell.cp;
+const echo = shell.echo;
+const exec = shell.exec;
+const exit = shell.exit;
+const find = shell.find;
+const ls = shell.ls;
+const mkdir = shell.mkdir;
+const pwd = shell.pwd;
+const rm = shell.rm;
+const test = shell.test;
+
 //------------------------------------------------------------------------------
 // Settings
 //------------------------------------------------------------------------------
@@ -146,7 +160,7 @@ function generateRulesIndex(basedir) {
  * @returns {string} The result of the executed command.
  */
 function execSilent(cmd) {
-    return exec(cmd, { silent: true }).output;
+    return exec(cmd, { silent: true }).stdout;
 }
 
 /**
@@ -329,7 +343,7 @@ function getFirstCommitOfFile(filePath) {
  * @param {string} filePath The file path to check.
  * @returns {string} The tag name.
  */
-function getTagOfFirstOccurrence(filePath) {
+function getFirstVersionOfFile(filePath) {
     const firstCommit = getFirstCommitOfFile(filePath);
     let tags = execSilent(`git tag --contains ${firstCommit}`);
 
@@ -343,15 +357,6 @@ function getTagOfFirstOccurrence(filePath) {
     }, []).sort(semver.compare)[0];
 }
 
-/**
- * Gets the version number where a given file was introduced first.
- * @param {string} filePath The file path to check.
- * @returns {string} The version number.
- */
-function getFirstVersionOfFile(filePath) {
-    return getTagOfFirstOccurrence(filePath);
-}
-
 /**
  * Gets the commit that deleted a file.
  * @param {string} filePath The path to the deleted file.
diff --git a/docs/rules/generator-star-spacing.md b/docs/rules/generator-star-spacing.md
index efa7a5a4e..95af6de80 100644
--- a/docs/rules/generator-star-spacing.md
+++ b/docs/rules/generator-star-spacing.md
@@ -93,7 +93,7 @@ An example of a configuration with overrides:
 ```
 
 In the example configuration above, the top level "before" and "after" options define the default behavior of
-the rule, while the "anonymous", "method", and "static" options override the default behavior.
+the rule, while the "anonymous" and "method" options override the default behavior.
 Overrides can be either an object with "before" and "after", or a shorthand string as above.
 
 ## Examples
diff --git a/lib/linter.js b/lib/linter.js
index 7e32eedb9..b06fb833e 100755
--- a/lib/linter.js
+++ b/lib/linter.js
@@ -178,7 +178,7 @@ function addDeclaredGlobals(program, globalScope, config, envContext) {
 
     Object.assign(declaredGlobals, builtin);
 
-    Object.keys(config.env).forEach(name => {
+    Object.keys(config.env).filter(name => config.env[name]).forEach(name => {
         const env = envContext.get(name),
             environmentGlobals = env && env.globals;
 


Test patch:
diff --git a/tests/lib/rules/no-catch-shadow.js b/tests/lib/rules/no-catch-shadow.js
index a302ae86b..3d1024f9e 100644
--- a/tests/lib/rules/no-catch-shadow.js
+++ b/tests/lib/rules/no-catch-shadow.js
@@ -36,6 +36,10 @@ ruleTester.run("no-catch-shadow", rule, {
                 "module.exports = broken;"
             ].join("\n"),
             parserOptions: { ecmaVersion: 6 }
+        },
+        {
+            code: "try {} catch (error) {}",
+            env: { shelljs: false }
         }
     ],
     invalid: [
diff --git a/tests/lib/rules/no-redeclare.js b/tests/lib/rules/no-redeclare.js
index 3d67961ed..2cd0f8ef5 100644
--- a/tests/lib/rules/no-redeclare.js
+++ b/tests/lib/rules/no-redeclare.js
@@ -35,7 +35,12 @@ ruleTester.run("no-redeclare", rule, {
         { code: "var top = 0;", env: { browser: true } },
         { code: "var top = 0;", options: [{ builtinGlobals: true }] },
         { code: "var top = 0;", options: [{ builtinGlobals: true }], parserOptions: { ecmaFeatures: { globalReturn: true } }, env: { browser: true } },
-        { code: "var top = 0;", options: [{ builtinGlobals: true }], parserOptions: { sourceType: "module" }, env: { browser: true } }
+        { code: "var top = 0;", options: [{ builtinGlobals: true }], parserOptions: { sourceType: "module" }, env: { browser: true } },
+        {
+            code: "var self = 1",
+            options: [{ builtinGlobals: true }],
+            env: { browser: false }
+        }
     ],
     invalid: [
         { code: "var a = 3; var a = 10;", parserOptions: { ecmaVersion: 6 }, errors: [{ message: "'a' is already defined.", type: "Identifier" }] },