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

iarna · 2018-04-10T00:40:18Z

Summary

Fixes #976.

@BYK mentioned to me that this was a problem you all had been having and while doing research for npm running lifecycle scripts in parallel I came across this solution and thought I'd share it with y'all:

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.

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.

BYK · 2018-04-10T11:22:09Z

Some failing tests are failing on master too. Submitted #5635 to fix them. I'll merge from master once that is merged and then this should be good to go.

BYK · 2018-04-10T15:01:38Z

Ignore the AppVeyor failure until we merge #5638.

BYK · 2018-04-11T10:32:38Z

I have one concern: yarn run x uses the same function and in that scenario, I think allowing stdin is something we want. If you agree, I'll add a new flag to this function, interactive or allowInteractive to disable this new behavior for yarn run.

arcanis · 2018-04-11T15:04:36Z

I don't think we should change yarn run's default behavior - it would be quite unexpected if programs that expect an stdin suddenly stopped working. Isn't the original issue only about the spinner, ie scripts executed during install?

BYK · 2018-04-11T15:31:49Z

I don't think we should change yarn run's default behavior - it would be quite unexpected if programs that expect an stdin suddenly stopped working. Isn't the original issue only about the spinner, ie scripts executed during install?

Yes to both. I'll look into the spinner option and see if it is used by anything else. If not I'll rename it and switch the behavior to 'inherit' everything like we currently do.

buildsize · 2018-04-11T16:47:27Z

File name	Previous Size	New Size	Change
yarn-[version].noarch.rpm	907.78 KB	907.98 KB	205 bytes (0%)
yarn-[version].js	3.94 MB	3.94 MB	733 bytes (0%)
yarn-legacy-[version].js	4.1 MB	4.1 MB	1006 bytes (0%)
yarn-v[version].tar.gz	912.75 KB	912.97 KB	223 bytes (0%)
yarn_[version]all.deb	674.58 KB	674.67 KB	92 bytes (0%)

BYK · 2018-04-12T09:45:35Z

__tests__/commands/version.js

+      config,
+      cmd: pkg.scripts.preversion,
+      cwd: config.cwd,
+      isInteractive: false,


@iarna - I think we need to mark postX and preX scripts on the main package as interactive. What do you think?

/cc @bestander @arcanis

Currently the plan for npm is that ONLY preinstall, install and postinstall will be run non-interactively and in parallel. Everything else (like preversion, version & postversion) would continue as it is.

The "why" of where the line is drawn gets down to why we would ever run something non-interactively. I see there being two reasons:

It allows for parallel execution which is a dramatic speed boost for projects with a lot of compiled dependencies.

It hides a bunch of output that is uninteresting and non-actionable, making it easier for users to see import warning and notice output.

Both of these apply to the install suite of scripts, and only the second applies elsewhere. As the others are ordinarily only run on the main package I don't think they can justify non-interactivity.

(I would keep preinstall and postinstall non-interactive on the main package for consistency and so that folks don't end up surprised when they publish something with interactivity in one of those. The rule of thumb being: If it won't work published it probably shouldn't work in dev.)

BYK · 2018-04-12T09:46:29Z

src/cli/commands/run.js

+            config,
+            cmd: cmdWithArgs,
+            cwd: config.cwd,
+            isInteractive,


If we decide to make postX and preX scripts interactive, then this line should simply become isInteractive: true and all other changes above should be removed.

I think the pre/post scripts can be left as non-interactive, this should be fine

arcanis · 2018-04-13T17:06:25Z

src/util/child.js

@@ -61,7 +61,7 @@ type ProcessFn = (
 export function spawn(
  program: string,
  args: Array<string>,
-  opts?: child_process$spawnOpts & {process?: ProcessFn} = {},
+  opts?: child_process$spawnOpts & {detached?: boolean, process?: ProcessFn} = {},


Why do we need to declared detached? Is it missing in the flow definition for child_process$spawnOpts? In that case, maybe we should make a PR to add it

Yes to both :)

BYK · 2018-04-15T13:28:54Z

@iarna - updated as you suggested. Once you give me the go, I'll merge it.

Let us know if you want someone from @yarnpkg/core to give it a second look before merging tho.

Thanks again for submitting this!

kaylie-alexa · 2018-04-16T21:33:06Z

src/cli/commands/run.js

        } else {
-          await execCommand(stage, config, cmdWithArgs, config.cwd);
+          await execCommand({stage, config, cmd: cmdWithArgs, cwd: config.cwd, isInteractive: true});


Thanks for cleaning this up with named parameters! Can we do

await execCommand({ stage, config, cmd: cmdWithArgs, cwd: config.cwd, isInteractive: true, customShell: customShell && String(customShell) });

and remove the conditional now? :)

kaylie-alexa · 2018-04-16T21:40:42Z

src/util/execute-lifecycle-script.js

+  // if this is not interactive, run child processes detached so they can't hang
+  // trying to read from /dev/tty but not on Windows, because Windows opens
+  // a new console window and there is no /dev/tty anyway
+  const detached = !isInteractive && process.platform !== 'win32';


I find it slightly more readable this way, wdyt?

// By default (non-interactive), pipe everything to the terminal and run child process detached // as long as it's not Windows (since windows does not have /dev/tty) let stdio; let detached = process.platform !== 'win32'; if (isInteractive) { stdio = 'inherit'; detached = false; }

Don't really like using let there but I also agree that your version expresses the intent more clearly. I'll see if I can find a middle ground :)

I ended up using your version since it is clearly more readable.

