Use PATH from env argument to pexpect.spawn

Currently pexpect.spawn takes an env argument along the lines of
subprocess.Popen but fails to use PATH from that env when finding
the binary to run.

The result when running a binary not on the PATH of the parent
process is pexpect will raise an exception, or worse, find a
different binary with the same name.

This change passes the env from spawn into the which function
so the correct PATH is searched and adds test coverage.

5 files changed, 116 insertions, 12 deletions

diff --git a/pexpect/pty_spawn.py b/pexpect/pty_spawn.py
index 3518cc1..e5b5814 100644
--- a/pexpect/pty_spawn.py
+++ b/pexpect/pty_spawn.py
@@ -266,7 +266,7 @@ class spawn(SpawnBase):
             self.args.insert(0, command)
             self.command = command
 
-        command_with_path = which(self.command)
+        command_with_path = which(self.command, env=self.env)
         if command_with_path is None:
             raise ExceptionPexpect('The command was not found or was not ' +
                     'executable: %s.' % self.command)
diff --git a/pexpect/utils.py b/pexpect/utils.py
index 737f0ed..c2763c4 100644
--- a/pexpect/utils.py
+++ b/pexpect/utils.py
@@ -31,7 +31,7 @@ def is_executable_file(path):
     return os.access(fpath, os.X_OK)
 
 
-def which(filename):
+def which(filename, env=None):
     '''This takes a given filename; tries to find it in the environment path;
     then checks if it is executable. This returns the full path to the filename
     if found and executable. Otherwise this returns None.'''
@@ -39,10 +39,11 @@ def which(filename):
     # Special case where filename contains an explicit path.
     if os.path.dirname(filename) != '' and is_executable_file(filename):
         return filename
-    if 'PATH' not in os.environ or os.environ['PATH'] == '':
+    if env is None:
+        env = os.environ
+    p = env.get('PATH')
+    if not p:
         p = os.defpath
-    else:
-        p = os.environ['PATH']
     pathlist = p.split(os.pathsep)
     for path in pathlist:
         ff = os.path.join(path, filename)
diff --git a/tests/test_env.py b/tests/test_env.py
new file mode 100755
index 0000000..ecaaa4b
--- /dev/null
+++ b/tests/test_env.py
@@ -0,0 +1,93 @@
+#!/usr/bin/env python
+'''
+PEXPECT LICENSE
+
+    This license is approved by the OSI and FSF as GPL-compatible.
+        http://opensource.org/licenses/isc-license.txt
+
+    Copyright (c) 2016, Martin Packman <martin.packman@canonical.com>
+    PERMISSION TO USE, COPY, MODIFY, AND/OR DISTRIBUTE THIS SOFTWARE FOR ANY
+    PURPOSE WITH OR WITHOUT FEE IS HEREBY GRANTED, PROVIDED THAT THE ABOVE
+    COPYRIGHT NOTICE AND THIS PERMISSION NOTICE APPEAR IN ALL COPIES.
+    THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+    WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+    MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
+    ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+    WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
+    ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
+    OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+
+'''
+import contextlib
+import os
+import tempfile
+import unittest
+
+import pexpect
+from . import PexpectTestCase
+
+
+@contextlib.contextmanager
+def example_script(name, output='success'):
+    " helper to create a temporary shell script that tests can run "
+    tempdir = tempfile.mkdtemp(prefix='tmp-pexpect-test')
+    try:
+        script_path = os.path.join(tempdir, name)
+        with open(script_path, 'w') as f:
+            f.write('#!/bin/sh\necho "%s"' % (output,))
+        try:
+            os.chmod(script_path, 0o755)
+            yield tempdir
+        finally:
+            os.remove(script_path)
+    finally:
+        os.rmdir(tempdir)
+
+
+class TestCaseEnv(PexpectTestCase.PexpectTestCase):
+    " tests for the env argument to pexpect.spawn and pexpect.run "
+
+    def test_run_uses_env(self):
+        " pexpect.run uses env argument when running child process "
+        script_name = 'run_uses_env.sh'
+        environ = {'PEXPECT_TEST_KEY': 'pexpect test value'}
+        with example_script(script_name, '$PEXPECT_TEST_KEY') as script_dir:
+            script = os.path.join(script_dir, script_name)
+            out = pexpect.run(script, env=environ)
+        self.assertEqual(out.rstrip(), b'pexpect test value')
+
+    def test_spawn_uses_env(self):
+        " pexpect.spawn uses env argument when running child process "
+        script_name = 'spawn_uses_env.sh'
+        environ = {'PEXPECT_TEST_KEY': 'pexpect test value'}
+        with example_script(script_name, '$PEXPECT_TEST_KEY') as script_dir:
+            script = os.path.join(script_dir, script_name)
+            child = pexpect.spawn(script, env=environ)
+            out = child.readline()
+            child.expect(pexpect.EOF)
+        self.assertEqual(child.exitstatus, 0)
+        self.assertEqual(out.rstrip(), b'pexpect test value')
+
+    def test_run_uses_env_path(self):
+        " pexpect.run uses binary from PATH when given in env argument "
+        script_name = 'run_uses_env_path.sh'
+        with example_script(script_name) as script_dir:
+            out = pexpect.run(script_name, env={'PATH': script_dir})
+        self.assertEqual(out.rstrip(), b'success')
+
+    def test_run_uses_env_path_over_path(self):
+        " pexpect.run uses PATH from env over os.environ "
+        script_name = 'run_uses_env_path_over_path.sh'
+        with example_script(script_name, output='failure') as wrong_dir:
+            with example_script(script_name) as right_dir:
+                orig_path = os.environ['PATH']
+                os.environ['PATH'] = wrong_dir
+                try:
+                    out = pexpect.run(script_name, env={'PATH': right_dir})
+                finally:
+                    os.environ['PATH'] = orig_path
+        self.assertEqual(out.rstrip(), b'success')
+
+
+if __name__ == '__main__':
+    unittest.main()
diff --git a/tests/test_misc.py b/tests/test_misc.py
index 16bdfc2..1fb558c 100755
--- a/tests/test_misc.py
+++ b/tests/test_misc.py
@@ -247,13 +247,6 @@ class TestCaseMisc(PexpectTestCase.PexpectTestCase):
         with self.assertRaises(TypeError):
             child.expect({})
 
-    def test_env(self):
-        " check keyword argument `env=' of pexpect.run() "
-        default_env_output = pexpect.run('env')
-        custom_env_output = pexpect.run('env', env={'_key': '_value'})
-        assert custom_env_output != default_env_output
-        assert b'_key=_value' in custom_env_output
-
     def test_cwd(self):
         " check keyword argument `cwd=' of pexpect.run() "
         tmp_dir = os.path.realpath(tempfile.gettempdir())
diff --git a/tests/test_which.py b/tests/test_which.py
index f909214..15a8944 100644
--- a/tests/test_which.py
+++ b/tests/test_which.py
@@ -20,6 +20,23 @@ class TestCaseWhich(PexpectTestCase.PexpectTestCase):
         assert exercise is not None
         assert exercise.startswith('/')
 
+    def test_path_from_env(self):
+        " executable found from optional env argument "
+        bin_name = 'pexpect-test-path-from-env'
+        tempdir = tempfile.mkdtemp()
+        try:
+            bin_path = os.path.join(tempdir, bin_name)
+            with open(bin_path, 'w') as f:
+                f.write('# test file not to be run')
+            try:
+                os.chmod(bin_path, 0o700)
+                found_path = pexpect.which(bin_name, env={'PATH': tempdir})
+            finally:
+                os.remove(bin_path)
+            self.assertEqual(bin_path, found_path)
+        finally:
+            os.rmdir(tempdir)
+
     def test_os_defpath_which(self):
         " which() finds an executable in $PATH and returns its abspath. "