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