Refactor args for Executable.__call__

- simplify output, error, and input redirection
- `return_output=True` is now `output=str`
- `return_output=True` will still work for the time being but is
  deprecated.

- previously you could say `return_output=True` and `output=<stream>`,
  which wouldn't actually write to the stream.  Now you actually can't
  specify erroneous cases since there is only one parameter with
  mutually exclusive options..

10 files changed, 113 insertions, 57 deletions

diff --git a/lib/spack/spack/cmd/bootstrap.py b/lib/spack/spack/cmd/bootstrap.py
index e4ec7da35d..bdbd623b39 100644
--- a/lib/spack/spack/cmd/bootstrap.py
+++ b/lib/spack/spack/cmd/bootstrap.py
@@ -42,7 +42,7 @@ def get_origin_url():
     git = which('git', required=True)
     origin_url = git(
         '--git-dir=%s' % git_dir, 'config', '--get', 'remote.origin.url',
-        return_output=True)
+        output=str)
     return origin_url.strip()
 
 
diff --git a/lib/spack/spack/cmd/create.py b/lib/spack/spack/cmd/create.py
index 7cea39cb55..edcea0718c 100644
--- a/lib/spack/spack/cmd/create.py
+++ b/lib/spack/spack/cmd/create.py
@@ -132,7 +132,7 @@ class ConfigureGuesser(object):
         # Peek inside the tarball.
         tar = which('tar')
         output = tar(
-            "--exclude=*/*/*", "-tf", stage.archive_file, return_output=True)
+            "--exclude=*/*/*", "-tf", stage.archive_file, output=str)
         lines = output.split("\n")
 
         # Set the configure line to the one that matched.
diff --git a/lib/spack/spack/cmd/pkg.py b/lib/spack/spack/cmd/pkg.py
index 448f762841..cf478d3763 100644
--- a/lib/spack/spack/cmd/pkg.py
+++ b/lib/spack/spack/cmd/pkg.py
@@ -79,7 +79,7 @@ def list_packages(rev):
     git = get_git()
     relpath = spack.packages_path[len(spack.prefix + os.path.sep):] + os.path.sep
     output = git('ls-tree', '--full-tree', '--name-only', rev, relpath,
-                 return_output=True)
+                 output=str)
     return sorted(line[len(relpath):] for line in output.split('\n') if line)
 
 
diff --git a/lib/spack/spack/compiler.py b/lib/spack/spack/compiler.py
index 887e416dc5..12c02e0ea2 100644
--- a/lib/spack/spack/compiler.py
+++ b/lib/spack/spack/compiler.py
@@ -24,7 +24,6 @@
 ##############################################################################
 import os
 import re
-import subprocess
 import itertools
 from datetime import datetime
 
@@ -52,7 +51,7 @@ _version_cache = {}
 def get_compiler_version(compiler_path, version_arg, regex='(.*)'):
     if not compiler_path in _version_cache:
         compiler = Executable(compiler_path)
-        output = compiler(version_arg, return_output=True, error=subprocess.STDOUT)
+        output = compiler(version_arg, output=str, error=str)
 
         match = re.search(regex, output)
         _version_cache[compiler_path] = match.group(1) if match else 'unknown'
diff --git a/lib/spack/spack/fetch_strategy.py b/lib/spack/spack/fetch_strategy.py
index 0657146bf6..337dd1e198 100644
--- a/lib/spack/spack/fetch_strategy.py
+++ b/lib/spack/spack/fetch_strategy.py
@@ -154,7 +154,7 @@ class URLFetchStrategy(FetchStrategy):
 
         # Run curl but grab the mime type from the http headers
         headers = spack.curl(
-            *curl_args, return_output=True, fail_on_error=False)
+            *curl_args, output=str, fail_on_error=False)
 
         if spack.curl.returncode != 0:
             # clean up archive on failure.
@@ -375,7 +375,7 @@ class GitFetchStrategy(VCSFetchStrategy):
 
     @property
     def git_version(self):
-        vstring = self.git('--version', return_output=True).lstrip('git version ')
+        vstring = self.git('--version', output=str).lstrip('git version ')
         return Version(vstring)
 
 
@@ -518,7 +518,7 @@ class SvnFetchStrategy(VCSFetchStrategy):
 
     def _remove_untracked_files(self):
         """Removes untracked files in an svn repository."""
