fix(build): Stop scripts from trying to read user input during install (

#5632) **Summary** Fixes #976. This is a bug fix to the problem where lifecycle scripts that try to go interactive hang yarn installs with no indication as to why. This sets stdin to /dev/null and, on unix-like systems, detaches the process from the terminal making it unable to read /dev/tty. Previously with the spinner enabled was impossible to give input to an interactive script on stdin, because a pipe was kept open from the main yarn process to the child process but it was never written to. Interactive scripts could previously use /dev/tty on unix-likes to bypass this, however the fact that scripts are run in parallel means that if two scripts go interactive in this manner that they'll step on each other. This does not change the behavior if the spinner is disabled. Also added a simple integration test with a blocking install script that times out without the patch. **Test plan** Try running `yarn add semantic-ui`. Hangs without the patch, fails with the patch.
yarnpkg · Apr 18, 2018 · 2f0eb8e · 2f0eb8e
1 parent f1b63ef
commit 2f0eb8e
Show file tree

Hide file tree

Showing 12 changed files with 239 additions and 131 deletions.
diff --git a/__tests__/commands/install/__snapshots__/integration.js.snap b/__tests__/commands/install/__snapshots__/integration.js.snap
@@ -1,6 +1,6 @@
 // Jest Snapshot v1, https://goo.gl/fbAQLP
 
-exports[`install should not hoist packages above their peer dependencies: install-file-as-default-no-semver 1`] = `
+exports[`don't install with file: protocol as default if target is valid semver: install-file-as-default-no-semver 1`] = `
 "{
   \\"author\\": \\"AJ ONeal <coolaj86@gmail.com> (http://coolaj86.info)\\",
   \\"name\\": \\"foo\\",

diff --git a/__tests__/commands/install/integration.js b/__tests__/commands/install/integration.js
@@ -492,14 +492,20 @@ test.concurrent('install with explicit file: protocol if target does not have pa
   });
 });
 
