From 9430063ba76246d4493c41933a5dea3902eece82 Mon Sep 17 00:00:00 2001 From: Chenxiong Qi Date: Aug 07 2018 12:04:07 +0000 Subject: Allow _run_command to capture and return output to stdout or stderr Once process executes successfully, _run_command will return a tuple consisting of three elements of process exit code and output to stdout and stderr, which could be None if it does not intent to capture and return them. Original behavior is still available. As long as error is raised from subprocess or process exits non-zero and stderr output is not captured, rpkgError will be raised. Several examples: 1. run rpmbuild to build SRPM and output to stdout and stderr are not captured. _run_command(['rpmbuild', '-bs', 'pkg.spec']) 2. run rpm to get package metadata from SPEC. _run_command( ['rpm', '-q', '--qf', '%{name}', '--specfile', 'pkg.spec'], return_stdout=True, return_text=True) 3. run rpm but error is raised. _run_command(['rpm', '-q', '-qf', '--specfile', 'pkg.spec']) Signed-off-by: Chenxiong Qi --- diff --git a/pyrpkg/__init__.py b/pyrpkg/__init__.py index a5e716d..9adc6d1 100644 --- a/pyrpkg/__init__.py +++ b/pyrpkg/__init__.py @@ -1001,7 +1001,9 @@ class Commands(object): """ return cccolutils.has_creds() - def _run_command(self, cmd, shell=False, env=None, pipe=[], cwd=None): + def _run_command(self, cmd, shell=False, env=None, pipe=[], cwd=None, + return_stdout=False, return_stderr=False, + return_text=False): """Run the given command. `_run_command` is able to run single command or two commands via pipe. @@ -1022,9 +1024,27 @@ class Commands(object): `shell`. :type pipe: str or list :param str cwd: optional directory to run the command from + :param bool return_stdout: whether to capture output to stdout and + return it. Default is False. + :param bool return_stderr: whether to capture output to stderr and + return it. Default is False. + :param bool return_text: whether to return the stdout and stder output + as text instead of byte string. Default is False. + :return: a tuple containing three values of exit code from process, + output to stdout, output to stderr. Either output to stdout or + stderr could be None if `return_stdout` or `return_stderr` is not + set. + :rtype: tuple(int, str, str) :raises rpkgError: any error raised from underlying `subprocess` while executing command in local system will be mapped - to :py:exc:`rpkgError`. + to :py:exc:`rpkgError`. In addition, if process returns non-zero + and error message output to stderr does not intent to be returned, + the error is raised as well. + + .. versionchanged:: 1.56 + Added argument return_stdout, return_stderr and return_text. + Return process exit code, output to stdout and stderr if process + executes successfully. """ # Process any environment variables. @@ -1046,23 +1066,43 @@ class Commands(object): else: self.log.debug('Running: %s', ' '.join(cmd)) + proc_stdout = subprocess.PIPE if return_stdout else None + proc_stderr = subprocess.PIPE if return_stderr else None + try: if pipe: # We're piping the stderr over as well, which is probably a # bad thing, but rpmbuild likes to put useful data on # stderr, so.... - proc = subprocess.Popen(command, env=environ, shell=shell, cwd=cwd, - stdout=subprocess.PIPE, stderr=subprocess.STDOUT) - subprocess.check_call(pipecmd, env=environ, shell=shell, cwd=cwd, stdin=proc.stdout) + parent_proc = subprocess.Popen( + command, env=environ, shell=shell, cwd=cwd, + stdout=subprocess.PIPE, stderr=subprocess.STDOUT) + + proc = subprocess.Popen( + pipecmd, env=environ, shell=shell, cwd=cwd, + stdin=parent_proc.stdout, + stdout=proc_stdout, stderr=proc_stderr, + universal_newlines=return_text) else: - subprocess.check_call(command, env=environ, shell=shell, cwd=cwd) - except (subprocess.CalledProcessError, OSError) as e: - raise rpkgError(e) + proc = subprocess.Popen( + command, env=environ, shell=shell, cwd=cwd, + stdout=proc_stdout, stderr=proc_stderr, + universal_newlines=return_text) except KeyboardInterrupt: raise rpkgError('Command is terminated by user.') except Exception as e: raise rpkgError(e) + exit_code = proc.wait() + if exit_code > 0 and not return_stderr: + raise rpkgError('Failed to execute command.') + + return ( + exit_code, + proc.stdout.read() if return_stdout else None, + proc.stderr.read() if return_stderr else None, + ) + def _newer(self, file1, file2): """Compare the last modification time of the given files diff --git a/tests/test_commands.py b/tests/test_commands.py index c53a24f..0dc3ace 100644 --- a/tests/test_commands.py +++ b/tests/test_commands.py @@ -953,3 +953,143 @@ class TestLint(CommandTestCase): srpm_path, bin_path, ], shell=True)]) + + +class TestRunCommand(CommandTestCase): + """Test _run_command""" + + require_test_repos = False + + def setUp(self): + super(TestRunCommand, self).setUp() + self.cmd = self.make_commands(path='/path/to/repo') + + @patch('subprocess.Popen') + def test_run_command_within_shell(self, Popen): + Popen.return_value.wait.return_value = 0 + + result = self.cmd._run_command(['rpmbuild'], shell=True) + + self.assertEqual((0, None, None), result) + Popen.assert_called_once_with( + 'rpmbuild', env=os.environ, shell=True, cwd=None, + stdout=None, stderr=None, universal_newlines=False) + + @patch('subprocess.Popen') + def test_run_command_without_shell(self, Popen): + Popen.return_value.wait.return_value = 0 + + result = self.cmd._run_command(['rpmbuild']) + + self.assertEqual((0, None, None), result) + Popen.assert_called_once_with( + ['rpmbuild'], env=os.environ, shell=False, cwd=None, + stdout=None, stderr=None, universal_newlines=False) + + @patch('subprocess.Popen') + def test_return_stdout(self, Popen): + Popen.return_value.wait.return_value = 0 + Popen.return_value.stdout.read.return_value = 'output' + + result = self.cmd._run_command( + ['rpmbuild'], shell=True, return_stdout=True) + + self.assertEqual((0, 'output', None), result) + Popen.assert_called_once_with( + 'rpmbuild', env=os.environ, shell=True, cwd=None, + stdout=subprocess.PIPE, stderr=None, universal_newlines=False) + + @patch('subprocess.Popen') + def test_return_stderr(self, Popen): + Popen.return_value.wait.return_value = 0 + Popen.return_value.stderr.read.return_value = 'output' + + result = self.cmd._run_command( + ['rpmbuild'], shell=True, return_stderr=True) + + self.assertEqual((0, None, 'output'), result) + Popen.assert_called_once_with( + 'rpmbuild', env=os.environ, shell=True, cwd=None, + stdout=None, stderr=subprocess.PIPE, universal_newlines=False) + + @patch('subprocess.Popen') + def test_pipe(self, Popen): + Popen.return_value.wait.return_value = 0 + + first_proc = Mock() + second_proc = Mock() + second_proc.wait.return_value = 0 + + Popen.side_effect = [first_proc, second_proc] + + result = self.cmd._run_command( + ['rpmbuild'], pipe=['grep', 'src.rpm']) + + self.assertEqual((0, None, None), result) + Popen.assert_has_calls([ + call(['rpmbuild'], + env=os.environ, shell=False, cwd=None, + stdout=subprocess.PIPE, stderr=subprocess.STDOUT), + call(['grep', 'src.rpm'], + env=os.environ, shell=False, cwd=None, + stdin=first_proc.stdout, stdout=None, stderr=None, + universal_newlines=False), + ]) + + @patch('subprocess.Popen') + def test_raise_error_if_error_is_raised_from_subprocess(self, Popen): + Popen.side_effect = OSError(1, "msg") + + six.assertRaisesRegex( + self, rpkgError, 'msg', self.cmd._run_command, ['rpmbuild']) + + @patch('subprocess.Popen') + def test_raise_error_if_command_returns_nonzeror(self, Popen): + Popen.return_value.wait.return_value = 1 + Popen.return_value.stderr.read.return_value = '' + + six.assertRaisesRegex( + self, rpkgError, 'Failed to execute command', + self.cmd._run_command, ['rpmbuild']) + + @patch('subprocess.Popen') + def test_return_error_msg_if_return_stderr_is_set(self, Popen): + Popen.return_value.wait.return_value = 1 + Popen.return_value.stderr.read.return_value = 'something wrong' + + result = self.cmd._run_command(['rpmbuild'], return_stderr=True) + self.assertEqual((1, None, 'something wrong'), result) + + @patch('subprocess.Popen') + def test_set_envs(self, Popen): + Popen.return_value.wait.return_value = 0 + + with patch.dict('os.environ', {}, clear=True): + result = self.cmd._run_command(['rpmbuild'], env={'myvar': 'test'}) + + self.assertEqual((0, None, None), result) + Popen.assert_called_once_with( + ['rpmbuild'], env={'myvar': 'test'}, + shell=False, cwd=None, stdout=None, stderr=None, + universal_newlines=False) + + @patch('subprocess.Popen') + def test_run_command_in_a_directory(self, Popen): + Popen.return_value.wait.return_value = 0 + + self.cmd._run_command(['rpmbuild'], cwd='/tmp/builddir') + + Popen.assert_called_once_with( + ['rpmbuild'], env=os.environ, shell=False, cwd='/tmp/builddir', + stdout=None, stderr=None, universal_newlines=False) + + @patch('subprocess.Popen') + def test_return_output_as_text(self, Popen): + Popen.return_value.wait.return_value = 0 + + result = self.cmd._run_command( + ['rpmbuild'], return_stdout=True, return_text=True) + + Popen.assert_called_once_with( + ['rpmbuild'], env=os.environ, shell=False, cwd=None, + stdout=subprocess.PIPE, stderr=None, universal_newlines=True) diff --git a/tests/utils.py b/tests/utils.py index 90318c6..a428731 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -139,10 +139,10 @@ class Utils(object): f.write(content) -class CommandTestCase(Assertions, Utils, unittest.TestCase): +class RepoCreationMixin(object): + """Provide methods to create and destroy git repositories for tests""" - def setUp(self): - # create a base repo + def _create_test_repos(self): self.repo_path = tempfile.mkdtemp(prefix='rpkg-commands-tests-') self.spec_file = 'docpkg.spec' @@ -185,10 +185,23 @@ class CommandTestCase(Assertions, Utils, unittest.TestCase): self.run_cmd(cmd, cwd=self.cloned_repo_path, stdout=subprocess.PIPE, stderr=subprocess.PIPE) - def tearDown(self): + def _destroy_test_repos(self): shutil.rmtree(self.repo_path) shutil.rmtree(self.cloned_repo_path) + +class CommandTestCase(RepoCreationMixin, Assertions, Utils, unittest.TestCase): + + require_test_repos = True + + def setUp(self): + if self.require_test_repos: + self._create_test_repos() + + def tearDown(self): + if self.require_test_repos: + self._destroy_test_repos() + def make_commands(self, path=None, user=None, dist=None, target=None, quiet=None): """Helper method for creating Commands object for test cases