-        status = self.svn('status', '--no-ignore', return_output=True)
+        status = self.svn('status', '--no-ignore', output=str)
         self.svn('status', '--no-ignore')
         for line in status.split('\n'):
             if not re.match('^[I?]', line):
diff --git a/lib/spack/spack/test/cc.py b/lib/spack/spack/test/cc.py
index 4188b8d550..905af28a06 100644
--- a/lib/spack/spack/test/cc.py
+++ b/lib/spack/spack/test/cc.py
@@ -65,17 +65,17 @@ class CompilerTest(unittest.TestCase):
 
     def check_cc(self, command, args, expected):
         os.environ['SPACK_TEST_COMMAND'] = command
-        self.assertEqual(self.cc(*args, return_output=True).strip(), expected)
+        self.assertEqual(self.cc(*args, output=str).strip(), expected)
 
 
     def check_ld(self, command, args, expected):
         os.environ['SPACK_TEST_COMMAND'] = command
-        self.assertEqual(self.ld(*args, return_output=True).strip(), expected)
+        self.assertEqual(self.ld(*args, output=str).strip(), expected)
 
 
     def check_cpp(self, command, args, expected):
         os.environ['SPACK_TEST_COMMAND'] = command
-        self.assertEqual(self.cpp(*args, return_output=True).strip(), expected)
+        self.assertEqual(self.cpp(*args, output=str).strip(), expected)
 
 
     def test_vcheck_mode(self):
diff --git a/lib/spack/spack/test/make_executable.py b/lib/spack/spack/test/make_executable.py
index 09efec8580..d568a28d44 100644
--- a/lib/spack/spack/test/make_executable.py
+++ b/lib/spack/spack/test/make_executable.py
@@ -56,47 +56,47 @@ class MakeExecutableTest(unittest.TestCase):
 
     def test_make_normal(self):
         make = MakeExecutable('make', 8)
-        self.assertEqual(make(return_output=True).strip(), '-j8')
-        self.assertEqual(make('install', return_output=True).strip(), '-j8 install')
+        self.assertEqual(make(output=str).strip(), '-j8')
+        self.assertEqual(make('install', output=str).strip(), '-j8 install')
 
 
     def test_make_explicit(self):
         make = MakeExecutable('make', 8)
-        self.assertEqual(make(parallel=True, return_output=True).strip(), '-j8')
-        self.assertEqual(make('install', parallel=True, return_output=True).strip(), '-j8 install')
+        self.assertEqual(make(parallel=True, output=str).strip(), '-j8')
+        self.assertEqual(make('install', parallel=True, output=str).strip(), '-j8 install')
 
 
     def test_make_one_job(self):
         make = MakeExecutable('make', 1)
-        self.assertEqual(make(return_output=True).strip(), '')
-        self.assertEqual(make('install', return_output=True).strip(), 'install')
+        self.assertEqual(make(output=str).strip(), '')
+        self.assertEqual(make('install', output=str).strip(), 'install')
 
 
     def test_make_parallel_false(self):
         make = MakeExecutable('make', 8)
-        self.assertEqual(make(parallel=False, return_output=True).strip(), '')
-        self.assertEqual(make('install', parallel=False, return_output=True).strip(), 'install')
+        self.assertEqual(make(parallel=False, output=str).strip(), '')
+        self.assertEqual(make('install', parallel=False, output=str).strip(), 'install')
 
 
     def test_make_parallel_disabled(self):
         make = MakeExecutable('make', 8)
 
         os.environ['SPACK_NO_PARALLEL_MAKE'] = 'true'
-        self.assertEqual(make(return_output=True).strip(), '')
-        self.assertEqual(make('install', return_output=True).strip(), 'install')
+        self.assertEqual(make(output=str).strip(), '')
+        self.assertEqual(make('install', output=str).strip(), 'install')
 
         os.environ['SPACK_NO_PARALLEL_MAKE'] = '1'
-        self.assertEqual(make(return_output=True).strip(), '')
-        self.assertEqual(make('install', return_output=True).strip(), 'install')
+        self.assertEqual(make(output=str).strip(), '')
+        self.assertEqual(make('install', output=str).strip(), 'install')
 
         # These don't disable (false and random string)
         os.environ['SPACK_NO_PARALLEL_MAKE'] = 'false'
