Re: [PATCH] mkvenv: always pass locally-installed packages to pip

[John Snow]

On Tue, Jun 6, 2023 at 4:23 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> Let pip decide whether a new version should be installed or the current
> one is okay.  This ensures that the virtual environment is updated
> (either upgraded or downgraded) whenever a new version of a package is
> requested.
>
> The hardest part here is figuring out if a package is installed in
> the venv (which also has to be done twice to account for the presence
> of either setuptools in Python <3.8, or importlib in Python >=3.8).
>
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Cc: John Snow <jsnow@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  python/scripts/mkvenv.py | 76 ++++++++++++++++++++++++++++++++++++++--

:(

The best laid plans of mice and men,

--js

>  1 file changed, 74 insertions(+), 2 deletions(-)
>
> diff --git a/python/scripts/mkvenv.py b/python/scripts/mkvenv.py
> index 3a9aef46a51..3957e7d6e08 100644
> --- a/python/scripts/mkvenv.py
> +++ b/python/scripts/mkvenv.py
> @@ -553,6 +553,74 @@ def pkgname_from_depspec(dep_spec: str) -> str:
>      return match.group(0)
>
>
> +def _get_path_importlib(package: str) -> Optional[str]:
> +    # pylint: disable=import-outside-toplevel
> +    # pylint: disable=no-name-in-module
> +    # pylint: disable=import-error
> +    try:
> +        # First preference: Python 3.8+ stdlib
> +        from importlib.metadata import (  # type: ignore
> +            PackageNotFoundError,
> +            distribution,
> +        )
> +    except ImportError as exc:
> +        logger.debug("%s", str(exc))
> +        # Second preference: Commonly available PyPI backport
> +        from importlib_metadata import (  # type: ignore
> +            PackageNotFoundError,
> +            distribution,
> +        )
> +
> +    try:
> +        return str(distribution(package).locate_file("."))
> +    except PackageNotFoundError:
> +        return None
> +
> +
> +def _get_path_pkg_resources(package: str) -> Optional[str]:
> +    # pylint: disable=import-outside-toplevel
> +    # Bundled with setuptools; has a good chance of being available.
> +    import pkg_resources
> +
> +    try:
> +        return str(pkg_resources.get_distribution(package).location)
> +    except pkg_resources.DistributionNotFound:
> +        return None
> +
> +
> +def _get_path(package: str) -> Optional[str]:
> +    try:
> +        return _get_path_importlib(package)
> +    except ImportError as exc:
> +        logger.debug("%s", str(exc))
> +
> +    try:
> +        return _get_path_pkg_resources(package)
> +    except ImportError as exc:
> +        logger.debug("%s", str(exc))
> +        raise Ouch(
> +            "Neither importlib.metadata nor pkg_resources found. "
> +            "Use Python 3.8+, or install importlib-metadata or setuptools."
> +        ) from exc
> +
> +
> +def _path_is_prefix(prefix: Union[str, Path], path: Union[str, Path]) -> 
> bool:
> +    prefix = str(prefix)
> +    path = str(path)
> +    try:
> +        return os.path.commonpath([prefix, path]) == prefix
> +    except ValueError:
> +        return False
> +
> +
> +def _is_system_package(package: str) -> bool:
> +    path = _get_path(package)
> +    return path is not None and not (
> +        _path_is_prefix(sysconfig.get_path("purelib"), path)
> +        or _path_is_prefix(sysconfig.get_path("platlib"), path)
> +    )
> +
> +
>  def _get_version_importlib(package: str) -> Optional[str]:
>      # pylint: disable=import-outside-toplevel
>      # pylint: disable=no-name-in-module
> @@ -741,8 +809,12 @@ def _do_ensure(
>      for spec in dep_specs:
>          matcher = distlib.version.LegacyMatcher(spec)
>          ver = _get_version(matcher.name)
> -        if ver is None or not matcher.match(
> -            distlib.version.LegacyVersion(ver)
> +        if (
> +            ver is None
> +            # Always pass installed package to pip, so that they can be
> +            # updated if the requested version changes
> +            or not _is_system_package(matcher.name)
> +            or not matcher.match(distlib.version.LegacyVersion(ver))

Wait, so what exactly does this change? In what cases will we
downgrade and in what cases will we be fine with what we already have?

I'm a little fuzzy on what precisely this fixes, though I gather it's
to do with Avocado-Framework.

>          ):
>              absent.append(spec)
>          else:
> --
> 2.40.1
>