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

[Paolo Bonzini]

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