-        self.assertEqual(make(return_output=True).strip(), '-j8')
-        self.assertEqual(make('install', return_output=True).strip(), '-j8 install')
+        self.assertEqual(make(output=str).strip(), '-j8')
+        self.assertEqual(make('install', output=str).strip(), '-j8 install')
 
         os.environ['SPACK_NO_PARALLEL_MAKE'] = 'foobar'
-        self.assertEqual(make(return_output=True).strip(), '-j8')
-        self.assertEqual(make('install', return_output=True).strip(), '-j8 install')
+        self.assertEqual(make(output=str).strip(), '-j8')
+        self.assertEqual(make('install', output=str).strip(), '-j8 install')
 
         del os.environ['SPACK_NO_PARALLEL_MAKE']
 
@@ -106,20 +106,20 @@ class MakeExecutableTest(unittest.TestCase):
 
         # These should work
         os.environ['SPACK_NO_PARALLEL_MAKE'] = 'true'
-        self.assertEqual(make(parallel=True, return_output=True).strip(), '')
-        self.assertEqual(make('install', parallel=True, return_output=True).strip(), 'install')
+        self.assertEqual(make(parallel=True, output=str).strip(), '')
+        self.assertEqual(make('install', parallel=True, output=str).strip(), 'install')
 
         os.environ['SPACK_NO_PARALLEL_MAKE'] = '1'
-        self.assertEqual(make(parallel=True, return_output=True).strip(), '')
-        self.assertEqual(make('install', parallel=True, return_output=True).strip(), 'install')
+        self.assertEqual(make(parallel=True, output=str).strip(), '')
+        self.assertEqual(make('install', parallel=True, output=str).strip(), 'install')
 
         # These don't disable (false and random string)
         os.environ['SPACK_NO_PARALLEL_MAKE'] = 'false'
-        self.assertEqual(make(parallel=True, return_output=True).strip(), '-j8')
-        self.assertEqual(make('install', parallel=True, return_output=True).strip(), '-j8 install')
+        self.assertEqual(make(parallel=True, output=str).strip(), '-j8')
+        self.assertEqual(make('install', parallel=True, output=str).strip(), '-j8 install')
 
         os.environ['SPACK_NO_PARALLEL_MAKE'] = 'foobar'
-        self.assertEqual(make(parallel=True, return_output=True).strip(), '-j8')
-        self.assertEqual(make('install', parallel=True, return_output=True).strip(), '-j8 install')
+        self.assertEqual(make(parallel=True, output=str).strip(), '-j8')
+        self.assertEqual(make('install', parallel=True, output=str).strip(), '-j8 install')
 
         del os.environ['SPACK_NO_PARALLEL_MAKE']
diff --git a/lib/spack/spack/test/mock_repo.py b/lib/spack/spack/test/mock_repo.py
index c454b1f106..9738ba4e72 100644
--- a/lib/spack/spack/test/mock_repo.py
+++ b/lib/spack/spack/test/mock_repo.py
@@ -141,7 +141,7 @@ class MockGitRepo(MockVCSRepo):
             self.url = self.path
 
     def rev_hash(self, rev):
-        return git('rev-parse', rev, return_output=True).strip()
+        return git('rev-parse', rev, output=str).strip()
 
 
 class MockSvnRepo(MockVCSRepo):
@@ -193,4 +193,4 @@ class MockHgRepo(MockVCSRepo):
 
     def get_rev(self):
         """Get current mercurial revision."""
-        return hg('id', '-i', return_output=True).strip()
+        return hg('id', '-i', output=str).strip()
diff --git a/lib/spack/spack/test/svn_fetch.py b/lib/spack/spack/test/svn_fetch.py
index 2ee4748fdb..7d150b42f4 100644
--- a/lib/spack/spack/test/svn_fetch.py
+++ b/lib/spack/spack/test/svn_fetch.py
@@ -65,7 +65,7 @@ class SvnFetchTest(MockPackagesTest):
     def assert_rev(self, rev):
         """Check that the current revision is equal to the supplied rev."""
         def get_rev():
-            output = svn('info', return_output=True)
+            output = svn('info', output=str)
             self.assertTrue("Revision" in output)
             for line in output.split('\n'):
                 match = re.match(r'Revision: (\d+)', line)
