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

Add exec subcommand to use PIPE and REDIRECTION ? #315

Closed
nonylene opened this issue Apr 14, 2017 · 7 comments
Closed

Add exec subcommand to use PIPE and REDIRECTION ? #315

nonylene opened this issue Apr 14, 2017 · 7 comments

Comments

@nonylene
Copy link
Contributor

nonylene commented Apr 14, 2017

PIPE and REDIRECTION are not working with pipenv run, because this uses pexpect.spawn
pexpect.spawn.wait with non-interactive environment.

$ pipenv run python3 -c "print('foo')" | grep foo
# => not found
$ pipenv run python3 -c "print('foo')" > bar.txt
# => bar.txt is empty

If use os.exec instead of spawn, pipe and redirection are available.

diff --git a/pipenv/cli.py b/pipenv/cli.py
index 62a4165..f48b9e7 100644
--- a/pipenv/cli.py
+++ b/pipenv/cli.py
@@ -997,7 +997,8 @@ def run(command, args, no_interactive=False, three=None, python=False):

     # Spawn the new process, and interact with it.
     try:
-        c = pexpect.spawn(which(command), list(args))
+        os.execv(which(command), [command] + list(args))
+        # c = pexpect.spawn(which(command), list(args))
     except pexpect.exceptions.ExceptionPexpect:
         click.echo(crayons.red('The command ({0}) was not found within the virtualenv!'.format(which(command))))
         sys.exit(1)
$ python3 -m pipenv run python3 -c "print('foo')" | grep foo
foo
$ python3 -m pipenv run python3 -c "print('foo')" > bar.txt
$ cat bar.txt
foo

Can you add exec subcommand like bundler, or change run command to use os.exec ?

version

3.5.6, or current master (7b17058)

@jamieconnolly
Copy link
Contributor

I've run into this issue too, whilst trying to test against the Python version inside a shell script.

Looking through the Pexpect FAQs, it seems the issue can be addressed whilst still calling pexpect.spawn.

Q: Why don’t shell pipe and redirect (| and >) work when I spawn a command?
A: Remember that Pexpect does NOT interpret shell meta characters such as redirect, pipe, or wild cards (>, |, or *). That’s done by a shell not the command you are spawning. This is a common mistake. If you want to run a command and pipe it through another command then you must also start a shell. For example:

child = pexpect.spawn('/bin/bash -c "ls -l | grep LOG > log_list.txt"')
child.expect(pexpect.EOF)

The second form of spawn (where you pass a list of arguments) is useful in situations where you wish to spawn a command and pass it its own argument list. This can make syntax more clear. For example, the following is equivalent to the previous example:

shell_cmd = 'ls -l | grep LOG > log_list.txt'
child = pexpect.spawn('/bin/bash', ['-c', shell_cmd])
child.expect(pexpect.EOF)

I hope this helps 👍

@nonylene
Copy link
Contributor Author

After further investigate, I found that this is caused by using pexpect.spawn.wait() with non-interactive environment.

pexpect.spawn.wait() does not read any data from child, so parent process cannot read output.

This waits until the child exits. This is a blocking call. This will not read any data from the child, so this will block forever if the child has unread output and has terminated. In other words, the child may have printed output then called exit(), but, the child is technically still alive until its output is read by the parent.

This method is non-blocking if wait() has already been called previously or isalive() method returns False. It simply returns the previously determined exit status.

http://pexpect.readthedocs.io/en/stable/api/pexpect.html#pexpect.spawn.wait

Workaround with pexpect (only stdout)

Pexpect can output result to stdout by setting logfile_read. Pipe and redirection (only stdout) works with below code.

diff --git a/pipenv/cli.py b/pipenv/cli.py
index 62a4165..e0af16c 100644
--- a/pipenv/cli.py
+++ b/pipenv/cli.py
@@ -997,7 +997,7 @@ def run(command, args, no_interactive=False, three=None, python=False):

     # Spawn the new process, and interact with it.
     try:
-        c = pexpect.spawn(which(command), list(args))
+        # use unicode mode ( https://github.com/pexpect/pexpect/pull/182 )
+        c = pexpect.spawnu(which(command), list(args))
     except pexpect.exceptions.ExceptionPexpect:
         click.echo(crayons.red('The command ({0}) was not found within the virtualenv!'.format(which(command))))
         sys.exit(1)
@@ -1011,7 +1011,9 @@ def run(command, args, no_interactive=False, three=None, python=False):

     # Interact with the new shell.
     if no_interactive:
-        c.wait()
+        c.logfile_read = sys.stdout
+        c.expect(pexpect.EOF)
     else:
         c.interact()
         c.close()

However, pipe or redirect with stdin or stderr still not available.

$ echo "foo" | python3 -m pipenv run python3 -c "import sys; print(sys.stdin.readline())" | cat
# => no output
$ python3 -m pipenv run python3 -c "import sys; sys.stderr.write('foo')" 2> bar.txt
a
# => bar.txt is empty

All problems are solved by using os.exec. Pexpect is not required if no expects are used.

I think pipenv run should use os.exec instead of pexpect.

@nateprewitt
Copy link
Sponsor Member

Hey everyone, sorry this hasn't been addressed yet. We're trying to work through tickets as time permits. I don't see a particular reason that pipenv run needs to be a subprocess, and we have a whole list of tickets that pertain to problems because of this model. I'd like to see some formal verification that this approach doesn't break anything the subprocess currently supports though.

Using os.exec seems like it will help the honcho/coverage issues we've had in the past too. I'll need to investigate that further (or anyone with some spare time could verify that pipenv run honcho/pipenv run coverage [params] works with the os.exec model.)

@kennethreitz, do you have any particular reasoning that we need to be spawning off subprocesses for pipenv run?

@kennethreitz
Copy link
Contributor

Nope!

@kennethreitz
Copy link
Contributor

Just seemed like the best solution to me.

@kennethreitz
Copy link
Contributor

@nonylene can you send a pull request?

@nonylene
Copy link
Contributor Author

@kennethreitz OK, I will fix this !

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

No branches or pull requests

4 participants