Re: [PATCH 07/27] mkvenv: add diagnose() method for ensure() failures

[John Snow <jsnow@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4138 bytes --]

On Thu, May 11, 2023, 2:53 AM Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 5/11/23 05:54, John Snow wrote:
> > This is a routine that is designed to print some usable info for human
> > beings back out to the terminal if/when "mkvenv ensure" fails to locate
> > or install a package during configure time, such as meson or sphinx.
> >
> > Since we are requiring that "meson" and "sphinx" are installed to the
> > same Python environment as QEMU is configured to build with, this can
> > produce some surprising failures when things are mismatched. This method
> > is here to try and ease that sting by offering some actionable
> > diagnosis.
>
> I think this is a bit too verbose/scary, especially the "Ouch" for
> what was a totally normal occurrence before (no "--enable-docs" and sphinx
> absent or too old) and the "ERROR" from "pip install --no-index".
>
> Here is an attempt to tone it down:
>
> diff --git a/python/scripts/mkvenv.py b/python/scripts/mkvenv.py
> index 8e097e4759e3..5d30174a9aff 100644
> --- a/python/scripts/mkvenv.py
> +++ b/python/scripts/mkvenv.py
> @@ -74,6 +74,7 @@
>       Iterator,
>       Optional,
>       Sequence,
> +    Tuple,
>       Union,
>   )
>   import venv
> @@ -594,7 +595,7 @@ def diagnose(
>       online: bool,
>       wheels_dir: Optional[Union[str, Path]],
>       prog: Optional[str],
> -) -> str:
> +) -> Tuple[str, bool]:
>       """
>       Offer a summary to the user as to why a package failed to be
> installed.
>
> @@ -610,6 +611,9 @@ def diagnose(
>       """
>       # pylint: disable=too-many-branches
>
> +    # Some errors are not particularly serious
> +    bad = False
> +
>       pkg_name = pkgname_from_depspec(dep_spec)
>       pkg_version = None
>
> @@ -654,11 +658,11 @@ def diagnose(
>               "No suitable version found in, or failed to install from"
>               f" '{wheels_dir}'."
>           )
> -    else:
> -        lines.append("No local package directory was searched.")
> +        bad = True
>
>       if online:
>           lines.append("A suitable version could not be obtained from
> PyPI.")
> +        bad = True
>       else:
>           lines.append(
>               "mkvenv was configured to operate offline and did not check
> PyPI."
> @@ -675,10 +679,14 @@ def diagnose(
>                   f"Typically this means that '{prog}' has been installed "
>                   "against a different Python interpreter on your system."
>               )
> +            bad = True
>
>       lines = [f" • {line}" for line in lines]
> -    lines.insert(0, f"Could not ensure availability of '{dep_spec}':")
> -    return os.linesep.join(lines)
> +    if bad:
> +        lines.insert(0, f"Could not ensure availability of '{dep_spec}':")
> +    else:
> +        lines.insert(0, f"'{dep_spec}' not found:")
> +    return os.linesep.join(lines), bad
>
>
>   def pip_install(
> @@ -731,7 +739,7 @@ def _do_ensure(
>               dep_specs,
>               online=False,
>               wheels_dir=wheels_dir,
> -            devnull=online and not wheels_dir,
> +            devnull=not wheels_dir,
>           )
>           # (A) or (B) happened. Success.
>       except subprocess.CalledProcessError:
> @@ -778,7 +786,10 @@ def ensure(
>           _do_ensure(dep_specs, online, wheels_dir)
>       except subprocess.CalledProcessError as exc:
>           # Well, that's not good.
> -        raise Ouch(diagnose(dep_specs[0], online, wheels_dir, prog)) from
> exc
> +        msg, bad = diagnose(dep_specs[0], online, wheels_dir, prog)
> +        if bad:
> +            raise Ouch(msg) from exc
> +        print("", msg, "\n", sep="\n", file=sys.stderr)
>
>
>   def post_venv_setup() -> None:
>
>
> Paolo
>

You're right, in the "optional" case for sphinx the error isn't really
*that* bad or serious. I'll try to work this or something very similar to
it in.

I was thinking it could be up to the caller to discard the input, but I
suppose we can also route the semantics down into the tool, too.

I'll play with it.

--js

>

[-- Attachment #2: Type: text/html, Size: 5540 bytes --]