diff --git a/lib/spack/spack/util/executable.py b/lib/spack/spack/util/executable.py
index ba765eb662..fc27b789d0 100644
--- a/lib/spack/spack/util/executable.py
+++ b/lib/spack/spack/util/executable.py
@@ -55,24 +55,80 @@ class Executable(object):
 
 
     def __call__(self, *args, **kwargs):
-        """Run the executable with subprocess.check_output, return output."""
-        return_output = kwargs.get("return_output", False)
-        fail_on_error = kwargs.get("fail_on_error", True)
-        ignore_errors = kwargs.get("ignore_errors", ())
+        """Run this executable in a subprocess.
+
+        Arguments
+          args
+            command line arguments to the executable to run.
+
+        Optional arguments
+
+          fail_on_error
+
+            Raise an exception if the subprocess returns an
+            error. Default is True.  When not set, the return code is
+            avaiale as `exe.returncode`.
+
+          ignore_errors
+
+            An optional list/tuple of error codes that can be
+            *ignored*.  i.e., if these codes are returned, this will
+            not raise an exception when `fail_on_error` is `True`.
+
+          output, error
+
+            These arguments allow you to specify new stdout and stderr
+            values.  They default to `None`, which means the
+            subprocess will inherit the parent's file descriptors.
+
+            You can set these to:
+            - python streams, e.g. open Python file objects, or os.devnull;
+            - filenames, which will be automatically opened for writing; or
+            - `str`, as in the Python string type. If you set these to `str`,
+               output and error will be written to pipes and returned as
+               a string.  If both `output` and `error` are set to `str`,
+               then one string is returned containing output concatenated
+               with error.
+
+          input
+
+            Same as output, error, but `str` is not an allowed value.
+
+        Deprecated arguments
+
+          return_output[=False]
+
+            Setting this to True is the same as setting output=str.
+            This argument may be removed in future Spack versions.
+
+        """
+        fail_on_error = kwargs.pop("fail_on_error", True)
+        ignore_errors = kwargs.pop("ignore_errors", ())
+
+        # TODO: This is deprecated.  Remove in a future version.
+        return_output = kwargs.pop("return_output", False)
 
         # Default values of None says to keep parent's file descriptors.
-        output        = kwargs.get("output", None)
-        error         = kwargs.get("error", None)
-        input         = kwargs.get("input", None)
+        if return_output:
+            output = str
+        else:
+            output = kwargs.pop("output", None)
+
+        error         = kwargs.pop("error", None)
+        input         = kwargs.pop("input", None)
+        if input is str:
+            raise ValueError("Cannot use `str` as input stream.")
 
         def streamify(arg, mode):
             if isinstance(arg, basestring):
                 return open(arg, mode), True
+            elif arg is str:
+                return subprocess.PIPE, False
             else:
                 return arg, False
-        output, ostream = streamify(output, 'w')
-        error,  estream = streamify(error,  'w')
-        input,  istream = streamify(input,  'r')
+        ostream, close_ostream = streamify(output, 'w')
+        estream, close_estream = streamify(error,  'w')
+        istream, close_istream = streamify(input,  'r')
 
         # if they just want to ignore one error code, make it a tuple.
         if isinstance(ignore_errors, int):
@@ -92,19 +148,20 @@ class Executable(object):
         tty.debug(cmd_line)
 
         try:
-            if return_output:
-                output = subprocess.PIPE
-
             proc = subprocess.Popen(
-                cmd, stdin=input, stderr=error, stdout=output)
+                cmd, stdin=istream, stderr=estream, stdout=ostream)
             out, err = proc.communicate()
 
             rc = self.returncode = proc.returncode
             if fail_on_error and rc != 0 and (rc not in ignore_errors):
                 raise ProcessError("Command exited with status %d:"
                                    % proc.returncode, cmd_line)
-            if return_output:
-                return out
+
+            if output is str or error is str:
+                result = ''
+                if output is str: result += out
+                if error is str:  result += err
+                return result
 
         except OSError, e:
             raise ProcessError(
@@ -119,9 +176,9 @@ class Executable(object):
                     % (proc.returncode, cmd_line))
 
         finally:
-            if ostream: output.close()
-            if estream: error.close()
-            if istream: input.close()
+            if close_ostream: output.close()
+            if close_estream: error.close()
+            if close_istream: input.close()
 
 
     def __eq__(self, other):