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

[Paolo Bonzini <pbonzini@redhat.com>]

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