Merge pull request #383 from LLNL/features/better-executable-args

Refactor args for Executable.__call__

11 files changed, 116 insertions, 60 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):
diff --git a/var/spack/repos/builtin/packages/gcc/package.py b/var/spack/repos/builtin/packages/gcc/package.py
index 61b16f3fd8..3e5895cfb8 100644
--- a/var/spack/repos/builtin/packages/gcc/package.py
+++ b/var/spack/repos/builtin/packages/gcc/package.py
@@ -50,7 +50,7 @@ class Gcc(Package):
     version('4.5.4', '27e459c2566b8209ab064570e1b378f7')
 
     variant('gold', default=True, description="Build the gold linker plugin for ld-based LTO")
-    
+
     depends_on("mpfr")
     depends_on("gmp")
     depends_on("mpc")     # when @4.5:
@@ -102,7 +102,7 @@ class Gcc(Package):
             configure(*options)
             make()
             make("install")
-            
+
         self.write_rpath_specs()
 
 
@@ -121,7 +121,7 @@ class Gcc(Package):
             return
 
         gcc = Executable(join_path(self.prefix.bin, 'gcc'))
-        lines = gcc('-dumpspecs', return_output=True).strip().split("\n")
+        lines = gcc('-dumpspecs', output=str).strip().split("\n")
         specs_file = join_path(self.spec_dir, 'specs')
         with closing(open(specs_file, 'w')) as out:
             for line in lines: