On Thu, Mar 17, 2022 at 12:34 PM Hanna Reitz <hre...@redhat.com> wrote:
>
> On 17.03.22 17:31, John Snow wrote:
> > 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
>
> That would be better, although I’m not quite sure I’d immediately know
> what this means. Something like “Does self.stdout contain both stdout
> and stderr?” above it would clear my potential and/or assumed confusion,
> I believe.
Sure thing.
(Thanks!)
