From 10848c2e9a7cd3771c469dfd5ddb235f76b229ba Mon Sep 17 00:00:00 2001 From: Todd Gamblin Date: Thu, 21 Jan 2016 02:40:38 -0800 Subject: 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=`, 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.. --- lib/spack/spack/cmd/bootstrap.py | 2 +- lib/spack/spack/cmd/create.py | 2 +- lib/spack/spack/cmd/pkg.py | 2 +- lib/spack/spack/compiler.py | 3 +- lib/spack/spack/fetch_strategy.py | 6 +-- lib/spack/spack/test/cc.py | 6 +-- lib/spack/spack/test/make_executable.py | 48 ++++++++--------- lib/spack/spack/test/mock_repo.py | 4 +- lib/spack/spack/test/svn_fetch.py | 2 +- lib/spack/spack/util/executable.py | 95 ++++++++++++++++++++++++++------- 10 files changed, 113 insertions(+), 57 deletions(-) (limited to 'lib') 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): -- cgit v1.2.3-60-g2f50