Skip to content

Conversation

@dherman
Copy link
Collaborator

@dherman dherman commented Dec 25, 2016

(Pulled out from #122.)

For some reason, node-gyp never exits if we run `npm` commands without
the `--silent` option. Passing `--no-progress --no-color` and/or
capturing the output of stderr and stdout doesn't help.

This will still make the npm commands fail in case something goes wrong,
and won't suppress the node-gyp output when running cargo in verbose
mode.
@dherman
Copy link
Collaborator Author

dherman commented Dec 25, 2016

r? @jedireza

@jedireza
Copy link
Contributor

jedireza commented Dec 25, 2016

r+ to the changes, they're simple enough.

I wonder what we lose by adding --silent? At first glance this sounds like a bug that should be filed up-stream.

If we get around this [Windows bug] using --silent do we want to create an issue that tracks this and removes the argument when it's not necessary?

Maybe we could apply this argument for Windows users only.

I only hesitate because we're silencing something that may be helpful when looking at build logs or during development.

@jedireza
Copy link
Contributor

jedireza commented Dec 26, 2016

I see now that the output of cargo build scripts are suppressed anyways, unless an error occurs.

There is a -vv argument (possibly undocumented) we can pass to cargo to see the output of the build script while it's running.

This can be helpful in the case of windows builds hanging since an error hasn't actually occurred yet (process hasn't finished). With this [-vv] I'm able to get more insight into what's causing the hang (possibly something related to $ node-gyp rebuild).

@dherman
Copy link
Collaborator Author

dherman commented Jan 5, 2017

OK, it took a while but the bug has been identified in Rust's Command API, and a PR has been filed:

rust-lang/rust#38835

So we should just go ahead with the workaround but leave a comment to remove the workaround once the Rust fix has been in long enough. We may want to wait at least a couple of Rust release cycles so people can use older Rusts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants