Skip to content
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

Allowed PythonShell to work with unset std streams #190 #191

Merged
merged 1 commit into from Aug 2, 2019
Merged

Allowed PythonShell to work with unset std streams #190 #191

merged 1 commit into from Aug 2, 2019

Conversation

joaoe
Copy link
Contributor

@joaoe joaoe commented Aug 1, 2019

The std streams might not be available when using the stdio option.

The std streams might not be available when using the stdio option.

should(pyshell.stdin).be.eql(null);
should(pyshell.stdout).be.eql(null);
should(pyshell.stderr).be.eql(null);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is stdout and stderr null? Because these both inherit the parent's stdout / stderr shouldn't they have a value?

Copy link
Contributor Author

@joaoe joaoe Aug 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They should be null. The underlying code does a file descriptor dup() so the child process just inherits the file descriptors directly and then all output goes to /dev/tty (for instance, if you are working in a terminal) bypassing any streams in memory in the local process (in the case of using inherit).

// 3 different ways of assigning values to the std streams in child_process.spawn()
// * ignore - pipe to /dev/null
// * inherit - inherit fd from parent process;
// * process.stderr - pass output directly to that stream.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Almenon
Copy link
Collaborator

Almenon commented Aug 2, 2019

@extrabacon can you configure the appveyor webhook in settings please? This PR should have triggered a build there :/

In the meantime I'll merge this PR in and see if it passes tests on the main branch

@Almenon Almenon merged commit 5f3b80a into extrabacon:master Aug 2, 2019
@Almenon Almenon added this to the V1.0.8 milestone Aug 2, 2019
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.

None yet

2 participants