On Thu, May 11, 2023, 2:53 AM Paolo Bonzini <pbonz...@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
>
