-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(build): Stop scripts from trying to read user input during install #5632
Conversation
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.
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. |
Ignore the AppVeyor failure until we merge #5638. |
I have one concern: |
I don't think we should change |
Yes to both. I'll look into the |
|
__tests__/commands/version.js
Outdated
config, | ||
cmd: pkg.scripts.preversion, | ||
cwd: config.cwd, | ||
isInteractive: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@iarna - I think we need to mark postX
and preX
scripts on the main package as interactive. What do you think?
/cc @bestander @arcanis
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.)
src/cli/commands/run.js
Outdated
config, | ||
cmd: cmdWithArgs, | ||
cwd: config.cwd, | ||
isInteractive, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the pre/post scripts can be left as non-interactive, this should be fine
@@ -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} = {}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes to both :)
@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! |
src/cli/commands/run.js
Outdated
} else { | ||
await execCommand(stage, config, cmdWithArgs, config.cwd); | ||
await execCommand({stage, config, cmd: cmdWithArgs, cwd: config.cwd, isInteractive: true}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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? :)
src/util/execute-lifecycle-script.js
Outdated
// 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'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ended up using your version since it is clearly more readable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add at least one integration test to make sure the new behavior is correct since we missed this earlier.
src/package-install-scripts.js
Outdated
@@ -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), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also fixed a minor issue here when you only had 1 install script but you ended up seeing a bunch of [-/1] waiting...
messages.
@BYK looks good! |
I've merged this since the single build failure was due to a flaky test on master. I'll fix that separately: #5700 |
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!) |
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>
.
|
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.