-test.concurrent("don't install with file: protocol as default if target is valid semver", (): Promise<void> => {
+test("don't install with file: protocol as default if target is valid semver", (): Promise<void> => {
   return runInstall({}, 'install-file-as-default-no-semver', async config => {
     expect(await fs.readFile(path.join(config.cwd, 'node_modules', 'foo', 'package.json'))).toMatchSnapshot(
       'install-file-as-default-no-semver',
     );
   });
 });
 
+test.concurrent("don't hang when an install script tries to read from stdin", (): Promise<void> =>
+  runInstall({}, 'install-blocking-script', (_config, _reporter, _install, getStdout) =>
+    expect(getStdout()).toMatch(/Building fresh packages/),
+  ),
+);
+
 // When local packages are installed, dependencies with different forms of the same relative path
 // should be deduped e.g. 'file:b' and 'file:./b'
 test.concurrent('install file: dedupe dependencies 1', (): Promise<void> => {

diff --git a/__tests__/commands/run.js b/__tests__/commands/run.js
@@ -50,8 +50,8 @@ const runRunWithCustomShell = function(customShell, ...args): Promise<void> {
   })(...args);
 };
 
-test('lists all available commands with no arguments', (): Promise<void> => {
-  return runRun([], {}, 'no-args', (config, reporter): ?Promise<void> => {
+test('lists all available commands with no arguments', (): Promise<void> =>
+  runRun([], {}, 'no-args', (config, reporter): ?Promise<void> => {
     const rprtr = new reporters.BufferReporter({stdout: null, stdin: null});
     const scripts = ['build', 'prestart', 'start'];
     const hints = {
@@ -69,43 +69,48 @@ test('lists all available commands with no arguments', (): Promise<void> => {
     rprtr.error(rprtr.lang('commandNotSpecified'));
 
     expect(reporter.getBuffer()).toEqual(rprtr.getBuffer());
-  });
-});
+  }));
 
-test('runs script containing spaces', (): Promise<void> => {
-  return runRun(['build'], {}, 'spaces', async (config): ?Promise<void> => {
+test('runs script containing spaces', (): Promise<void> =>
+  runRun(['build'], {}, 'spaces', async (config): ?Promise<void> => {
     const pkg = await fs.readJson(path.join(config.cwd, 'package.json'));
     // The command gets called with a space appended
-    const args = ['build', config, pkg.scripts.build, config.cwd];
-
-    expect(execCommand).toBeCalledWith(...args);
-  });
-});
+    expect(execCommand).toBeCalledWith({
+      stage: 'build',
+      config,
+      cmd: pkg.scripts.build,
+      cwd: config.cwd,
+      isInteractive: true,
+    });
+  }));
 
-test('properly handles extra arguments and pre/post scripts', (): Promise<void> => {
-  return runRun(['start', '--hello'], {}, 'extra-args', async (config): ?Promise<void> => {
+test('properly handles extra arguments and pre/post scripts', (): Promise<void> =>
+  runRun(['start', '--hello'], {}, 'extra-args', async (config): ?Promise<void> => {
     const pkg = await fs.readJson(path.join(config.cwd, 'package.json'));
-    const poststart = ['poststart', config, pkg.scripts.poststart, config.cwd];
-    const prestart = ['prestart', config, pkg.scripts.prestart, config.cwd];
-    const start = ['start', config, pkg.scripts.start + ' --hello', config.cwd];
-
-    expect(execCommand.mock.calls[0]).toEqual(prestart);
-    expect(execCommand.mock.calls[1]).toEqual(start);
-    expect(execCommand.mock.calls[2]).toEqual(poststart);
-  });
-});
+    const poststart = {stage: 'poststart', config, cmd: pkg.scripts.poststart, cwd: config.cwd, isInteractive: true};
+    const prestart = {stage: 'prestart', config, cmd: pkg.scripts.prestart, cwd: config.cwd, isInteractive: true};
+    const start = {stage: 'start', config, cmd: pkg.scripts.start + ' --hello', cwd: config.cwd, isInteractive: true};
 
-test('properly handle bin scripts', (): Promise<void> => {
-  return runRun(['cat-names'], {}, 'bin', config => {
+    expect(execCommand.mock.calls[0]).toEqual([prestart]);
+    expect(execCommand.mock.calls[1]).toEqual([start]);
+    expect(execCommand.mock.calls[2]).toEqual([poststart]);
+  }));
+
+test('properly handle bin scripts', (): Promise<void> =>
+  runRun(['cat-names'], {}, 'bin', config => {
     const script = path.join(config.cwd, 'node_modules', '.bin', 'cat-names');
-    const args = ['cat-names', config, script, config.cwd];
 
-    expect(execCommand).toBeCalledWith(...args);
-  });
-});
+    expect(execCommand).toBeCalledWith({
+      stage: 'cat-names',
+      config,
+      cmd: script,
+      cwd: config.cwd,
+      isInteractive: true,
+    });
+  }));
 
-test('properly handle env command', (): Promise<void> => {
-  return runRun(['env'], {}, 'no-args', (config, reporter): ?Promise<void> => {
+test('properly handle env command', (): Promise<void> =>
+  runRun(['env'], {}, 'no-args', (config, reporter): ?Promise<void> => {
     // $FlowFixMe
     const result = JSON.parse(reporter.getBuffer()[0].data);
 
@@ -134,58 +139,66 @@ test('properly handle env command', (): Promise<void> => {
     expect(result).toHaveProperty('npm_lifecycle_event');
     expect(result).toHaveProperty('npm_execpath');
     expect(result).toHaveProperty('npm_node_execpath');
-  });
-});
+  }));
 
-test('adds string delimiters if args have spaces', (): Promise<void> => {
-  return runRun(['cat-names', '--filter', 'cat names'], {}, 'bin', config => {
+test('adds string delimiters if args have spaces', (): Promise<void> =>
+  runRun(['cat-names', '--filter', 'cat names'], {}, 'bin', config => {
     const script = path.join(config.cwd, 'node_modules', '.bin', 'cat-names');
     const q = process.platform === 'win32' ? '"' : "'";
-    const args = ['cat-names', config, `${script} --filter ${q}cat names${q}`, config.cwd];
 
-    expect(execCommand).toBeCalledWith(...args);
-  });
-});
+    expect(execCommand).toBeCalledWith({
+      stage: 'cat-names',
+      config,
+      cmd: `${script} --filter ${q}cat names${q}`,
+      cwd: config.cwd,
+      isInteractive: true,
+    });
+  }));
 
-test('adds quotes if args have spaces and quotes', (): Promise<void> => {
-  return runRun(['cat-names', '--filter', '"cat names"'], {}, 'bin', config => {
+test('adds quotes if args have spaces and quotes', (): Promise<void> =>
+  runRun(['cat-names', '--filter', '"cat names"'], {}, 'bin', config => {
     const script = path.join(config.cwd, 'node_modules', '.bin', 'cat-names');
     const quotedCatNames = process.platform === 'win32' ? '^"\\^"cat^ names\\^"^"' : `'"cat names"'`;
-    const args = ['cat-names', config, `${script} --filter ${quotedCatNames}`, config.cwd];
 
-    expect(execCommand).toBeCalledWith(...args);
-  });
-});
+    expect(execCommand).toBeCalledWith({
+      stage: 'cat-names',
+      config,
+      cmd: `${script} --filter ${quotedCatNames}`,
+      cwd: config.cwd,
+      isInteractive: true,
+    });
+  }));
 
-test('returns noScriptsAvailable with no scripts', (): Promise<void> => {
-  return runRun([], {}, 'no-scripts', (config, reporter) => {
+test('returns noScriptsAvailable with no scripts', (): Promise<void> =>
+  runRun([], {}, 'no-scripts', (config, reporter) => {
     expect(reporter.getBuffer()).toMatchSnapshot();
-  });
-});
+  }));
 
-test('returns noBinAvailable with no bins', (): Promise<void> => {
-  return runRun([], {}, 'no-bin', (config, reporter) => {
+test('returns noBinAvailable with no bins', (): Promise<void> =>
+  runRun([], {}, 'no-bin', (config, reporter) => {
     expect(reporter.getBuffer()).toMatchSnapshot();
-  });
-});
+  }));
 
-test('adds workspace root node_modules/.bin to path when in a workspace', (): Promise<void> => {
-  return runRunInWorkspacePackage('packages/pkg1', ['env'], {}, 'workspace', (config, reporter): ?Promise<void> => {
+test('adds workspace root node_modules/.bin to path when in a workspace', (): Promise<void> =>
+  runRunInWorkspacePackage('packages/pkg1', ['env'], {}, 'workspace', (config, reporter): ?Promise<void> => {
     const logEntry = reporter.getBuffer().find(entry => entry.type === 'log');
     const parsedLogData = JSON.parse(logEntry ? logEntry.data.toString() : '{}');
     const envPaths = (parsedLogData.PATH || parsedLogData.Path).split(path.delimiter);
 
     expect(envPaths).toContain(path.join(config.cwd, 'node_modules', '.bin'));
     expect(envPaths).toContain(path.join(config.cwd, 'packages', 'pkg1', 'node_modules', '.bin'));
-  });
-});
+  }));
 
-test('runs script with custom script-shell', (): Promise<void> => {
-  return runRunWithCustomShell('/usr/bin/dummy', ['start'], {}, 'script-shell', async (config): ?Promise<void> => {
+test('runs script with custom script-shell', (): Promise<void> =>
+  runRunWithCustomShell('/usr/bin/dummy', ['start'], {}, 'script-shell', async (config): ?Promise<void> => {
     const pkg = await fs.readJson(path.join(config.cwd, 'package.json'));
     // The command gets called with the provided customShell
-    const args = ['start', config, pkg.scripts.start, config.cwd, '/usr/bin/dummy'];
-
-    expect(execCommand).toBeCalledWith(...args);
-  });
-});
+    expect(execCommand).toBeCalledWith({
+      stage: 'start',
+      config,
+      cmd: pkg.scripts.start,
+      cwd: config.cwd,
+      isInteractive: true,
+      customShell: '/usr/bin/dummy',
+    });
+  }));
diff --git a/__tests__/commands/version.js b/__tests__/commands/version.js
@@ -75,29 +75,59 @@ test('run version and make sure all lifecycle steps are executed', (): Promise<v
   return runRun([], {newVersion, gitTagVersion}, 'no-args', async (config): ?Promise<void> => {
     const pkg = await fs.readJson(path.join(config.cwd, 'package.json'));
 
-    const preversionLifecycle = ['preversion', config, pkg.scripts.preversion, config.cwd];
-    const versionLifecycle = ['version', config, pkg.scripts.version, config.cwd];
-    const postversionLifecycle = ['postversion', config, pkg.scripts.postversion, config.cwd];
+    const preversionLifecycle = {
+      stage: 'preversion',
+      config,
+      cmd: pkg.scripts.preversion,
+      cwd: config.cwd,
+      isInteractive: true,
+    };
+    const versionLifecycle = {
+      stage: 'version',
+      config,
+      cmd: pkg.scripts.version,
+      cwd: config.cwd,
+      isInteractive: true,
+    };
+    const postversionLifecycle = {
+      stage: 'postversion',
+      config,
+      cmd: pkg.scripts.postversion,
+      cwd: config.cwd,
+      isInteractive: true,
+    };
 
     expect(execCommand.mock.calls.length).toBe(3);
 
-    expect(execCommand.mock.calls[0]).toEqual(preversionLifecycle);
-    expect(execCommand.mock.calls[1]).toEqual(versionLifecycle);
-    expect(execCommand.mock.calls[2]).toEqual(postversionLifecycle);
+    expect(execCommand.mock.calls[0]).toEqual([preversionLifecycle]);
+    expect(execCommand.mock.calls[1]).toEqual([versionLifecycle]);
+    expect(execCommand.mock.calls[2]).toEqual([postversionLifecycle]);
   });
 });
 
 test('run version and make sure only the defined lifecycle steps are executed', (): Promise<void> => {
   return runRun([], {newVersion, gitTagVersion}, 'pre-post', async (config): ?Promise<void> => {
     const pkg = await fs.readJson(path.join(config.cwd, 'package.json'));
 
-    const preversionLifecycle = ['preversion', config, pkg.scripts.preversion, config.cwd];
-    const postversionLifecycle = ['postversion', config, pkg.scripts.postversion, config.cwd];
+    const preversionLifecycle = {
+      stage: 'preversion',
+      config,
+      cmd: pkg.scripts.preversion,
+      cwd: config.cwd,
+      isInteractive: true,
+    };
+    const postversionLifecycle = {
+      stage: 'postversion',
+      config,
+      cmd: pkg.scripts.postversion,
+      cwd: config.cwd,
+      isInteractive: true,
+    };
 
     expect(execCommand.mock.calls.length).toBe(2);
 
-    expect(execCommand.mock.calls[0]).toEqual(preversionLifecycle);
-    expect(execCommand.mock.calls[1]).toEqual(postversionLifecycle);
+    expect(execCommand.mock.calls[0]).toEqual([preversionLifecycle]);
+    expect(execCommand.mock.calls[1]).toEqual([postversionLifecycle]);
   });
 });
 

diff --git a/__tests__/fixtures/install/install-blocking-script/blocking/install.js b/__tests__/fixtures/install/install-blocking-script/blocking/install.js
@@ -0,0 +1,25 @@
+const readline = require('readline');
+const rl = readline.createInterface({
+  input: process.stdin,
+  output: process.stdout,
+  prompt: 'OHAI> ',
+});
+
+rl.prompt();
+
+rl
+  .on('line', line => {
+    switch (line.trim()) {
+      case 'hello':
+        console.log('world!');
+        break;
+      default:
+        console.log(`Say what? I might have heard '${line.trim()}'`);
+        break;
+    }
+    rl.prompt();
+  })
+  .on('close', () => {
+    console.log('Have a great day!');
+    process.exit(0);
+  });
diff --git a/__tests__/fixtures/install/install-blocking-script/blocking/package.json b/__tests__/fixtures/install/install-blocking-script/blocking/package.json
@@ -0,0 +1,7 @@
+{
+  "name": "blocking",
+  "version": "0.0.0",
+  "scripts": {
+    "install": "node install.js"
+  }
+}
diff --git a/__tests__/fixtures/install/install-blocking-script/package.json b/__tests__/fixtures/install/install-blocking-script/package.json
@@ -0,0 +1,5 @@
+{
+  "dependencies": {
+    "blocking": "file:blocking"
+  }
+}
diff --git a/src/cli/commands/run.js b/src/cli/commands/run.js
@@ -82,11 +82,14 @@ export async function run(config: Config, reporter: Reporter, flags: Object, arg
         // only tack on trailing arguments for default script, ignore for pre and post - #1595
         const cmdWithArgs = stage === action ? sh`${unquoted(cmd)} ${args}` : cmd;
         const customShell = config.getOption('script-shell');
-        if (customShell) {
-          await execCommand(stage, config, cmdWithArgs, config.cwd, String(customShell));
-        } else {
-          await execCommand(stage, config, cmdWithArgs, config.cwd);
-        }
+        await execCommand({
+          stage,
+          config,
+          cmd: cmdWithArgs,
+          cwd: config.cwd,
+          isInteractive: true,
+          customShell: customShell ? String(customShell) : undefined,
+        });
       }
     } else if (action === 'env') {
       reporter.log(JSON.stringify(await makeEnv('env', config.cwd, config), null, 2), {force: true});

diff --git a/src/cli/commands/version.js b/src/cli/commands/version.js
@@ -52,7 +52,7 @@ export async function setVersion(
 
   function runLifecycle(lifecycle: string): Promise<void> {
     if (scripts[lifecycle]) {
-      return execCommand(lifecycle, config, scripts[lifecycle], config.cwd);
+      return execCommand({stage: lifecycle, config, cmd: scripts[lifecycle], cwd: config.cwd, isInteractive: true});
     }
 
     return Promise.resolve();