On Thu, Mar 17, 2022 at 11:56 AM Hanna Reitz <hre...@redhat.com> wrote: > > On 17.03.22 16:13, John Snow wrote: > > On Thu, Mar 17, 2022 at 5:23 AM Hanna Reitz <hre...@redhat.com> wrote: > >> On 08.03.22 02:57, John Snow wrote: > >>> This adds an Exception that extends the Python stdlib > >>> subprocess.CalledProcessError. > >>> > >>> The difference is that the str() method of this exception also adds the > >>> stdout/stderr logs. In effect, if this exception goes unhandled, Python > >>> will print the output in a visually distinct wrapper to the terminal so > >>> that it's easy to spot in a sea of traceback information. > >>> > >>> Signed-off-by: John Snow <js...@redhat.com> > >>> Reviewed-by: Eric Blake <ebl...@redhat.com> > >>> --- > >>> python/qemu/utils/__init__.py | 36 +++++++++++++++++++++++++++++++++++ > >>> 1 file changed, 36 insertions(+) > >>> > >>> diff --git a/python/qemu/utils/__init__.py b/python/qemu/utils/__init__.py > >>> index 5babf40df2..355ac550bc 100644 > >>> --- a/python/qemu/utils/__init__.py > >>> +++ b/python/qemu/utils/__init__.py > >>> @@ -18,6 +18,7 @@ > >>> import os > >>> import re > >>> import shutil > >>> +from subprocess import CalledProcessError > >>> import textwrap > >>> from typing import Optional > >>> > >>> @@ -26,6 +27,7 @@ > >>> > >>> > >>> __all__ = ( > >>> + 'VerboseProcessError', > >>> 'add_visual_margin', > >>> 'get_info_usernet_hostfwd_port', > >>> 'kvm_available', > >>> @@ -121,3 +123,37 @@ def _wrap(line: str) -> str: > >>> os.linesep.join(_wrap(line) for line in content.splitlines()), > >>> _bar(None, top=False), > >>> )) > >>> + > >>> + > >>> +class VerboseProcessError(CalledProcessError): > >>> + """ > >>> + The same as CalledProcessError, but more verbose. > >>> + > >>> + This is useful for debugging failed calls during test executions. > >>> + The return code, signal (if any), and terminal output will be > >>> displayed > >>> + on unhandled exceptions. > >>> + """ > >>> + def summary(self) -> str: > >>> + """Return the normal CalledProcessError str() output.""" > >>> + return super().__str__() > >>> + > >>> + def __str__(self) -> str: > >>> + lmargin = ' ' > >>> + width = -len(lmargin) > >>> + sections = [] > >>> + > >>> + name = 'output' if self.stderr is None else 'stdout' > >>> + if self.stdout: > >>> + sections.append(add_visual_margin(self.stdout, width, name)) > >>> + else: > >>> + sections.append(f"{name}: N/A") > >>> + > >>> + if self.stderr: > >>> + sections.append(add_visual_margin(self.stderr, width, > >>> 'stderr')) > >>> + elif self.stderr is not None: > >> What exactly is this condition for? I would’ve understood if it was > >> `self.stdout` (because the stdout section then is called just “output”, > >> so it would make sense to omit the stderr block), but this way it looks > >> like we’ll only go here if `self.stderr` is an empty string (which > >> doesn’t make much sense to me, and I don’t understand why we wouldn’t > >> have the same in the `self.stdout` part above). > >> > >> (tl;dr: should this be `self.stdout`?) > >> > >> Hanna > >> > > if self.stderr is None, it means that the IO streams were combined. If > > it is merely empty, it means there wasn't any stderr output. > > Might warrant a comment? :)
How 'bout: has_combined_output = self.stderr is None > > > so: > > > > if self.stderr: There's content here, so put it in a lil' box > > else: could be either None or the empty string. If it's None, we > > didn't *have* a stderr, so don't print anything at all, let the > > "output" section above handle it. If we did have stderr and it just > > happened to be empty, write N/A. > > > > I wanted that "N/A" to provide active feedback to show the human > > operator that we're not just failing to show them what the stderr > > output was: there genuinely wasn't any. > > Thanks, that makes sense. > > Reviewed-by: Hanna Reitz <hre...@redhat.com> >