BYK

I'll add at least one integration test to make sure the new behavior is correct since we missed this earlier.

BYK · 2018-04-18T14:10:22Z

src/package-install-scripts.js

@@ -279,18 +301,15 @@ export default class PackageInstallScripts {
      workQueue.add(pkg);
    }

+    const set = this.reporter.activitySet(
+      installablePkgs,
+      Math.min(installablePkgs, this.config.childConcurrency, workQueue.size),


Also fixed a minor issue here when you only had 1 install script but you ended up seeing a bunch of [-/1] waiting... messages.

iarna · 2018-04-18T17:57:44Z

@BYK looks good!

BYK · 2018-04-18T22:05:38Z

I've merged this since the single build failure was due to a flaky test on master. I'll fix that separately: #5700

TallOrderDev · 2018-04-19T19:59:36Z

Is this live now? Running into the issue and not sure if I just need to upgrade yarn or not (Perfect timing if it is live!)

arcanis · 2018-04-19T20:18:06Z

It has been merged, but not yet released. You're free to use the nightly builds, which should be stable enough anyway (we merge only if the tests pass) :) https://yarnpkg.com/en/docs/nightly

…

On Thu, Apr 19, 2018, 8:59 PM John Gill ***@***.***> wrote: Is this live now? Running into the issue and not sure if I just need to upgrade yarn or not (Perfect timing if it is live!) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5632 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA_Wa8Td3URVtoCyZTSFFRPzJwY2tXGYks5tqOyzgaJpZM4TNblc> .

iarna force-pushed the patch-1 branch from cf34e3d to 788ed9a Compare April 10, 2018 02:01

BYK added 2 commits April 10, 2018 12:03

Fix detached not being boolean without spinner

ffe6896

Fix flow type error

3ef8374

BYK self-assigned this Apr 10, 2018

BYK requested review from rally25rs, arcanis and bestander April 10, 2018 11:22

Merge branch 'master' into blockstdin

67b13b4

Merge branch 'master' into blockstdin

7a73029

BYK changed the title ~~Fix: Stop scripts from trying to read user input~~ fix(build): Stop scripts from trying to read user input during install Apr 11, 2018

BYK added 2 commits April 11, 2018 16:32

Merge branch 'master' into blockstdin

d12d515

Refactor how run works and introduce isInteractive

3eb41cc

BYK added 2 commits April 12, 2018 10:39

Fix tests

8d6fb26

Merge branch 'master' into blockstdin

e70c849

BYK reviewed Apr 12, 2018

View reviewed changes

arcanis approved these changes Apr 13, 2018

View reviewed changes

BYK added 2 commits April 15, 2018 14:14

Merge branch 'master' into blockstdin

7491db7

Only make pcakage insall scripts non-interactive

c135044

kaylie-alexa reviewed Apr 16, 2018

View reviewed changes

BYK added 3 commits April 17, 2018 17:55

Merge branch 'master' into blockstdin

fe348a5

Merge branch 'master' into blockstdin

f88538b

Make code more readable

230dd2a

BYK added 2 commits April 18, 2018 11:29

Fix lint

782de04

Fix error in logic when ignoring stdin

d391efc

BYK reviewed Apr 18, 2018

View reviewed changes

Remove obsolete argument

3a23fd3

BYK added 3 commits April 18, 2018 22:20

Add integration test

5957111

Merge branch 'master' into blockstdin

47d374f

Don't use snapshots for stdout

05d4338

BYK merged commit 2f0eb8e into yarnpkg:master Apr 18, 2018

allomov mentioned this pull request May 29, 2019

Bug: unable to install semantic-ui #976

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

iarna commented Apr 10, 2018 •

edited by BYK

Loading

BYK commented Apr 10, 2018

BYK commented Apr 10, 2018

BYK commented Apr 11, 2018

arcanis commented Apr 11, 2018

BYK commented Apr 11, 2018

buildsize bot commented Apr 11, 2018 •

edited

Loading

BYK Apr 12, 2018

iarna Apr 13, 2018

BYK Apr 12, 2018

arcanis Apr 13, 2018

arcanis Apr 13, 2018

BYK Apr 15, 2018

BYK commented Apr 15, 2018

kaylie-alexa Apr 16, 2018

kaylie-alexa Apr 16, 2018

BYK Apr 17, 2018

BYK Apr 18, 2018

BYK left a comment

BYK Apr 18, 2018 •

edited

Loading

iarna commented Apr 18, 2018

BYK commented Apr 18, 2018 •

edited

Loading

TallOrderDev commented Apr 19, 2018

arcanis commented Apr 19, 2018 via email

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

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

Conversation

iarna commented Apr 10, 2018 • edited by BYK Loading

BYK commented Apr 10, 2018

BYK commented Apr 10, 2018

BYK commented Apr 11, 2018

arcanis commented Apr 11, 2018

BYK commented Apr 11, 2018

buildsize bot commented Apr 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BYK commented Apr 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BYK left a comment

Choose a reason for hiding this comment

BYK Apr 18, 2018 • edited Loading

Choose a reason for hiding this comment

iarna commented Apr 18, 2018

BYK commented Apr 18, 2018 • edited Loading

TallOrderDev commented Apr 19, 2018

arcanis commented Apr 19, 2018 via email

iarna commented Apr 10, 2018 •

edited by BYK

Loading

buildsize bot commented Apr 11, 2018 •

edited

Loading

BYK Apr 18, 2018 •

edited

Loading

BYK commented Apr 18, 2018 •

edited

Loading