From ee5e1f4ec3cfc0b32f68f1501cd9936fbeac7a40 Mon Sep 17 00:00:00 2001 From: Jiri Popelka Date: Mon, 18 Sep 2023 13:46:43 +0200 Subject: [PATCH 01/18] Add link to Copr builds --- README.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/README.md b/README.md index 6355ab5..8e3714d 100644 --- a/README.md +++ b/README.md @@ -23,3 +23,7 @@ For development and tests: ## Documentation - https://fedora-ci.github.io/rpmdeplint + +## RPM builds + +- https://copr.fedorainfracloud.org/coprs/g/osci/rpmdeplint From c5bb19a030df316f0fb06889f675b560f7cb4b76 Mon Sep 17 00:00:00 2001 From: Jiri Popelka Date: Wed, 20 Sep 2023 19:08:38 +0200 Subject: [PATCH 02/18] Remove no longer needed method rpm 4.11.3 is 9 years old. --- rpmdeplint/__init__.py | 67 +++--------------------------------------- 1 file changed, 4 insertions(+), 63 deletions(-) diff --git a/rpmdeplint/__init__.py b/rpmdeplint/__init__.py index 483cf45..3ff1d68 100644 --- a/rpmdeplint/__init__.py +++ b/rpmdeplint/__init__.py @@ -4,10 +4,7 @@ # (at your option) any later version. -import ctypes import logging -import os -import os.path from collections import defaultdict from collections.abc import Iterable from typing import Any @@ -62,15 +59,16 @@ def __init__(self) -> None: def add_package(self, pkg, dependencies: Iterable, problems: list): nevra: str = str(pkg) self._packagedeps[nevra]["dependencies"].extend(map(str, dependencies)) - if len(problems) != 0: + if problems: all_problems = [] # For each problem, find all the problematic RPM "rules" which # lead to the problem and also include them in # the `overall_problems` description. for problem in problems: all_problems.append(str(problem)) - for rule in problem.findallproblemrules(): - all_problems.append(rule.info().problemstr()) + all_problems.extend( + rule.info().problemstr() for rule in problem.findallproblemrules() + ) self._packagedeps[nevra]["problems"].extend(all_problems) self._packages_with_problems.add(nevra) self._overall_problems.update(all_problems) @@ -348,9 +346,6 @@ def _file_conflict_is_permitted(self, left, right, filename: str) -> bool: Returns True if rpm would allow both the given packages to share ownership of the given filename. """ - if not hasattr(rpm, "files"): - return self._file_conflict_is_permitted_rpm411(left, right, filename) - ts = rpm.TransactionSet() ts.setVSFlags(rpm._RPMVSF_NOSIGNATURES) @@ -376,60 +371,6 @@ def _file_conflict_is_permitted(self, left, right, filename: str) -> bool: return True return False - def _file_conflict_is_permitted_rpm411(self, left, right, filename: str) -> bool: - # In rpm 4.12+ the rpmfilesCompare() function is exposed nicely as the - # rpm.files.matches Python method. In earlier rpm versions there is - # nothing equivalent in the Python bindings, although we can use ctypes - # to poke around and call the older rpmfiCompare() C API directly... - librpm = ctypes.CDLL("librpm.so.3") - _rpm = ctypes.CDLL(os.path.join(os.path.dirname(rpm.__file__), "_rpm.so")) - - class rpmfi_s(ctypes.Structure): - pass - - librpm.rpmfiCompare.argtypes = [ - ctypes.POINTER(rpmfi_s), - ctypes.POINTER(rpmfi_s), - ] - librpm.rpmfiCompare.restype = ctypes.c_int - _rpm.fiFromFi.argtypes = [ctypes.py_object] - _rpm.fiFromFi.restype = ctypes.POINTER(rpmfi_s) - - ts = rpm.TransactionSet() - ts.setVSFlags(rpm._RPMVSF_NOSIGNATURES) - - left_hdr = ts.hdrFromFdno(open(left.lookup_location()[0], "rb")) - right_hdr = ts.hdrFromFdno(open(self.download_package_header(right), "rb")) - left_fi = rpm.fi(left_hdr) - try: - while left_fi.FN() != filename: - left_fi.next() - except StopIteration as e: - raise KeyError(f"Entry {filename} not found in {left}") from e - right_fi = rpm.fi(right_hdr) - try: - while right_fi.FN() != filename: - right_fi.next() - except StopIteration as exc: - raise KeyError(f"Entry {filename} not found in {right}") from exc - if librpm.rpmfiCompare(_rpm.fiFromFi(left_fi), _rpm.fiFromFi(right_fi)) == 0: - logger.debug( - "Conflict on %s between %s and %s permitted because files match", - filename, - left, - right, - ) - return True - if left_fi.FColor() != right_fi.FColor(): - logger.debug( - "Conflict on %s between %s and %s permitted because colors differ", - filename, - left, - right, - ) - return True - return False - def find_conflicts(self) -> list[str]: """ Find undeclared file conflicts in the packages under test. From 1267e71bd508ab17fb3d9108af84171325239615 Mon Sep 17 00:00:00 2001 From: Jiri Popelka Date: Thu, 21 Sep 2023 12:14:48 +0200 Subject: [PATCH 03/18] Move cache related functions to Cache class and *_yumvars() to Repo class. --- acceptance_tests/test_check.py | 5 +- acceptance_tests/test_check_sat.py | 6 +- rpmdeplint/repodata.py | 248 +++++++++++++---------------- rpmdeplint/tests/test_repodata.py | 20 +-- 4 files changed, 128 insertions(+), 151 deletions(-) diff --git a/acceptance_tests/test_check.py b/acceptance_tests/test_check.py index 1416c3a..4e24c20 100644 --- a/acceptance_tests/test_check.py +++ b/acceptance_tests/test_check.py @@ -12,7 +12,7 @@ from rpmfluff import SimpleRpmBuild, SourceFile from rpmfluff.yumrepobuild import YumRepoBuild -from rpmdeplint.repodata import cache_entry_path +from rpmdeplint.repodata import Cache def expected_cache_path(repodir: str, name: str, old=False) -> Path: @@ -23,7 +23,7 @@ def expected_cache_path(repodir: str, name: str, old=False) -> Path: """ file = next(Path(repodir, "repodata").glob(f"*-{name}.*")) checksum = file.name.split("-", 1)[0] - return cache_entry_path(checksum) / file.name if old else cache_entry_path(checksum) + return Cache.entry_path(checksum) / file.name if old else Cache.entry_path(checksum) def test_finds_all_problems(request, dir_server): @@ -326,5 +326,4 @@ def cleanUp(): assert exitcode == 1 assert err.startswith("Failed to download repodata") - assert "404" in err assert "Traceback" not in err diff --git a/acceptance_tests/test_check_sat.py b/acceptance_tests/test_check_sat.py index 23a2856..e4e7079 100644 --- a/acceptance_tests/test_check_sat.py +++ b/acceptance_tests/test_check_sat.py @@ -43,12 +43,12 @@ def cleanUp(): assert out == "" -def test_error_if_repository_names_not_provided(tmpdir): +def test_error_if_repository_names_not_provided(tmp_path): exitcode, out, err = run_rpmdeplint( - ["rpmdeplint", "check-sat", f"--repo={tmpdir.dirpath()}"] + ["rpmdeplint", "check-sat", f"--repo={tmp_path}"] ) assert exitcode == 2 assert ( - f"error: argument -r/--repo: Repo '{tmpdir.dirpath()}' is not in the form ," + f"error: argument -r/--repo: Repo '{tmp_path}' is not in the form ," in err ) diff --git a/rpmdeplint/repodata.py b/rpmdeplint/repodata.py index e48be07..37ed5ff 100644 --- a/rpmdeplint/repodata.py +++ b/rpmdeplint/repodata.py @@ -12,9 +12,10 @@ import shutil import tempfile import time +from contextlib import suppress from os import getenv from pathlib import Path -from typing import BinaryIO, Optional +from typing import BinaryIO, Optional, Union import librepo import requests @@ -40,74 +41,92 @@ class RepoDownloadError(Exception): """ -def get_yumvars() -> dict[str, str]: - # This is not all the yumvars, but hopefully good enough... - - try: - import dnf.conf - import dnf.rpm - except ImportError: - pass - else: - installroot = "" - subst = dnf.conf.Conf().substitutions - subst["releasever"] = dnf.rpm.detect_releasever(installroot) - return subst - - try: - import rpmUtils - import yum - import yum.config - except ImportError: - pass - else: - return { - "arch": rpmUtils.arch.getCanonArch(), - "basearch": rpmUtils.arch.getBaseArch(), - "releasever": yum.config._getsysver( - "/", ["system-release(releasever)", "redhat-release"] - ), - } - - # Probably not going to work but there's not much else we can do... - return { - "arch": "$arch", - "basearch": "$basearch", - "releasever": "$releasever", - } - - -def substitute_yumvars(s: str, yumvars: dict[str, str]) -> str: - for name, value in yumvars.items(): - s = s.replace(f"${name}", value) - return s - - -def cache_base_path() -> Path: - default_cache_home = Path.home() / ".cache" - return Path(getenv("XDG_CACHE_HOME", default_cache_home)) / "rpmdeplint" - - -def cache_entry_path(checksum: str) -> Path: - return cache_base_path() / checksum[:1] / checksum[1:] - - -def clean_cache(): - expiry_time = time.time() - float( - os.environ.get("RPMDEPLINT_EXPIRY_SECONDS", "604800") - ) - if not cache_base_path().is_dir(): - return # nothing to do - for subdir in cache_base_path().iterdir(): - # Should be a subdirectory named after the first checksum letter - if not subdir.is_dir(): - continue - for entry in subdir.iterdir(): - if not entry.is_file(): +class Cache: + @staticmethod + def base_path() -> Path: + default_cache_home = Path.home() / ".cache" + return Path(getenv("XDG_CACHE_HOME", default_cache_home)) / "rpmdeplint" + + @staticmethod + def entry_path(checksum: str) -> Path: + return Cache.base_path() / checksum[:1] / checksum[1:] + + @staticmethod + def clean(): + expiry_time = time.time() - float(getenv("RPMDEPLINT_EXPIRY_SECONDS", 604800)) + if not Cache.base_path().is_dir(): + return # nothing to do + for subdir in Cache.base_path().iterdir(): + # Should be a subdirectory named after the first checksum letter + if not subdir.is_dir(): continue - if entry.stat().st_mtime < expiry_time: - logger.debug("Purging expired cache file %s", entry) - entry.unlink() + for entry in subdir.iterdir(): + if not entry.is_file(): + continue + if entry.stat().st_mtime < expiry_time: + logger.debug("Purging expired cache file %s", entry) + entry.unlink() + + @staticmethod + def download_repodata_file(checksum: str, url: str) -> BinaryIO: + """ + Each created file in cache becomes immutable, and is referenced in + the directory tree within XDG_CACHE_HOME as + $XDG_CACHE_HOME/rpmdeplint// + + Both metadata and the files to be cached are written to a tempdir first + then renamed to the cache dir atomically to avoid them potentially being + accessed before written to cache. + """ + filepath_in_cache: Path = Cache.entry_path(checksum) + try: + f = open(filepath_in_cache, "rb") + except OSError as e: + if e.errno == errno.ENOENT: + pass # cache entry does not exist, we will download it + elif e.errno == errno.EISDIR: + # This is the original cache directory layout, merged in commit + # 6f11c3708, although it didn't appear in any released version + # of rpmdeplint. To be helpful we will fix it up, by just + # deleting the directory and letting it be replaced by a file. + shutil.rmtree(filepath_in_cache, ignore_errors=True) + else: + raise + else: + logger.debug("Using cached file %s for %s", filepath_in_cache, url) + # Bump the modtime on the cache file we are using, + # since our cache expiry is LRU based on modtime. + os.utime(f.fileno()) # Python 3.3+ + return f + filepath_in_cache.parent.mkdir(parents=True, exist_ok=True) + fd, temp_path = tempfile.mkstemp(dir=filepath_in_cache.parent, text=False) + logger.debug("Downloading %s to cache temp file %s", url, temp_path) + try: + f = os.fdopen(fd, "wb+") + except Exception: + os.close(fd) + raise + try: + try: + response = requests_session.get(url, stream=True) + response.raise_for_status() + for chunk in response.raw.stream(decode_content=False): + f.write(chunk) + response.close() + except OSError as e: + raise RepoDownloadError( + f"Failed to download repodata file {os.path.basename(url)}" + ) from e + f.flush() + f.seek(0) + os.fchmod(f.fileno(), 0o644) + os.rename(temp_path, filepath_in_cache) + logger.debug("Using cached file %s for %s", filepath_in_cache, url) + return f + except Exception: + f.close() + os.unlink(temp_path) + raise class Repo: @@ -118,13 +137,36 @@ class Repo: yum_main_config_path = "/etc/yum.conf" yum_repos_config_glob = "/etc/yum.repos.d/*.repo" + @staticmethod + def get_yumvars() -> dict[str, str]: + with suppress(ModuleNotFoundError): + import dnf.conf + import dnf.rpm + + subst = dnf.conf.Conf().substitutions + subst["releasever"] = dnf.rpm.detect_releasever(installroot="") + return subst + + # Probably not going to work but there's not much else we can do... + return { + "arch": "$arch", + "basearch": "$basearch", + "releasever": "$releasever", + } + @classmethod def from_yum_config(cls): """ Yields Repo instances loaded from the system-wide Yum configuration in :file:`/etc/yum.conf` and :file:`/etc/yum.repos.d/`. """ - yumvars = get_yumvars() + + def substitute_yumvars(s: str, _yumvars: dict[str, str]) -> str: + for name, value in _yumvars.items(): + s = s.replace(f"${name}", value) + return s + + yumvars = cls.get_yumvars() config = configparser.RawConfigParser() config.read([cls.yum_main_config_path, *glob.glob(cls.yum_repos_config_glob)]) for section in config.sections(): @@ -189,7 +231,7 @@ def __init__( self.skip_if_unavailable = skip_if_unavailable def download_repodata(self): - clean_cache() + Cache.clean() logger.debug( "Loading repodata for %s from %s", self.name, self.baseurl or self.metalink ) @@ -220,10 +262,10 @@ def download_repodata(self): ) self._download_metadata_result(h, r) self._yum_repomd = r.yum_repomd - self.primary = self._download_repodata_file( + self.primary = Cache.download_repodata_file( self.primary_checksum, self.primary_url ) - self.filelists = self._download_repodata_file( + self.filelists = Cache.download_repodata_file( self.filelists_checksum, self.filelists_url ) @@ -235,67 +277,7 @@ def _download_metadata_result(self, handle: librepo.Handle, result: librepo.Resu f"Failed to download repodata for {self!r}: {ex.args[1]}" ) from ex - def _download_repodata_file(self, checksum: str, url: str) -> BinaryIO: - """ - Each created file in cache becomes immutable, and is referenced in - the directory tree within XDG_CACHE_HOME as - $XDG_CACHE_HOME/rpmdeplint// - - Both metadata and the files to be cached are written to a tempdir first - then renamed to the cache dir atomically to avoid them potentially being - accessed before written to cache. - """ - filepath_in_cache: Path = cache_entry_path(checksum) - try: - f = open(filepath_in_cache, "rb") - except OSError as e: - if e.errno == errno.ENOENT: - pass # cache entry does not exist, we will download it - elif e.errno == errno.EISDIR: - # This is the original cache directory layout, merged in commit - # 6f11c3708, although it didn't appear in any released version - # of rpmdeplint. To be helpful we will fix it up, by just - # deleting the directory and letting it be replaced by a file. - shutil.rmtree(filepath_in_cache, ignore_errors=True) - else: - raise - else: - logger.debug("Using cached file %s for %s", filepath_in_cache, url) - # Bump the modtime on the cache file we are using, - # since our cache expiry is LRU based on modtime. - os.utime(f.fileno()) # Python 3.3+ - return f - filepath_in_cache.parent.mkdir(parents=True, exist_ok=True) - fd, temp_path = tempfile.mkstemp(dir=filepath_in_cache.parent, text=False) - logger.debug("Downloading %s to cache temp file %s", url, temp_path) - try: - f = os.fdopen(fd, "wb+") - except Exception: - os.close(fd) - raise - try: - try: - response = requests_session.get(url, stream=True) - response.raise_for_status() - for chunk in response.raw.stream(decode_content=False): - f.write(chunk) - response.close() - except OSError as e: - raise RepoDownloadError( - f"Failed to download repodata file {os.path.basename(url)} for {self!r}: {e}" - ) from e - f.flush() - f.seek(0) - os.fchmod(f.fileno(), 0o644) - os.rename(temp_path, filepath_in_cache) - logger.debug("Using cached file %s for %s", filepath_in_cache, url) - return f - except Exception: - f.close() - os.unlink(temp_path) - raise - - def _is_header_complete(self, local_path: str) -> bool: + def _is_header_complete(self, local_path: Union[Path, str]) -> bool: """ Returns `True` if the RPM file `local_path` has complete RPM header. """ diff --git a/rpmdeplint/tests/test_repodata.py b/rpmdeplint/tests/test_repodata.py index c9e012b..6902882 100644 --- a/rpmdeplint/tests/test_repodata.py +++ b/rpmdeplint/tests/test_repodata.py @@ -3,12 +3,12 @@ # the Free Software Foundation; either version 2 of the License, or # (at your option) any later version. -import os import platform +from pathlib import Path import pytest -from rpmdeplint.repodata import Repo, RepoDownloadError, get_yumvars +from rpmdeplint.repodata import Repo, RepoDownloadError @pytest.fixture() @@ -73,7 +73,7 @@ def test_loads_system_yum_repo_with_substitutions(yumdir, monkeypatch): ensure=True, ) monkeypatch.setattr( - "rpmdeplint.repodata.get_yumvars", + "rpmdeplint.repodata.Repo.get_yumvars", lambda: { "releasever": "21", "basearch": "s390x", @@ -91,16 +91,12 @@ def test_yumvars(): # also will be different in mock for example (where neither yum nor dnf are # present). So the best we can do is touch the code path and makes sure it # gives back some values. - yumvars = get_yumvars() - if ( - "ID=fedora\nVERSION_ID=25\n" in open("/etc/os-release").read() - and os.path.exists("/usr/bin/dnf") - and platform.machine() == "x86_64" - ): + yumvars = Repo.get_yumvars() + if Path("/usr/bin/dnf").is_file(): # The common case on developer's machines - assert yumvars["arch"] == "x86_64" - assert yumvars["basearch"] == "x86_64" - assert yumvars["releasever"] == "25" + assert yumvars["arch"] == platform.machine() + assert yumvars["basearch"] == platform.machine() + assert int(yumvars["releasever"]) else: # Everywhere else, just assume it's fine assert "arch" in yumvars From 66abc4a9a41d2c14a6ccbdc6728d737a10d4f7ef Mon Sep 17 00:00:00 2001 From: Jiri Popelka Date: Thu, 21 Sep 2023 13:15:25 +0200 Subject: [PATCH 04/18] Don't pass Handle into _download_metadata_result() Use the one from object attributes. --- rpmdeplint/repodata.py | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/rpmdeplint/repodata.py b/rpmdeplint/repodata.py index 37ed5ff..1d09a8d 100644 --- a/rpmdeplint/repodata.py +++ b/rpmdeplint/repodata.py @@ -230,6 +230,8 @@ def __init__( self.metalink = metalink self.skip_if_unavailable = skip_if_unavailable + self.librepo_handle: Optional[librepo.Handle] = None + def download_repodata(self): Cache.clean() logger.debug( @@ -251,7 +253,7 @@ def download_repodata(self): h.setopt(librepo.LRO_INTERRUPTIBLE, True) h.setopt(librepo.LRO_YUMDLIST, []) if self.baseurl and os.path.isdir(self.baseurl): - self._download_metadata_result(h, r) + self._download_metadata_result(r) self._yum_repomd = r.yum_repomd self._root_path = self.baseurl self.primary = open(self.primary_url, "rb") @@ -260,7 +262,7 @@ def download_repodata(self): self._root_path = h.destdir = tempfile.mkdtemp( self.name, prefix=REPO_CACHE_NAME_PREFIX, dir=REPO_CACHE_DIR ) - self._download_metadata_result(h, r) + self._download_metadata_result(r) self._yum_repomd = r.yum_repomd self.primary = Cache.download_repodata_file( self.primary_checksum, self.primary_url @@ -269,9 +271,11 @@ def download_repodata(self): self.filelists_checksum, self.filelists_url ) - def _download_metadata_result(self, handle: librepo.Handle, result: librepo.Result): + def _download_metadata_result(self, result: librepo.Result): + if not self.librepo_handle: + raise RuntimeError("No Librepo Handle") try: - handle.perform(result) + self.librepo_handle.perform(result) except librepo.LibrepoException as ex: raise RepoDownloadError( f"Failed to download repodata for {self!r}: {ex.args[1]}" @@ -325,7 +329,7 @@ def download_package_header(self, location: str, baseurl: str) -> str: only when complete RPM file is downloaded. """ local_path = os.path.join(self._root_path, os.path.basename(location)) - if self.librepo_handle.local: + if self.librepo_handle and self.librepo_handle.local: logger.debug("Using package %s from local filesystem directly", local_path) return local_path From e6340c7183b7333923e34e94b015b4fcfc3cc434 Mon Sep 17 00:00:00 2001 From: Jiri Popelka Date: Fri, 22 Sep 2023 11:52:28 +0200 Subject: [PATCH 05/18] post-release is deprecated https://github.com/pypa/setuptools_scm/blob/main/docs/extending.md#available-implementations --- pyproject.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pyproject.toml b/pyproject.toml index 38ab6f8..aed6c23 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -55,7 +55,7 @@ rpmdeplint = "rpmdeplint.cli:main" [tool.hatch.version] source = "vcs" -raw-options.version_scheme = "post-release" +raw-options.version_scheme = "no-guess-dev" [tool.ruff] select = [ From 5108289ef839a9e25bff6abdf5ca6ef7db67e159 Mon Sep 17 00:00:00 2001 From: Jiri Popelka Date: Fri, 22 Sep 2023 11:57:25 +0200 Subject: [PATCH 06/18] LRO_MIRRORLIST is deprecated Also, use class properties instead of setopt(). https://github.com/rpm-software-management/librepo/blob/7c9af219abd49f8961542b7622fc82cfdaa572e3/librepo/python/__init__.py#L43 --- rpmdeplint/repodata.py | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/rpmdeplint/repodata.py b/rpmdeplint/repodata.py index 1d09a8d..9b00657 100644 --- a/rpmdeplint/repodata.py +++ b/rpmdeplint/repodata.py @@ -12,6 +12,7 @@ import shutil import tempfile import time +from collections.abc import Iterator from contextlib import suppress from os import getenv from pathlib import Path @@ -155,7 +156,7 @@ def get_yumvars() -> dict[str, str]: } @classmethod - def from_yum_config(cls): + def from_yum_config(cls) -> Iterator["Repo"]: """ Yields Repo instances loaded from the system-wide Yum configuration in :file:`/etc/yum.conf` and :file:`/etc/yum.repos.d/`. @@ -243,15 +244,12 @@ def download_repodata(self): if self.baseurl: h.urls = [self.baseurl] if self.metalink: - h.mirrorlist = self.metalink - h.setopt( - librepo.LRO_DESTDIR, - tempfile.mkdtemp( - self.name, prefix=REPO_CACHE_NAME_PREFIX, dir=REPO_CACHE_DIR - ), + h.metalinkurl = self.metalink + h.destdir = tempfile.mkdtemp( + self.name, prefix=REPO_CACHE_NAME_PREFIX, dir=REPO_CACHE_DIR ) - h.setopt(librepo.LRO_INTERRUPTIBLE, True) - h.setopt(librepo.LRO_YUMDLIST, []) + h.interruptible = True + h.yumdlist = [] # Download repomd.xml only if self.baseurl and os.path.isdir(self.baseurl): self._download_metadata_result(r) self._yum_repomd = r.yum_repomd From bba9410040710e3aeac5334807ff420bb10f7940 Mon Sep 17 00:00:00 2001 From: Jiri Popelka Date: Fri, 22 Sep 2023 13:58:37 +0200 Subject: [PATCH 07/18] Use Result.rpmmd_repomd instead of deprecated Result.yum_repomd https://github.com/rpm-software-management/librepo/blob/7c9af219abd49f8961542b7622fc82cfdaa572e3/librepo/python/__init__.py#L851 Also, we don't need to pass Result instance into the Handle.perform() as it will create (and return) its own if not specified. https://github.com/rpm-software-management/librepo/blob/7c9af219abd49f8961542b7622fc82cfdaa572e3/librepo/python/__init__.py#L1594 --- rpmdeplint/repodata.py | 37 +++++++++++++++++++------------------ 1 file changed, 19 insertions(+), 18 deletions(-) diff --git a/rpmdeplint/repodata.py b/rpmdeplint/repodata.py index 9b00657..d3fd185 100644 --- a/rpmdeplint/repodata.py +++ b/rpmdeplint/repodata.py @@ -16,7 +16,7 @@ from contextlib import suppress from os import getenv from pathlib import Path -from typing import BinaryIO, Optional, Union +from typing import Any, BinaryIO, Optional, Union import librepo import requests @@ -232,6 +232,7 @@ def __init__( self.skip_if_unavailable = skip_if_unavailable self.librepo_handle: Optional[librepo.Handle] = None + self._rpmmd_repomd: Optional[dict[str, Any]] = None def download_repodata(self): Cache.clean() @@ -239,7 +240,6 @@ def download_repodata(self): "Loading repodata for %s from %s", self.name, self.baseurl or self.metalink ) self.librepo_handle = h = librepo.Handle() - r = librepo.Result() h.repotype = librepo.LR_YUMREPO if self.baseurl: h.urls = [self.baseurl] @@ -251,8 +251,7 @@ def download_repodata(self): h.interruptible = True h.yumdlist = [] # Download repomd.xml only if self.baseurl and os.path.isdir(self.baseurl): - self._download_metadata_result(r) - self._yum_repomd = r.yum_repomd + self._download_metadata_result() self._root_path = self.baseurl self.primary = open(self.primary_url, "rb") self.filelists = open(self.filelists_url, "rb") @@ -260,8 +259,7 @@ def download_repodata(self): self._root_path = h.destdir = tempfile.mkdtemp( self.name, prefix=REPO_CACHE_NAME_PREFIX, dir=REPO_CACHE_DIR ) - self._download_metadata_result(r) - self._yum_repomd = r.yum_repomd + self._download_metadata_result() self.primary = Cache.download_repodata_file( self.primary_checksum, self.primary_url ) @@ -269,15 +267,16 @@ def download_repodata(self): self.filelists_checksum, self.filelists_url ) - def _download_metadata_result(self, result: librepo.Result): + def _download_metadata_result(self) -> None: if not self.librepo_handle: raise RuntimeError("No Librepo Handle") try: - self.librepo_handle.perform(result) + result: librepo.Result = self.librepo_handle.perform() except librepo.LibrepoException as ex: raise RepoDownloadError( f"Failed to download repodata for {self!r}: {ex.args[1]}" ) from ex + self._rpmmd_repomd = result.rpmmd_repomd def _is_header_complete(self, local_path: Union[Path, str]) -> bool: """ @@ -363,32 +362,34 @@ def download_package_header(self, location: str, baseurl: str) -> str: return target.local_path @property - def yum_repomd(self): - return self._yum_repomd - - @property - def repomd_fn(self) -> str: - return os.path.join(self._root_path, "repodata", "repomd.xml") + def repomd(self) -> dict[str, Any]: + if not self._rpmmd_repomd: + raise RuntimeError("_rpmmd_repomd is not set") + return self._rpmmd_repomd @property def primary_url(self) -> str: if not self.baseurl: raise RuntimeError("baseurl not specified") - return os.path.join(self.baseurl, self.yum_repomd["primary"]["location_href"]) + return os.path.join( + self.baseurl, self.repomd["records"]["primary"]["location_href"] + ) @property def primary_checksum(self) -> str: - return self.yum_repomd["primary"]["checksum"] + return self.repomd["records"]["primary"]["checksum"] @property def filelists_checksum(self) -> str: - return self.yum_repomd["filelists"]["checksum"] + return self.repomd["records"]["filelists"]["checksum"] @property def filelists_url(self) -> str: if not self.baseurl: raise RuntimeError("baseurl not specified") - return os.path.join(self.baseurl, self.yum_repomd["filelists"]["location_href"]) + return os.path.join( + self.baseurl, self.repomd["records"]["filelists"]["location_href"] + ) def __repr__(self): if self.baseurl: From 56ed1865cfc03d13f9075a67646eb20c8ebf45f3 Mon Sep 17 00:00:00 2001 From: Jiri Popelka Date: Fri, 22 Sep 2023 14:51:41 +0200 Subject: [PATCH 08/18] Move the Handle initialization into _download_metadata_result() The tempfile.mkdtemp() was also called twice, which looks like https://bugzilla.redhat.com/show_bug.cgi?id=1343247#c17 --- rpmdeplint/repodata.py | 35 +++++++++++++++++------------------ 1 file changed, 17 insertions(+), 18 deletions(-) diff --git a/rpmdeplint/repodata.py b/rpmdeplint/repodata.py index d3fd185..0b5adee 100644 --- a/rpmdeplint/repodata.py +++ b/rpmdeplint/repodata.py @@ -233,33 +233,22 @@ def __init__( self.librepo_handle: Optional[librepo.Handle] = None self._rpmmd_repomd: Optional[dict[str, Any]] = None + self._root_path: str = "" + self.primary: Optional[BinaryIO] = None + self.filelists: Optional[BinaryIO] = None def download_repodata(self): Cache.clean() logger.debug( "Loading repodata for %s from %s", self.name, self.baseurl or self.metalink ) - self.librepo_handle = h = librepo.Handle() - h.repotype = librepo.LR_YUMREPO - if self.baseurl: - h.urls = [self.baseurl] - if self.metalink: - h.metalinkurl = self.metalink - h.destdir = tempfile.mkdtemp( - self.name, prefix=REPO_CACHE_NAME_PREFIX, dir=REPO_CACHE_DIR - ) - h.interruptible = True - h.yumdlist = [] # Download repomd.xml only + self._download_metadata_result() if self.baseurl and os.path.isdir(self.baseurl): - self._download_metadata_result() self._root_path = self.baseurl self.primary = open(self.primary_url, "rb") self.filelists = open(self.filelists_url, "rb") else: - self._root_path = h.destdir = tempfile.mkdtemp( - self.name, prefix=REPO_CACHE_NAME_PREFIX, dir=REPO_CACHE_DIR - ) - self._download_metadata_result() + self._root_path = self.librepo_handle.destdir self.primary = Cache.download_repodata_file( self.primary_checksum, self.primary_url ) @@ -268,8 +257,18 @@ def download_repodata(self): ) def _download_metadata_result(self) -> None: - if not self.librepo_handle: - raise RuntimeError("No Librepo Handle") + self.librepo_handle = h = librepo.Handle() + h.repotype = librepo.LR_YUMREPO + if self.baseurl: + h.urls = [self.baseurl] + if self.metalink: + h.metalinkurl = self.metalink + h.destdir = tempfile.mkdtemp( + self.name, prefix=REPO_CACHE_NAME_PREFIX, dir=REPO_CACHE_DIR + ) + h.interruptible = True + h.yumdlist = [] # Download repomd.xml only + try: result: librepo.Result = self.librepo_handle.perform() except librepo.LibrepoException as ex: From 28c647cd2155e0c9a6f9e17041abe9cd91c653e4 Mon Sep 17 00:00:00 2001 From: Jiri Popelka Date: Fri, 22 Sep 2023 18:31:07 +0200 Subject: [PATCH 09/18] Have is_local attribute to know whether the repo is a local dir The code in the 'if self.librepo_handle.local' hasn't been reached since 6f11c3708 when the setting of Handle.local option was removed. Again set the Handle.local if the repo is local so that we don't make local copies of any files and therefore don't need the tempdir for them. --- acceptance_tests/test_list_deps.py | 2 +- rpmdeplint/repodata.py | 21 +++++++++++----- rpmdeplint/tests/test_repodata.py | 40 ++++++++++++++++++++++++++++++ 3 files changed, 56 insertions(+), 7 deletions(-) diff --git a/acceptance_tests/test_list_deps.py b/acceptance_tests/test_list_deps.py index 07744cb..2627679 100644 --- a/acceptance_tests/test_list_deps.py +++ b/acceptance_tests/test_list_deps.py @@ -97,7 +97,7 @@ def cleanUp(): assert exitcode == 3 -def test_rpmdeplint_errors_on_unavailble_url(request): +def test_rpmdeplint_errors_on_unavailable_url(request): url = "http://example.test" p1 = SimpleRpmBuild("a", "0.1", "1", ["i386"]) p1.make() diff --git a/rpmdeplint/repodata.py b/rpmdeplint/repodata.py index 0b5adee..99ef964 100644 --- a/rpmdeplint/repodata.py +++ b/rpmdeplint/repodata.py @@ -227,10 +227,12 @@ def __init__( self.name = repo_name if not baseurl and not metalink: raise RuntimeError("Must specify either baseurl or metalink for repo") + baseurl = baseurl and baseurl.removeprefix("file://") self.baseurl = baseurl self.metalink = metalink self.skip_if_unavailable = skip_if_unavailable + self.is_local = baseurl is not None and Path(baseurl).is_dir() self.librepo_handle: Optional[librepo.Handle] = None self._rpmmd_repomd: Optional[dict[str, Any]] = None self._root_path: str = "" @@ -243,11 +245,13 @@ def download_repodata(self): "Loading repodata for %s from %s", self.name, self.baseurl or self.metalink ) self._download_metadata_result() - if self.baseurl and os.path.isdir(self.baseurl): + if self.is_local: + # path to the local repo dir self._root_path = self.baseurl self.primary = open(self.primary_url, "rb") self.filelists = open(self.filelists_url, "rb") else: + # tempdir with downloaded repomd.xml and rpms self._root_path = self.librepo_handle.destdir self.primary = Cache.download_repodata_file( self.primary_checksum, self.primary_url @@ -263,11 +267,16 @@ def _download_metadata_result(self) -> None: h.urls = [self.baseurl] if self.metalink: h.metalinkurl = self.metalink - h.destdir = tempfile.mkdtemp( - self.name, prefix=REPO_CACHE_NAME_PREFIX, dir=REPO_CACHE_DIR - ) + if self.is_local: + # no files will be downloaded + h.local = True + else: + # tempdir for repomd.xml files and downloaded rpms + h.destdir = tempfile.mkdtemp( + self.name, prefix=REPO_CACHE_NAME_PREFIX, dir=REPO_CACHE_DIR + ) h.interruptible = True - h.yumdlist = [] # Download repomd.xml only + h.yumdlist = [] # Download only repomd.xml from repodata/ try: result: librepo.Result = self.librepo_handle.perform() @@ -325,7 +334,7 @@ def download_package_header(self, location: str, baseurl: str) -> str: only when complete RPM file is downloaded. """ local_path = os.path.join(self._root_path, os.path.basename(location)) - if self.librepo_handle and self.librepo_handle.local: + if self.is_local: logger.debug("Using package %s from local filesystem directly", local_path) return local_path diff --git a/rpmdeplint/tests/test_repodata.py b/rpmdeplint/tests/test_repodata.py index 6902882..828e96c 100644 --- a/rpmdeplint/tests/test_repodata.py +++ b/rpmdeplint/tests/test_repodata.py @@ -4,9 +4,12 @@ # (at your option) any later version. import platform +import shutil from pathlib import Path import pytest +from rpmfluff import SimpleRpmBuild +from rpmfluff.yumrepobuild import YumRepoBuild from rpmdeplint.repodata import Repo, RepoDownloadError @@ -57,6 +60,20 @@ def test_loads_system_yum_repo_with_mirrorlist(yumdir): assert repos[0].metalink == "http://example.invalid/dummy" +def test_loads_system_yum_repo_local(yumdir): + local_repo = yumdir.join("local_repo").mkdir() + yumdir.join("yum.repos.d", "dummy.repo").write( + f"[dummy]\nname=Dummy\nbaseurl=file://{local_repo}\n", ensure=True + ) + + repos = list(Repo.from_yum_config()) + assert len(repos) == 1 + assert repos[0].name == "dummy" + assert repos[0].baseurl == str(local_repo) + assert repos[0].is_local + assert repos[0].metalink is None + + def test_skips_disabled_system_yum_repo(yumdir): yumdir.join("yum.repos.d", "dummy.repo").write( "[dummy]\nname=Dummy\nbaseurl=http://example.invalid/dummy\nenabled=0\n", @@ -129,3 +146,26 @@ def test_skip_if_unavailable_is_obeyed(yumdir): assert len(repos) == 1 assert repos[0].name == "dummy" assert repos[0].skip_if_unavailable is True + + +def test_download_repodata_from_local_repo(request): + satori = SimpleRpmBuild("satori", "1", "3", ["noarch"]) + repobuild = YumRepoBuild([satori]) + repobuild.make("noarch") + + def cleanUp(): + shutil.rmtree(repobuild.repoDir) + satori.clean() + + request.addfinalizer(cleanUp) + + repo = Repo(repo_name="dummy", baseurl=repobuild.repoDir) + assert repo.is_local + repo.download_repodata() + assert repo.repomd + assert repo.primary_checksum + assert repo.primary_url + assert repo.primary + assert repo.filelists_checksum + assert repo.filelists_url + assert repo.filelists From 8f13c14727adff88ce7e62ff3d91997ffe62cc0b Mon Sep 17 00:00:00 2001 From: Jiri Popelka Date: Fri, 22 Sep 2023 21:20:06 +0200 Subject: [PATCH 10/18] Remove old cache directory layout support. As the removed comment says, it didn't appear in any released version. --- acceptance_tests/test_check.py | 37 ++-------------------------------- rpmdeplint/repodata.py | 21 ++++--------------- 2 files changed, 6 insertions(+), 52 deletions(-) diff --git a/acceptance_tests/test_check.py b/acceptance_tests/test_check.py index 4e24c20..d32f923 100644 --- a/acceptance_tests/test_check.py +++ b/acceptance_tests/test_check.py @@ -15,7 +15,7 @@ from rpmdeplint.repodata import Cache -def expected_cache_path(repodir: str, name: str, old=False) -> Path: +def expected_cache_path(repodir: str, name: str) -> Path: """ For the test repo located in *repodir*, return the path within the rpmdeplint cache where we expect the metadata file with given suffix @@ -23,7 +23,7 @@ def expected_cache_path(repodir: str, name: str, old=False) -> Path: """ file = next(Path(repodir, "repodata").glob(f"*-{name}.*")) checksum = file.name.split("-", 1)[0] - return Cache.entry_path(checksum) / file.name if old else Cache.entry_path(checksum) + return Cache.entry_path(checksum) def test_finds_all_problems(request, dir_server): @@ -239,39 +239,6 @@ def cleanup2(): assert second_filelists_cache_path.exists() -def test_migrates_old_cache_layout(request, dir_server): - p1 = SimpleRpmBuild("a", "0.1", "1", ["i386"]) - repo = YumRepoBuild([p1]) - repo.make("i386") - dir_server.basepath = repo.repoDir - - def cleanUp(): - shutil.rmtree(repo.repoDir) - shutil.rmtree(p1.get_base_dir()) - - request.addfinalizer(cleanUp) - - old_cache_path = expected_cache_path(repo.repoDir, "primary.xml", old=True) - new_cache_path = expected_cache_path(repo.repoDir, "primary.xml") - - # Simulate the old cache path left over from an older version of rpmdeplint - old_cache_path.parent.mkdir(parents=True) - old_cache_path.write_text("lol\n") - - exitcode, out, err = run_rpmdeplint( - [ - "rpmdeplint", - "check", - f"--repo=base,{dir_server.url}", - p1.get_built_rpm("i386"), - ] - ) - assert exitcode == 0 - assert err == "" - assert not old_cache_path.exists() - assert new_cache_path.is_file() - - def test_prints_error_on_repo_download_failure(request): # Specifically we don't want an unhandled exception, because that triggers abrt. test_tool_rpm = SimpleRpmBuild("test-tool", "10", "3.el6", ["x86_64"]) diff --git a/rpmdeplint/repodata.py b/rpmdeplint/repodata.py index 99ef964..d93527a 100644 --- a/rpmdeplint/repodata.py +++ b/rpmdeplint/repodata.py @@ -5,11 +5,9 @@ import configparser -import errno import glob import logging import os -import shutil import tempfile import time from collections.abc import Iterator @@ -80,25 +78,14 @@ def download_repodata_file(checksum: str, url: str) -> BinaryIO: accessed before written to cache. """ filepath_in_cache: Path = Cache.entry_path(checksum) - try: - f = open(filepath_in_cache, "rb") - except OSError as e: - if e.errno == errno.ENOENT: - pass # cache entry does not exist, we will download it - elif e.errno == errno.EISDIR: - # This is the original cache directory layout, merged in commit - # 6f11c3708, although it didn't appear in any released version - # of rpmdeplint. To be helpful we will fix it up, by just - # deleting the directory and letting it be replaced by a file. - shutil.rmtree(filepath_in_cache, ignore_errors=True) - else: - raise - else: + if filepath_in_cache.is_file(): + f = filepath_in_cache.open(mode="rb") logger.debug("Using cached file %s for %s", filepath_in_cache, url) # Bump the modtime on the cache file we are using, # since our cache expiry is LRU based on modtime. - os.utime(f.fileno()) # Python 3.3+ + os.utime(f.fileno()) return f + filepath_in_cache.parent.mkdir(parents=True, exist_ok=True) fd, temp_path = tempfile.mkstemp(dir=filepath_in_cache.parent, text=False) logger.debug("Downloading %s to cache temp file %s", url, temp_path) From b127cac29f995a879e7e514732207bc5136b0774 Mon Sep 17 00:00:00 2001 From: Jiri Popelka Date: Mon, 25 Sep 2023 16:49:18 +0200 Subject: [PATCH 11/18] Use SimpleRpmBuild.clean() --- .pre-commit-config.yaml | 2 +- acceptance_tests/test_check.py | 16 +++---- acceptance_tests/test_check_conflicts.py | 50 +++++++++++----------- acceptance_tests/test_check_repoclosure.py | 48 ++++++++++----------- acceptance_tests/test_check_sat.py | 4 +- acceptance_tests/test_check_upgrade.py | 12 +++--- acceptance_tests/test_list_deps.py | 16 +++---- 7 files changed, 74 insertions(+), 74 deletions(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index bf9b31b..0630a06 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -12,7 +12,7 @@ repos: hooks: - id: prettier - repo: https://github.com/astral-sh/ruff-pre-commit - rev: v0.0.289 + rev: v0.0.291 hooks: - id: ruff args: [--fix, --exit-non-zero-on-fix] diff --git a/acceptance_tests/test_check.py b/acceptance_tests/test_check.py index d32f923..db86619 100644 --- a/acceptance_tests/test_check.py +++ b/acceptance_tests/test_check.py @@ -61,7 +61,7 @@ def test_finds_all_problems(request, dir_server): def cleanUp(): shutil.rmtree(baserepo.repoDir) for p in repo_packages + test_packages: - shutil.rmtree(p.get_base_dir()) + p.clean() request.addfinalizer(cleanUp) @@ -98,8 +98,8 @@ def test_guesses_arch_when_combined_with_noarch_package(request, dir_server): def cleanUp(): shutil.rmtree(baserepo.repoDir) - shutil.rmtree(p_noarch.get_base_dir()) - shutil.rmtree(p_archful.get_base_dir()) + p_noarch.clean() + p_archful.clean() request.addfinalizer(cleanUp) @@ -128,7 +128,7 @@ def test_cache_is_used_when_available(request, dir_server): def cleanUp(): shutil.rmtree(baserepo.repoDir) - shutil.rmtree(p1.get_base_dir()) + p1.clean() request.addfinalizer(cleanUp) @@ -183,7 +183,7 @@ def test_cache_doesnt_grow_unboundedly(request, dir_server): def cleanup(): shutil.rmtree(firstrepo.repoDir) - shutil.rmtree(p1.get_base_dir()) + p1.clean() request.addfinalizer(cleanup) @@ -210,7 +210,7 @@ def cleanup(): def cleanup2(): shutil.rmtree(secondrepo.repoDir) - shutil.rmtree(p2.get_base_dir()) + p2.clean() request.addfinalizer(cleanup2) @@ -245,7 +245,7 @@ def test_prints_error_on_repo_download_failure(request): test_tool_rpm.make() def cleanUp(): - shutil.rmtree(test_tool_rpm.get_base_dir()) + test_tool_rpm.clean() request.addfinalizer(cleanUp) @@ -278,7 +278,7 @@ def test_prints_error_on_repodata_file_download_failure(request, dir_server): def cleanUp(): shutil.rmtree(repo.repoDir) - shutil.rmtree(p1.get_base_dir()) + p1.clean() request.addfinalizer(cleanUp) diff --git a/acceptance_tests/test_check_conflicts.py b/acceptance_tests/test_check_conflicts.py index 8b40275..0dba02e 100644 --- a/acceptance_tests/test_check_conflicts.py +++ b/acceptance_tests/test_check_conflicts.py @@ -33,8 +33,8 @@ def test_finds_undeclared_file_conflict(request, dir_server): def cleanUp(): shutil.rmtree(baserepo.repoDir) - shutil.rmtree(p2.get_base_dir()) - shutil.rmtree(p1.get_base_dir()) + p2.clean() + p1.clean() request.addfinalizer(cleanUp) @@ -71,8 +71,8 @@ def test_finds_undeclared_file_conflict_with_repo_on_local_filesystem(request): def cleanUp(): shutil.rmtree(baserepo.repoDir) - shutil.rmtree(p2.get_base_dir()) - shutil.rmtree(p1.get_base_dir()) + p2.clean() + p1.clean() request.addfinalizer(cleanUp) @@ -110,8 +110,8 @@ def test_package_does_not_conflict_with_earlier_version_of_itself(request, dir_s def cleanUp(): shutil.rmtree(baserepo.repoDir) - shutil.rmtree(p2.get_base_dir()) - shutil.rmtree(p1.get_base_dir()) + p2.clean() + p1.clean() request.addfinalizer(cleanUp) @@ -150,8 +150,8 @@ def test_conflict_is_ignored_for_rpm_level_conflicts(request, dir_server): def cleanUp(): shutil.rmtree(baserepo.repoDir) - shutil.rmtree(p2.get_base_dir()) - shutil.rmtree(p1.get_base_dir()) + p2.clean() + p1.clean() request.addfinalizer(cleanUp) @@ -188,8 +188,8 @@ def test_conflict_is_ignored_if_files_match(request, dir_server): def cleanUp(): shutil.rmtree(baserepo.repoDir) - shutil.rmtree(p2.get_base_dir()) - shutil.rmtree(p1.get_base_dir()) + p2.clean() + p1.clean() request.addfinalizer(cleanUp) @@ -240,10 +240,10 @@ def test_conflict_not_ignored_if_contents_match_but_perms_differ(request, dir_se def cleanUp(): shutil.rmtree(baserepo.repoDir) - shutil.rmtree(basepackage.get_base_dir()) - shutil.rmtree(different_mode.get_base_dir()) - shutil.rmtree(different_owner.get_base_dir()) - shutil.rmtree(different_group.get_base_dir()) + basepackage.clean() + different_mode.clean() + different_owner.clean() + different_group.clean() request.addfinalizer(cleanUp) @@ -292,8 +292,8 @@ def test_conflict_is_ignored_if_file_colors_are_different(request, dir_server): def cleanUp(): shutil.rmtree(baserepo.repoDir) - shutil.rmtree(p2.get_base_dir()) - shutil.rmtree(p1.get_base_dir()) + p2.clean() + p1.clean() request.addfinalizer(cleanUp) @@ -333,7 +333,7 @@ def test_does_not_fail_with_signed_rpms(request, dir_server): def cleanUp(): shutil.rmtree(baserepo.repoDir) - shutil.rmtree(p2.get_base_dir()) + p2.clean() request.addfinalizer(cleanUp) @@ -372,8 +372,8 @@ def test_conflict_is_ignored_if_not_installable_concurrently(request, dir_server def cleanUp(): shutil.rmtree(repo.repoDir) - shutil.rmtree(glib_28.get_base_dir()) - shutil.rmtree(glib_26.get_base_dir()) + glib_28.clean() + glib_26.clean() request.addfinalizer(cleanUp) @@ -415,8 +415,8 @@ def test_finds_conflicts_in_installonly_packages(request, dir_server): def cleanUp(): shutil.rmtree(baserepo.repoDir) - shutil.rmtree(kernel1.get_base_dir()) - shutil.rmtree(kernel2.get_base_dir()) + kernel1.clean() + kernel2.clean() request.addfinalizer(cleanUp) @@ -465,8 +465,8 @@ def test_finds_conflict_against_older_subpackage(request, dir_server): def cleanUp(): shutil.rmtree(baserepo.repoDir) - shutil.rmtree(oldvim.get_base_dir()) - shutil.rmtree(newvim.get_base_dir()) + oldvim.clean() + newvim.clean() request.addfinalizer(cleanUp) @@ -521,8 +521,8 @@ def test_obeys_xml_base_when_downloading_packages(request, tmpdir, dir_server): p1.make() def cleanUp(): - shutil.rmtree(p2.get_base_dir()) - shutil.rmtree(p1.get_base_dir()) + p2.clean() + p1.clean() request.addfinalizer(cleanUp) diff --git a/acceptance_tests/test_check_repoclosure.py b/acceptance_tests/test_check_repoclosure.py index eb1c7f3..109e6a2 100644 --- a/acceptance_tests/test_check_repoclosure.py +++ b/acceptance_tests/test_check_repoclosure.py @@ -28,9 +28,9 @@ def test_catches_soname_change(request, dir_server): def cleanUp(): shutil.rmtree(baserepo.repoDir) - shutil.rmtree(p_depending.get_base_dir()) - shutil.rmtree(p_older.get_base_dir()) - shutil.rmtree(p_newer.get_base_dir()) + p_depending.clean() + p_older.clean() + p_newer.clean() request.addfinalizer(cleanUp) @@ -67,9 +67,9 @@ def test_catches_soname_change_with_package_rename(request, dir_server): def cleanUp(): shutil.rmtree(baserepo.repoDir) - shutil.rmtree(p_depending.get_base_dir()) - shutil.rmtree(p_older.get_base_dir()) - shutil.rmtree(p_newer.get_base_dir()) + p_depending.clean() + p_older.clean() + p_newer.clean() request.addfinalizer(cleanUp) @@ -102,8 +102,8 @@ def test_ignores_dependency_problems_in_packages_under_test(request, dir_server) def cleanUp(): shutil.rmtree(baserepo.repoDir) - shutil.rmtree(p2.get_base_dir()) - shutil.rmtree(p1.get_base_dir()) + p2.clean() + p1.clean() request.addfinalizer(cleanUp) @@ -140,10 +140,10 @@ def test_ignores_problems_in_older_packages(request, dir_server): def cleanUp(): shutil.rmtree(baserepo.repoDir) - shutil.rmtree(p_older_depending.get_base_dir()) - shutil.rmtree(p_newer_depending.get_base_dir()) - shutil.rmtree(p_older.get_base_dir()) - shutil.rmtree(p_newer.get_base_dir()) + p_older_depending.clean() + p_newer_depending.clean() + p_older.clean() + p_newer.clean() request.addfinalizer(cleanUp) @@ -179,10 +179,10 @@ def test_ignores_problems_in_obsoleted_packages(request, dir_server): def cleanUp(): shutil.rmtree(baserepo.repoDir) - shutil.rmtree(p_obsolete_depending.get_base_dir()) - shutil.rmtree(p_newer_depending.get_base_dir()) - shutil.rmtree(p_older.get_base_dir()) - shutil.rmtree(p_newer.get_base_dir()) + p_obsolete_depending.clean() + p_newer_depending.clean() + p_older.clean() + p_newer.clean() request.addfinalizer(cleanUp) @@ -212,8 +212,8 @@ def test_warns_on_preexisting_repoclosure_problems(request, dir_server): def cleanUp(): shutil.rmtree(baserepo.repoDir) - shutil.rmtree(p2.get_base_dir()) - shutil.rmtree(p1.get_base_dir()) + p2.clean() + p1.clean() request.addfinalizer(cleanUp) @@ -247,9 +247,9 @@ def test_works_on_different_platform_to_current(request, dir_server): def cleanUp(): shutil.rmtree(baserepo.repoDir) - shutil.rmtree(grep.get_base_dir()) - shutil.rmtree(needs_grep.get_base_dir()) - shutil.rmtree(package_to_test.get_base_dir()) + grep.clean() + needs_grep.clean() + package_to_test.clean() request.addfinalizer(cleanUp) @@ -282,9 +282,9 @@ def test_arch_set_manually_is_passed_to_sack(request, dir_server): def cleanUp(): shutil.rmtree(baserepo.repoDir) - shutil.rmtree(grep.get_base_dir()) - shutil.rmtree(needs_grep.get_base_dir()) - shutil.rmtree(package_to_test.get_base_dir()) + grep.clean() + needs_grep.clean() + package_to_test.clean() request.addfinalizer(cleanUp) diff --git a/acceptance_tests/test_check_sat.py b/acceptance_tests/test_check_sat.py index e4e7079..8094130 100644 --- a/acceptance_tests/test_check_sat.py +++ b/acceptance_tests/test_check_sat.py @@ -22,8 +22,8 @@ def test_shows_error_for_rpms(request, dir_server): def cleanUp(): shutil.rmtree(baserepo.repoDir) - shutil.rmtree(p2.get_base_dir()) - shutil.rmtree(p1.get_base_dir()) + p2.clean() + p1.clean() request.addfinalizer(cleanUp) diff --git a/acceptance_tests/test_check_upgrade.py b/acceptance_tests/test_check_upgrade.py index 57263b6..377051a 100644 --- a/acceptance_tests/test_check_upgrade.py +++ b/acceptance_tests/test_check_upgrade.py @@ -22,8 +22,8 @@ def test_finds_newer_version_in_repo(request, dir_server): def cleanUp(): shutil.rmtree(baserepo.repoDir) - shutil.rmtree(p2.get_base_dir()) - shutil.rmtree(p1.get_base_dir()) + p2.clean() + p1.clean() request.addfinalizer(cleanUp) @@ -56,8 +56,8 @@ def test_finds_obsoleting_package_in_repo(request, dir_server): def cleanUp(): shutil.rmtree(baserepo.repoDir) - shutil.rmtree(p2.get_base_dir()) - shutil.rmtree(p1.get_base_dir()) + p2.clean() + p1.clean() request.addfinalizer(cleanUp) @@ -89,8 +89,8 @@ def test_epoch(request, dir_server): def cleanUp(): shutil.rmtree(baserepo.repoDir) - shutil.rmtree(p2.get_base_dir()) - shutil.rmtree(p1.get_base_dir()) + p2.clean() + p1.clean() request.addfinalizer(cleanUp) diff --git a/acceptance_tests/test_list_deps.py b/acceptance_tests/test_list_deps.py index 2627679..beb8a9d 100644 --- a/acceptance_tests/test_list_deps.py +++ b/acceptance_tests/test_list_deps.py @@ -22,8 +22,8 @@ def test_lists_dependencies_for_rpms(request, dir_server): def cleanUp(): shutil.rmtree(baserepo.repoDir) - shutil.rmtree(p2.get_base_dir()) - shutil.rmtree(p1.get_base_dir()) + p2.clean() + p1.clean() request.addfinalizer(cleanUp) @@ -53,8 +53,8 @@ def test_lists_dependencies_for_rpms_served_from_filesystem(request): def cleanUp(): shutil.rmtree(baserepo.repoDir) - shutil.rmtree(p2.get_base_dir()) - shutil.rmtree(p1.get_base_dir()) + p2.clean() + p1.clean() request.addfinalizer(cleanUp) @@ -81,8 +81,8 @@ def test_errors_out_for_unsatisfiable_deps(request, dir_server): def cleanUp(): shutil.rmtree(baserepo.repoDir) - shutil.rmtree(p1.get_base_dir()) - shutil.rmtree(p2.get_base_dir()) + p2.clean() + p1.clean() request.addfinalizer(cleanUp) @@ -103,7 +103,7 @@ def test_rpmdeplint_errors_on_unavailable_url(request): p1.make() def cleanUp(): - shutil.rmtree(p1.get_base_dir()) + p1.clean() request.addfinalizer(cleanUp) @@ -139,7 +139,7 @@ def test_handles_invalid_rpm_without_crashing(request, dir_server, tmpdir): def cleanUp(): shutil.rmtree(baserepo.repoDir) - shutil.rmtree(p2.get_base_dir()) + p2.clean() request.addfinalizer(cleanUp) From c8f12bbfde1f983a5748b1cc61c788af237ee05a Mon Sep 17 00:00:00 2001 From: Jiri Popelka Date: Tue, 26 Sep 2023 18:46:17 +0200 Subject: [PATCH 12/18] Cache now works with metalink/mirrorlist The Repo.baseurl has been replaced with urls attribute containing baseurl and/or all urls resolved from metalink/mirrorlist. In Cache.download_repodata_file() we try them all until we succeed or raise if we don't. Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1343247#c18 --- acceptance_tests/test_check.py | 4 +- rpmdeplint/__init__.py | 4 +- rpmdeplint/repodata.py | 141 +++++++++++++++++------------- rpmdeplint/tests/test_repodata.py | 14 +-- 4 files changed, 91 insertions(+), 72 deletions(-) diff --git a/acceptance_tests/test_check.py b/acceptance_tests/test_check.py index db86619..a702bae 100644 --- a/acceptance_tests/test_check.py +++ b/acceptance_tests/test_check.py @@ -259,7 +259,7 @@ def cleanUp(): ) assert exitcode == 1 - assert err.startswith("Failed to download repodata") + assert err.startswith("Failed to download repo metadata") assert "Traceback" not in err @@ -292,5 +292,5 @@ def cleanUp(): ) assert exitcode == 1 - assert err.startswith("Failed to download repodata") + assert err.startswith("Failed to download repodata file") assert "Traceback" not in err diff --git a/rpmdeplint/__init__.py b/rpmdeplint/__init__.py index 3ff1d68..76cea1f 100644 --- a/rpmdeplint/__init__.py +++ b/rpmdeplint/__init__.py @@ -149,10 +149,10 @@ def __init__(self, repos: Iterable, packages: Iterable[str], arch=None): solv_repo = self.pool.add_repo(repo.name) # solv.xfopen does not accept unicode filenames on Python 2 solv_repo.add_rpmmd( - solv.xfopen_fd(str(repo.primary_url), repo.primary.fileno()), None + solv.xfopen_fd(repo.primary_urls[0], repo.primary.fileno()), None ) solv_repo.add_rpmmd( - solv.xfopen_fd(str(repo.filelists_url), repo.filelists.fileno()), + solv.xfopen_fd(repo.filelists_urls[0], repo.filelists.fileno()), None, solv.Repo.REPO_EXTEND_SOLVABLES, ) diff --git a/rpmdeplint/repodata.py b/rpmdeplint/repodata.py index d93527a..3e64c25 100644 --- a/rpmdeplint/repodata.py +++ b/rpmdeplint/repodata.py @@ -67,7 +67,7 @@ def clean(): entry.unlink() @staticmethod - def download_repodata_file(checksum: str, url: str) -> BinaryIO: + def download_repodata_file(checksum: str, urls: list[str]) -> BinaryIO: """ Each created file in cache becomes immutable, and is referenced in the directory tree within XDG_CACHE_HOME as @@ -77,10 +77,13 @@ def download_repodata_file(checksum: str, url: str) -> BinaryIO: then renamed to the cache dir atomically to avoid them potentially being accessed before written to cache. """ + if not urls: + raise ValueError("No urls specified to download repodata from") + filepath_in_cache: Path = Cache.entry_path(checksum) if filepath_in_cache.is_file(): f = filepath_in_cache.open(mode="rb") - logger.debug("Using cached file %s for %s", filepath_in_cache, url) + logger.debug("Using cached file %s for %s", filepath_in_cache, urls[0]) # Bump the modtime on the cache file we are using, # since our cache expiry is LRU based on modtime. os.utime(f.fileno()) @@ -88,28 +91,31 @@ def download_repodata_file(checksum: str, url: str) -> BinaryIO: filepath_in_cache.parent.mkdir(parents=True, exist_ok=True) fd, temp_path = tempfile.mkstemp(dir=filepath_in_cache.parent, text=False) - logger.debug("Downloading %s to cache temp file %s", url, temp_path) try: f = os.fdopen(fd, "wb+") except Exception: os.close(fd) raise try: - try: - response = requests_session.get(url, stream=True) - response.raise_for_status() - for chunk in response.raw.stream(decode_content=False): - f.write(chunk) - response.close() - except OSError as e: - raise RepoDownloadError( - f"Failed to download repodata file {os.path.basename(url)}" - ) from e + for index, url in enumerate(urls): + try: + logger.debug("Downloading %s", url) + response = requests_session.get(url, stream=True) + response.raise_for_status() + for chunk in response.raw.stream(decode_content=False): + f.write(chunk) + response.close() + break + except OSError as e: + msg = f"Failed to download repodata file {url}, {e}" + if index == len(urls) - 1: # last url in the list + raise RepoDownloadError(msg) from e + logger.debug(msg) f.flush() f.seek(0) os.fchmod(f.fileno(), 0o644) os.rename(temp_path, filepath_in_cache) - logger.debug("Using cached file %s for %s", filepath_in_cache, url) + logger.debug("Cached as %s", filepath_in_cache) return f except Exception: f.close() @@ -213,66 +219,76 @@ def __init__( """ self.name = repo_name if not baseurl and not metalink: - raise RuntimeError("Must specify either baseurl or metalink for repo") - baseurl = baseurl and baseurl.removeprefix("file://") - self.baseurl = baseurl + raise ValueError("Must specify either baseurl or metalink for repo") + if baseurl and not baseurl.startswith("http"): + baseurl = baseurl.removeprefix("file://") + if not Path(baseurl).is_dir(): + raise ValueError(f"baseurl {baseurl!r} is not a local directory") + self.urls = [baseurl] if baseurl else [] self.metalink = metalink self.skip_if_unavailable = skip_if_unavailable self.is_local = baseurl is not None and Path(baseurl).is_dir() - self.librepo_handle: Optional[librepo.Handle] = None + self._librepo_handle = librepo.Handle() self._rpmmd_repomd: Optional[dict[str, Any]] = None - self._root_path: str = "" self.primary: Optional[BinaryIO] = None self.filelists: Optional[BinaryIO] = None def download_repodata(self): Cache.clean() - logger.debug( - "Loading repodata for %s from %s", self.name, self.baseurl or self.metalink - ) self._download_metadata_result() if self.is_local: - # path to the local repo dir - self._root_path = self.baseurl - self.primary = open(self.primary_url, "rb") - self.filelists = open(self.filelists_url, "rb") + self.primary = open(self.primary_urls[0], "rb") + self.filelists = open(self.filelists_urls[0], "rb") else: - # tempdir with downloaded repomd.xml and rpms - self._root_path = self.librepo_handle.destdir self.primary = Cache.download_repodata_file( - self.primary_checksum, self.primary_url + self.primary_checksum, self.primary_urls ) self.filelists = Cache.download_repodata_file( - self.filelists_checksum, self.filelists_url + self.filelists_checksum, self.filelists_urls ) def _download_metadata_result(self) -> None: - self.librepo_handle = h = librepo.Handle() + def perform() -> librepo.Result: + try: + return self._librepo_handle.perform() + except librepo.LibrepoException as ex: + raise RepoDownloadError( + f"Failed to download repo metadata for {self!r}: {ex.args[1]}" + ) from ex + + logger.debug( + "Loading repodata for repo %r from %s", + self.name, + self.urls or self.metalink, + ) + + h = self._librepo_handle h.repotype = librepo.LR_YUMREPO - if self.baseurl: - h.urls = [self.baseurl] + if self.urls: + h.urls = self.urls if self.metalink: h.metalinkurl = self.metalink if self.is_local: # no files will be downloaded h.local = True else: - # tempdir for repomd.xml files and downloaded rpms + # tempdir for repomd.xml (metadata) and downloaded rpms h.destdir = tempfile.mkdtemp( self.name, prefix=REPO_CACHE_NAME_PREFIX, dir=REPO_CACHE_DIR ) - h.interruptible = True + h.interruptible = True # Download is interruptible h.yumdlist = [] # Download only repomd.xml from repodata/ - try: - result: librepo.Result = self.librepo_handle.perform() - except librepo.LibrepoException as ex: - raise RepoDownloadError( - f"Failed to download repodata for {self!r}: {ex.args[1]}" - ) from ex + result = perform() self._rpmmd_repomd = result.rpmmd_repomd + if self.metalink: + # Only download & parse metalink/mirrorlist + h.fetchmirrors = True + perform() + self.urls.extend(m for m in h.mirrors if m.startswith("http")) + def _is_header_complete(self, local_path: Union[Path, str]) -> bool: """ Returns `True` if the RPM file `local_path` has complete RPM header. @@ -336,7 +352,7 @@ def download_package_header(self, location: str, baseurl: str) -> str: location, base_url=baseurl, dest=self._root_path, - handle=self.librepo_handle, + handle=self._librepo_handle, byterangestart=0, byterangeend=byterangeend, ) @@ -356,39 +372,42 @@ def download_package_header(self, location: str, baseurl: str) -> str: logger.debug("Saved as %s", target.local_path) return target.local_path + @property + def _root_path(self) -> str: + # Path to the local repo dir or + # tempdir with downloaded repomd.xml and rpms + return self.urls[0] if self.is_local else self._librepo_handle.destdir + @property def repomd(self) -> dict[str, Any]: if not self._rpmmd_repomd: raise RuntimeError("_rpmmd_repomd is not set") return self._rpmmd_repomd - @property - def primary_url(self) -> str: - if not self.baseurl: - raise RuntimeError("baseurl not specified") - return os.path.join( - self.baseurl, self.repomd["records"]["primary"]["location_href"] - ) - @property def primary_checksum(self) -> str: return self.repomd["records"]["primary"]["checksum"] + @property + def primary_urls(self) -> list[str]: + location_href = self.repomd["records"]["primary"]["location_href"] + return [os.path.join(url, location_href) for url in self.urls] + @property def filelists_checksum(self) -> str: return self.repomd["records"]["filelists"]["checksum"] @property - def filelists_url(self) -> str: - if not self.baseurl: - raise RuntimeError("baseurl not specified") - return os.path.join( - self.baseurl, self.repomd["records"]["filelists"]["location_href"] - ) + def filelists_urls(self) -> list[str]: + location_href = self.repomd["records"]["filelists"]["location_href"] + return [os.path.join(url, location_href) for url in self.urls] def __repr__(self): - if self.baseurl: - return f"{self.__class__.__name__}(repo_name={self.name!r}, baseurl={self.baseurl!r})" - if self.metalink: - return f"{self.__class__.__name__}(repo_name={self.name!r}, metalink={self.metalink!r})" - return f"{self.__class__.__name__}(repo_name={self.name!r})" + return ( + "Repo(" + f"repo_name={self.name!r}, " + f"urls={self.urls!r}, " + f"metalink={self.metalink!r}, " + f"skip_if_unavailable={self.skip_if_unavailable!r}, " + f"is_local={self.is_local!r})" + ) diff --git a/rpmdeplint/tests/test_repodata.py b/rpmdeplint/tests/test_repodata.py index 828e96c..c678e8c 100644 --- a/rpmdeplint/tests/test_repodata.py +++ b/rpmdeplint/tests/test_repodata.py @@ -32,7 +32,7 @@ def test_loads_system_yum_repo_with_baseurl(yumdir): repos = list(Repo.from_yum_config()) assert len(repos) == 1 assert repos[0].name == "dummy" - assert repos[0].baseurl == "http://example.invalid/dummy" + assert repos[0].urls[0] == "http://example.invalid/dummy" assert repos[0].metalink is None @@ -44,7 +44,7 @@ def test_loads_system_yum_repo_with_metalink(yumdir): repos = list(Repo.from_yum_config()) assert len(repos) == 1 assert repos[0].name == "dummy" - assert repos[0].baseurl is None + assert not repos[0].urls assert repos[0].metalink == "http://example.invalid/dummy" @@ -56,7 +56,7 @@ def test_loads_system_yum_repo_with_mirrorlist(yumdir): repos = list(Repo.from_yum_config()) assert len(repos) == 1 assert repos[0].name == "dummy" - assert repos[0].baseurl is None + assert not repos[0].urls assert repos[0].metalink == "http://example.invalid/dummy" @@ -69,7 +69,7 @@ def test_loads_system_yum_repo_local(yumdir): repos = list(Repo.from_yum_config()) assert len(repos) == 1 assert repos[0].name == "dummy" - assert repos[0].baseurl == str(local_repo) + assert repos[0].urls[0] == str(local_repo) assert repos[0].is_local assert repos[0].metalink is None @@ -100,7 +100,7 @@ def test_loads_system_yum_repo_with_substitutions(yumdir, monkeypatch): repos = list(Repo.from_yum_config()) assert len(repos) == 1 assert repos[0].name == "dummy" - assert repos[0].baseurl == "http://example.invalid/21/s390x/" + assert repos[0].urls[0] == "http://example.invalid/21/s390x/" def test_yumvars(): @@ -164,8 +164,8 @@ def cleanUp(): repo.download_repodata() assert repo.repomd assert repo.primary_checksum - assert repo.primary_url + assert repo.primary_urls assert repo.primary assert repo.filelists_checksum - assert repo.filelists_url + assert repo.filelists_urls assert repo.filelists From 497b094286b6f31e1906d4715b8a1958001060bc Mon Sep 17 00:00:00 2001 From: Jiri Popelka Date: Thu, 28 Sep 2023 13:33:44 +0200 Subject: [PATCH 13/18] Allow passing a metalink for --repo instead of baseurl Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1454525 --- acceptance_tests/test_check_sat.py | 2 +- rpmdeplint/cli.py | 12 +++++++----- rpmdeplint/repodata.py | 8 ++++---- rpmdeplint/tests/test_dependency_analyzer.py | 6 +++--- rpmdeplint/tests/test_repodata.py | 4 ++-- 5 files changed, 17 insertions(+), 15 deletions(-) diff --git a/acceptance_tests/test_check_sat.py b/acceptance_tests/test_check_sat.py index 8094130..ebaf3e0 100644 --- a/acceptance_tests/test_check_sat.py +++ b/acceptance_tests/test_check_sat.py @@ -49,6 +49,6 @@ def test_error_if_repository_names_not_provided(tmp_path): ) assert exitcode == 2 assert ( - f"error: argument -r/--repo: Repo '{tmp_path}' is not in the form ," + f"error: argument -r/--repo: Repo '{tmp_path}' is not in the form ," in err ) diff --git a/rpmdeplint/cli.py b/rpmdeplint/cli.py index 3b6d1bc..b767de4 100644 --- a/rpmdeplint/cli.py +++ b/rpmdeplint/cli.py @@ -146,10 +146,12 @@ def dependency_analyzer_from_args(args): def comma_separated_repo(value: str) -> Repo: if "," not in value: raise argparse.ArgumentTypeError( - f"Repo {value!r} is not in the form ," + f"Repo {value!r} is not in the form ," ) - repo_name, baseurl = value.split(",", 1) - return Repo(repo_name, baseurl) + repo_name, url_or_path = value.split(",", 1) + if url_or_path.startswith("http") and "/metalink?" in url_or_path: + return Repo(name=repo_name, metalink=url_or_path) + return Repo(name=repo_name, baseurl=url_or_path) def add_common_dependency_analyzer_args(parser): @@ -162,12 +164,12 @@ def add_common_dependency_analyzer_args(parser): parser.add_argument( "-r", "--repo", - metavar="NAME,URL", + metavar="NAME,URL_OR_PATH", type=comma_separated_repo, action="append", dest="repos", default=[], - help="Name and URL of a repo to test against", + help="Name and (baseurl or metalink or local path) of a repo to test against", ) parser.add_argument( "-R", diff --git a/rpmdeplint/repodata.py b/rpmdeplint/repodata.py index 3e64c25..3c1c5fe 100644 --- a/rpmdeplint/repodata.py +++ b/rpmdeplint/repodata.py @@ -200,13 +200,13 @@ def substitute_yumvars(s: str, _yumvars: dict[str, str]) -> str: def __init__( self, - repo_name: str, + name: str, baseurl: Optional[str] = None, metalink: Optional[str] = None, skip_if_unavailable: bool = False, ): """ - :param repo_name: Name of the repository, for example "fedora-updates" + :param name: Name of the repository, for example "fedora-updates" (used in problems and error messages) :param baseurl: URL or filesystem path to the base of the repository (there should be a repodata subdirectory under this) @@ -217,7 +217,7 @@ def __init__( Exactly one of the *baseurl* or *metalink* parameters must be supplied. """ - self.name = repo_name + self.name = name if not baseurl and not metalink: raise ValueError("Must specify either baseurl or metalink for repo") if baseurl and not baseurl.startswith("http"): @@ -405,7 +405,7 @@ def filelists_urls(self) -> list[str]: def __repr__(self): return ( "Repo(" - f"repo_name={self.name!r}, " + f"name={self.name!r}, " f"urls={self.urls!r}, " f"metalink={self.metalink!r}, " f"skip_if_unavailable={self.skip_if_unavailable!r}, " diff --git a/rpmdeplint/tests/test_dependency_analyzer.py b/rpmdeplint/tests/test_dependency_analyzer.py index be03291..b5b6625 100644 --- a/rpmdeplint/tests/test_dependency_analyzer.py +++ b/rpmdeplint/tests/test_dependency_analyzer.py @@ -46,7 +46,7 @@ def test_repos(self): self.addCleanup(lemon_meringue_pie.clean) da = DependencyAnalyzer( - repos=[Repo(repo_name="base_1", baseurl=base_1_repo.repoDir)], + repos=[Repo(name="base_1", baseurl=base_1_repo.repoDir)], packages=[ apple.get_built_rpm("x86_64"), lemon_meringue_pie.get_built_rpm("x86_64"), @@ -74,8 +74,8 @@ def test_repos(self): da = DependencyAnalyzer( repos=[ - Repo(repo_name="base_1", baseurl=base_1_repo.repoDir), - Repo(repo_name="base_2", baseurl=base_2_repo.repoDir), + Repo(name="base_1", baseurl=base_1_repo.repoDir), + Repo(name="base_2", baseurl=base_2_repo.repoDir), ], packages=[ apple.get_built_rpm("x86_64"), diff --git a/rpmdeplint/tests/test_repodata.py b/rpmdeplint/tests/test_repodata.py index c678e8c..b6a4c20 100644 --- a/rpmdeplint/tests/test_repodata.py +++ b/rpmdeplint/tests/test_repodata.py @@ -132,7 +132,7 @@ def test_bad_repo_url_raises_error(yumdir): with pytest.raises(RepoDownloadError) as rde: repos[0].download_repodata() assert "Cannot download repomd.xml" in str(rde.value) - assert "repo_name='dummy'" in str(rde.value) + assert "name='dummy'" in str(rde.value) def test_skip_if_unavailable_is_obeyed(yumdir): @@ -159,7 +159,7 @@ def cleanUp(): request.addfinalizer(cleanUp) - repo = Repo(repo_name="dummy", baseurl=repobuild.repoDir) + repo = Repo(name="dummy", baseurl=repobuild.repoDir) assert repo.is_local repo.download_repodata() assert repo.repomd From 56baae38e3174cf96220a39ba10c51941e30f604 Mon Sep 17 00:00:00 2001 From: Jiri Popelka Date: Thu, 28 Sep 2023 15:31:10 +0200 Subject: [PATCH 14/18] More types --- rpmdeplint/__init__.py | 88 ++++++++++++++++++++++++++---------------- 1 file changed, 55 insertions(+), 33 deletions(-) diff --git a/rpmdeplint/__init__.py b/rpmdeplint/__init__.py index 76cea1f..9348d0d 100644 --- a/rpmdeplint/__init__.py +++ b/rpmdeplint/__init__.py @@ -4,15 +4,26 @@ # (at your option) any later version. -import logging from collections import defaultdict from collections.abc import Iterable -from typing import Any +from logging import getLogger +from typing import Optional import rpm -import solv - -logger = logging.getLogger(__name__) +from solv import ( + Dataiterator, + Job, + Pool, + Problem, + Selection, + XSolvable, + xfopen_fd, +) +from solv import ( + Repo as SolvRepo, +) + +logger = getLogger(__name__) installonlypkgs = [ @@ -56,7 +67,9 @@ def __init__(self) -> None: self._packages_with_problems: set[str] = set() self._overall_problems: set[str] = set() - def add_package(self, pkg, dependencies: Iterable, problems: list): + def add_package( + self, pkg: XSolvable, dependencies: Iterable, problems: list[Problem] + ): nevra: str = str(pkg) self._packagedeps[nevra]["dependencies"].extend(map(str, dependencies)) if problems: @@ -112,7 +125,12 @@ class DependencyAnalyzer: methods to perform each check. """ - def __init__(self, repos: Iterable, packages: Iterable[str], arch=None): + def __init__( + self, + repos: Iterable, + packages: Iterable[str], + arch: Optional[str] = None, + ): """ :param repos: An iterable of :py:class:`rpmdeplint.repodata.Repo` instances :param packages: An iterable of RPM package paths to be tested @@ -120,11 +138,11 @@ def __init__(self, repos: Iterable, packages: Iterable[str], arch=None): # delayed import to avoid circular dependency from rpmdeplint.repodata import RepoDownloadError - self.pool = solv.Pool() + self.pool = Pool() self.pool.setarch(arch) #: List of :py:class:`solv.Solvable` to be tested (corresponding to *packages* parameter) - self.solvables = [] + self.solvables: list[XSolvable] = [] self.commandline_repo = self.pool.add_repo("@commandline") for rpmpath in packages: solvable = self.commandline_repo.add_rpm(rpmpath) @@ -149,12 +167,12 @@ def __init__(self, repos: Iterable, packages: Iterable[str], arch=None): solv_repo = self.pool.add_repo(repo.name) # solv.xfopen does not accept unicode filenames on Python 2 solv_repo.add_rpmmd( - solv.xfopen_fd(repo.primary_urls[0], repo.primary.fileno()), None + xfopen_fd(repo.primary_urls[0], repo.primary.fileno()), None ) solv_repo.add_rpmmd( - solv.xfopen_fd(repo.filelists_urls[0], repo.filelists.fileno()), + xfopen_fd(repo.filelists_urls[0], repo.filelists.fileno()), None, - solv.Repo.REPO_EXTEND_SOLVABLES, + SolvRepo.REPO_EXTEND_SOLVABLES, ) self.repos_by_name[repo.name] = repo @@ -167,8 +185,8 @@ def __init__(self, repos: Iterable, packages: Iterable[str], arch=None): # run the solver on this pool. multiversion_jobs = [] for name in installonlypkgs: - selection = self.pool.select(name, solv.Selection.SELECTION_PROVIDES) - multiversion_jobs.extend(selection.jobs(solv.Job.SOLVER_MULTIVERSION)) + selection = self.pool.select(name, Selection.SELECTION_PROVIDES) + multiversion_jobs.extend(selection.jobs(Job.SOLVER_MULTIVERSION)) self.pool.setpooljobs(multiversion_jobs) # Context manager protocol is only implemented for backwards compatibility. @@ -180,7 +198,7 @@ def __enter__(self): def __exit__(self, type, value, tb): return - def download_package_header(self, solvable) -> str: + def download_package_header(self, solvable: XSolvable) -> str: if solvable in self.solvables: # It's a package under test, nothing to download return solvable.lookup_location()[0] @@ -200,16 +218,16 @@ def try_to_install_all(self) -> DependencySet: ds = DependencySet() for solvable in self.solvables: logger.debug("Solving install jobs for %s", solvable) - jobs = solvable.Selection().jobs(solv.Job.SOLVER_INSTALL) + jobs = solvable.Selection().jobs(Job.SOLVER_INSTALL) if problems := solver.solve(jobs): ds.add_package(solvable, [], problems) else: ds.add_package(solvable, solver.transaction().newsolvables(), []) return ds - def _select_obsoleted_by(self, solvables: Iterable) -> Any: + def _select_obsoleted_by(self, solvables: Iterable[XSolvable]) -> Selection: """ - Returns a solv.Selection matching every solvable which is "obsoleted" + Returns a Selection matching every solvable which is "obsoleted" by some solvable in the given list -- either due to an explicit Obsoletes relationship, or because we have a solvable with the same name with a higher epoch-version-release. @@ -222,9 +240,9 @@ def _select_obsoleted_by(self, solvables: Iterable) -> Any: sel.add( self.pool.select( f"{solvable.name}.{solvable.arch} < {solvable.evr}", - solv.Selection.SELECTION_NAME - | solv.Selection.SELECTION_DOTARCH - | solv.Selection.SELECTION_REL, + Selection.SELECTION_NAME + | Selection.SELECTION_DOTARCH + | Selection.SELECTION_REL, ) ) for obsoletes_rel in solvable.lookup_deparray( @@ -275,9 +293,9 @@ def find_repoclosure_problems(self) -> list[str]: # XXX limit available packages to compatible arches? # (use libsolv archpolicies somehow) jobs = ( - solvable.Selection().jobs(solv.Job.SOLVER_INSTALL) - + obs_sel.jobs(solv.Job.SOLVER_ERASE) - + existing_obs_sel.jobs(solv.Job.SOLVER_ERASE) + solvable.Selection().jobs(Job.SOLVER_INSTALL) + + obs_sel.jobs(Job.SOLVER_ERASE) + + existing_obs_sel.jobs(Job.SOLVER_ERASE) ) if solver_problems := solver.solve(jobs): problem_msgs = [str(p) for p in solver_problems] @@ -286,8 +304,8 @@ def find_repoclosure_problems(self) -> list[str]: # excluded) then warn about it here but don't consider it # a problem. jobs = solvable.Selection().jobs( - solv.Job.SOLVER_INSTALL - ) + existing_obs_sel.jobs(solv.Job.SOLVER_ERASE) + Job.SOLVER_INSTALL + ) + existing_obs_sel.jobs(Job.SOLVER_ERASE) if existing_problems := solver.solve(jobs): for p in existing_problems: logger.warning( @@ -297,21 +315,23 @@ def find_repoclosure_problems(self) -> list[str]: problems.extend(problem_msgs) return problems - def _files_in_solvable(self, solvable) -> set: + def _files_in_solvable(self, solvable: XSolvable) -> set[str]: iterator = solvable.Dataiterator( self.pool.str2id("solvable:filelist"), None, - solv.Dataiterator.SEARCH_FILES | solv.Dataiterator.SEARCH_COMPLETE_FILELIST, + Dataiterator.SEARCH_FILES | Dataiterator.SEARCH_COMPLETE_FILELIST, ) return {match.str for match in iterator} - def _packages_can_be_installed_together(self, left, right) -> bool: + def _packages_can_be_installed_together( + self, left: XSolvable, right: XSolvable + ) -> bool: """ Returns True if the given packages can be installed together. """ solver = self.pool.Solver() - left_install_jobs = left.Selection().jobs(solv.Job.SOLVER_INSTALL) - right_install_jobs = right.Selection().jobs(solv.Job.SOLVER_INSTALL) + left_install_jobs = left.Selection().jobs(Job.SOLVER_INSTALL) + right_install_jobs = right.Selection().jobs(Job.SOLVER_INSTALL) # First check if each one can be installed on its own. If either of # these fails it is a warning, because it means we have no way to know # if they can be installed together or not. @@ -341,7 +361,9 @@ def _packages_can_be_installed_together(self, left, right) -> bool: return False return True - def _file_conflict_is_permitted(self, left, right, filename: str) -> bool: + def _file_conflict_is_permitted( + self, left: XSolvable, right: XSolvable, filename: str + ) -> bool: """ Returns True if rpm would allow both the given packages to share ownership of the given filename. @@ -446,7 +468,7 @@ def find_upgrade_problems(self) -> list[str]: # package in the repos is better than it, and we have a problem. self.pool.installed = self.commandline_repo try: - jobs = self.pool.Selection_all().jobs(solv.Job.SOLVER_UPDATE) + jobs = self.pool.Selection_all().jobs(Job.SOLVER_UPDATE) solver = self.pool.Solver() solver.set_flag(solver.SOLVER_FLAG_ALLOW_UNINSTALL, True) solver_problems = solver.solve(jobs) From 648ce2436ef36b0cab39dc89fb572a510916a93b Mon Sep 17 00:00:00 2001 From: Jiri Popelka Date: Thu, 28 Sep 2023 16:14:03 +0200 Subject: [PATCH 15/18] Update spec and Changelog for 2.0rc3 --- docs/CHANGES.rst | 12 ++++++++++++ rpmdeplint.spec | 2 +- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/docs/CHANGES.rst b/docs/CHANGES.rst index 81d4333..3dc7930 100644 --- a/docs/CHANGES.rst +++ b/docs/CHANGES.rst @@ -1,6 +1,18 @@ Changelog --------- +2.0rc3 +~~~~~~ + +* Repodata cache now works with metalink/mirrorlist + (`RHBZ#1343247 `__). +* Don't leak directories in `/var/tmp/rpmdeplint-*` + (`RHBZ#1343247 `__). +* Allow passing a metalink for --repo instead of baseurl + (`RHBZ#1454525 `__). +* Modernize deprecated stuff. +* Refactoring. + 2.0rc2 ~~~~~~ diff --git a/rpmdeplint.spec b/rpmdeplint.spec index 697a52a..be935a1 100644 --- a/rpmdeplint.spec +++ b/rpmdeplint.spec @@ -1,5 +1,5 @@ Name: rpmdeplint -Version: 2.0 +Version: 2.0rc3 Release: %autorelease Summary: Tool to find errors in RPM packages in the context of their dependency graph License: GPL-2.0-or-later From 11d738f81d7593323538fb245749fec2dc016b96 Mon Sep 17 00:00:00 2001 From: Jiri Popelka Date: Fri, 29 Sep 2023 10:50:39 +0200 Subject: [PATCH 16/18] Allow to specify just a repo name in --repo if it's already configured in /etc/yum.repos.d/ --- acceptance_tests/test_check_sat.py | 5 +-- docs/CHANGES.rst | 3 +- docs/rpmdeplint.rst | 43 ++++++++++++++----- rpmdeplint/cli.py | 25 +++++------ rpmdeplint/repodata.py | 38 ++++++++--------- rpmdeplint/tests/test_repodata.py | 66 ++++++++++++++++++------------ 6 files changed, 106 insertions(+), 74 deletions(-) diff --git a/acceptance_tests/test_check_sat.py b/acceptance_tests/test_check_sat.py index ebaf3e0..ef83722 100644 --- a/acceptance_tests/test_check_sat.py +++ b/acceptance_tests/test_check_sat.py @@ -48,7 +48,4 @@ def test_error_if_repository_names_not_provided(tmp_path): ["rpmdeplint", "check-sat", f"--repo={tmp_path}"] ) assert exitcode == 2 - assert ( - f"error: argument -r/--repo: Repo '{tmp_path}' is not in the form ," - in err - ) + assert f"error: argument -r/--repo: invalid repo value: '{tmp_path}'" in err diff --git a/docs/CHANGES.rst b/docs/CHANGES.rst index 3dc7930..61fed52 100644 --- a/docs/CHANGES.rst +++ b/docs/CHANGES.rst @@ -6,10 +6,11 @@ Changelog * Repodata cache now works with metalink/mirrorlist (`RHBZ#1343247 `__). -* Don't leak directories in `/var/tmp/rpmdeplint-*` +* Don't leak directories in :file:`/var/tmp/rpmdeplint-*` (`RHBZ#1343247 `__). * Allow passing a metalink for --repo instead of baseurl (`RHBZ#1454525 `__). +* Allow passing only a configured repo name for ``--repo``. * Modernize deprecated stuff. * Refactoring. diff --git a/docs/rpmdeplint.rst b/docs/rpmdeplint.rst index f53e92b..5a3bbec 100644 --- a/docs/rpmdeplint.rst +++ b/docs/rpmdeplint.rst @@ -17,24 +17,35 @@ RPM packages against given repositories. Options ~~~~~~~ -.. option:: --repo NAME,URL, -r NAME,URL +.. option:: --repo NAME[,URL_OR_PATH], -r NAME[,URL_OR_PATH] - Load yum repo from the given URL. You can also specify a local filesystem - path instead of a URL. + Load yum repository specified by name or by URL/path. - The NAME is for descriptive purposes only. It has no impact on dependency - resolution. If rpmdeplint finds a dependency problem relating to a package - in this repo, the NAME will appear in the error message. + If the repo is already configured in :file:`/etc/yum.repos.d/{*}.repo` + you can just specify its name, like:: + + --repo=fedora - Note that the URL should point at a directory containing - ``repodata/repomd.xml``. For example:: + Otherwise, specify a name and the baseurl or metalink. The NAME is for + descriptive purposes only in this case. It has no impact on dependency + resolution. If rpmdeplint finds a dependency problem relating to a package + in this repo, the NAME will appear in the error message. Examples:: --repo=fedora,https://download.fedoraproject.org/pub/fedora/linux/development/rawhide/Everything/x86_64/os/ + --repo=fedora,https://mirrors.fedoraproject.org/metalink?repo=rawhide&arch=x86_64 + + You can also specify a local filesystem path instead of a URL, like:: + + --repo=myrepo,/home/me/my_repo + + Note that the URL/path should point at a directory containing + ``repodata/repomd.xml``. Examples:: + .. option:: --repos-from-system, -R - Use yum repos from the system-wide configuration in :file:`/etc/yum.conf` - and :file:`/etc/yum.repos.d/{*}.repo`. Repos which are disabled in the + Use yum repos from the system-wide configuration in + :file:`/etc/yum.repos.d/{*}.repo`. Repos which are disabled in the configuration (``enabled=0``) are ignored. This option can be combined with one or more :option:`--repo` options. @@ -137,6 +148,18 @@ to check if it will cause dependency errors in Fedora:: --repo=fedora,https://download.fedoraproject.org/pub/fedora/linux/development/rawhide/Everything/x86_64/os/ \ greenwave-0.6.1-0.git.2.2529bfb.fc29.noarch.rpm +You don't have to specify the URLs of the repos if they are already configured in `/etc/yum.repos.d/`:: + + rpmdeplint check \ + --repo=fedora --repo=updates \ + greenwave-0.6.1-0.git.2.2529bfb.fc29.noarch.rpm + +or use all configured repos:: + + rpmdeplint check \ + --repos-from-system \ + greenwave-0.6.1-0.git.2.2529bfb.fc29.noarch.rpm + You can also use a local filesystem path instead of an absolute URL for the repos to test against. For example, if you are offline you could re-use your local dnf cache. (Note that rpmdeplint may need to fetch packages for file diff --git a/rpmdeplint/cli.py b/rpmdeplint/cli.py index b767de4..ef1c5e8 100644 --- a/rpmdeplint/cli.py +++ b/rpmdeplint/cli.py @@ -143,15 +143,16 @@ def dependency_analyzer_from_args(args): return DependencyAnalyzer(repos, list(args.rpms), arch=args.arch) -def comma_separated_repo(value: str) -> Repo: - if "," not in value: - raise argparse.ArgumentTypeError( - f"Repo {value!r} is not in the form ," - ) - repo_name, url_or_path = value.split(",", 1) - if url_or_path.startswith("http") and "/metalink?" in url_or_path: - return Repo(name=repo_name, metalink=url_or_path) - return Repo(name=repo_name, baseurl=url_or_path) +def repo(value: str) -> Repo: + if "," in value: + repo_name, url_or_path = value.split(",", 1) + if url_or_path.startswith("http") and "/metalink?" in url_or_path: + return Repo(name=repo_name, metalink=url_or_path) + return Repo(name=repo_name, baseurl=url_or_path) + + if repos := list(Repo.from_yum_config(name=value)): + return repos[0] + raise ValueError(f"Repo {value} is not configured") def add_common_dependency_analyzer_args(parser): @@ -164,12 +165,12 @@ def add_common_dependency_analyzer_args(parser): parser.add_argument( "-r", "--repo", - metavar="NAME,URL_OR_PATH", - type=comma_separated_repo, + metavar="NAME[,URL_OR_PATH]", + type=repo, action="append", dest="repos", default=[], - help="Name and (baseurl or metalink or local path) of a repo to test against", + help="Name and optional (baseurl or metalink or local path) of a repo to test against", ) parser.add_argument( "-R", diff --git a/rpmdeplint/repodata.py b/rpmdeplint/repodata.py index 3c1c5fe..1df1a24 100644 --- a/rpmdeplint/repodata.py +++ b/rpmdeplint/repodata.py @@ -4,27 +4,26 @@ # (at your option) any later version. -import configparser -import glob -import logging import os -import tempfile -import time from collections.abc import Iterator +from configparser import ConfigParser from contextlib import suppress +from logging import getLogger from os import getenv from pathlib import Path +from tempfile import mkdtemp, mkstemp +from time import time from typing import Any, BinaryIO, Optional, Union import librepo -import requests import rpm +from requests import Session -logger = logging.getLogger(__name__) -requests_session = requests.Session() +logger = getLogger(__name__) +requests_session = Session() -REPO_CACHE_DIR = os.path.join(os.sep, "var", "tmp") +REPO_CACHE_DIR = "/var/tmp" REPO_CACHE_NAME_PREFIX = "rpmdeplint-" @@ -52,7 +51,7 @@ def entry_path(checksum: str) -> Path: @staticmethod def clean(): - expiry_time = time.time() - float(getenv("RPMDEPLINT_EXPIRY_SECONDS", 604800)) + expiry_time = time() - float(getenv("RPMDEPLINT_EXPIRY_SECONDS", 604800)) if not Cache.base_path().is_dir(): return # nothing to do for subdir in Cache.base_path().iterdir(): @@ -90,7 +89,7 @@ def download_repodata_file(checksum: str, urls: list[str]) -> BinaryIO: return f filepath_in_cache.parent.mkdir(parents=True, exist_ok=True) - fd, temp_path = tempfile.mkstemp(dir=filepath_in_cache.parent, text=False) + fd, temp_path = mkstemp(dir=filepath_in_cache.parent, text=False) try: f = os.fdopen(fd, "wb+") except Exception: @@ -128,8 +127,7 @@ class Repo: Represents a Yum ("repomd") package repository to test dependencies against. """ - yum_main_config_path = "/etc/yum.conf" - yum_repos_config_glob = "/etc/yum.repos.d/*.repo" + yum_repos_d = Path("/etc/yum.repos.d/") @staticmethod def get_yumvars() -> dict[str, str]: @@ -149,10 +147,10 @@ def get_yumvars() -> dict[str, str]: } @classmethod - def from_yum_config(cls) -> Iterator["Repo"]: + def from_yum_config(cls, name: str = "") -> Iterator["Repo"]: """ - Yields Repo instances loaded from the system-wide Yum - configuration in :file:`/etc/yum.conf` and :file:`/etc/yum.repos.d/`. + Yields Repo instances loaded from the system-wide + configuration in :file:`/etc/yum.repos.d/`. """ def substitute_yumvars(s: str, _yumvars: dict[str, str]) -> str: @@ -161,10 +159,10 @@ def substitute_yumvars(s: str, _yumvars: dict[str, str]) -> str: return s yumvars = cls.get_yumvars() - config = configparser.RawConfigParser() - config.read([cls.yum_main_config_path, *glob.glob(cls.yum_repos_config_glob)]) + config = ConfigParser() + config.read(cls.yum_repos_d.glob("*.repo")) for section in config.sections(): - if section == "main": + if name and section != name: continue if config.has_option(section, "enabled") and not config.getboolean( section, "enabled" @@ -274,7 +272,7 @@ def perform() -> librepo.Result: h.local = True else: # tempdir for repomd.xml (metadata) and downloaded rpms - h.destdir = tempfile.mkdtemp( + h.destdir = mkdtemp( self.name, prefix=REPO_CACHE_NAME_PREFIX, dir=REPO_CACHE_DIR ) h.interruptible = True # Download is interruptible diff --git a/rpmdeplint/tests/test_repodata.py b/rpmdeplint/tests/test_repodata.py index b6a4c20..fffa689 100644 --- a/rpmdeplint/tests/test_repodata.py +++ b/rpmdeplint/tests/test_repodata.py @@ -15,18 +15,14 @@ @pytest.fixture() -def yumdir(tmpdir, monkeypatch): - tmpdir.join("yum.conf").write("[main]\n") - monkeypatch.setattr(Repo, "yum_main_config_path", str(tmpdir.join("yum.conf"))) - monkeypatch.setattr( - Repo, "yum_repos_config_glob", str(tmpdir.join("yum.repos.d", "*.repo")) - ) - return tmpdir +def yumdir(tmp_path, monkeypatch): + monkeypatch.setattr(Repo, "yum_repos_d", tmp_path) + return tmp_path def test_loads_system_yum_repo_with_baseurl(yumdir): - yumdir.join("yum.repos.d", "dummy.repo").write( - "[dummy]\nname=Dummy\nbaseurl=http://example.invalid/dummy\n", ensure=True + yumdir.joinpath("dummy.repo").write_text( + "[dummy]\nname=Dummy\nbaseurl=http://example.invalid/dummy\n" ) repos = list(Repo.from_yum_config()) @@ -36,9 +32,28 @@ def test_loads_system_yum_repo_with_baseurl(yumdir): assert repos[0].metalink is None +def test_loads_system_yum_repo_specific(yumdir): + yumdir.joinpath("first.repo").write_text( + "[first]\nname=First\nbaseurl=http://example.invalid/first\n" + ) + yumdir.joinpath("dummy.repo").write_text( + "[dummy]\nname=Dummy\nbaseurl=http://example.invalid/dummy\n\n" + "[dummy-debuginfo]\nname=Dummy debuginfo\nbaseurl=http://example.invalid/other\n" + ) + yumdir.joinpath("last.repo").write_text( + "[last]\nname=Last\nbaseurl=http://example.invalid/last\n" + ) + + repos = list(Repo.from_yum_config(name="dummy")) + assert len(repos) == 1 + assert repos[0].name == "dummy" + assert repos[0].urls[0] == "http://example.invalid/dummy" + assert repos[0].metalink is None + + def test_loads_system_yum_repo_with_metalink(yumdir): - yumdir.join("yum.repos.d", "dummy.repo").write( - "[dummy]\nname=Dummy\nmetalink=http://example.invalid/dummy\n", ensure=True + yumdir.joinpath("dummy.repo").write_text( + "[dummy]\nname=Dummy\nmetalink=http://example.invalid/dummy\n" ) repos = list(Repo.from_yum_config()) @@ -49,8 +64,8 @@ def test_loads_system_yum_repo_with_metalink(yumdir): def test_loads_system_yum_repo_with_mirrorlist(yumdir): - yumdir.join("yum.repos.d", "dummy.repo").write( - "[dummy]\nname=Dummy\nmirrorlist=http://example.invalid/dummy\n", ensure=True + yumdir.joinpath("dummy.repo").write_text( + "[dummy]\nname=Dummy\nmirrorlist=http://example.invalid/dummy\n" ) repos = list(Repo.from_yum_config()) @@ -61,9 +76,10 @@ def test_loads_system_yum_repo_with_mirrorlist(yumdir): def test_loads_system_yum_repo_local(yumdir): - local_repo = yumdir.join("local_repo").mkdir() - yumdir.join("yum.repos.d", "dummy.repo").write( - f"[dummy]\nname=Dummy\nbaseurl=file://{local_repo}\n", ensure=True + local_repo = yumdir / "local_repo" + local_repo.mkdir() + yumdir.joinpath("dummy.repo").write_text( + f"[dummy]\nname=Dummy\nbaseurl=file://{local_repo}\n" ) repos = list(Repo.from_yum_config()) @@ -75,9 +91,8 @@ def test_loads_system_yum_repo_local(yumdir): def test_skips_disabled_system_yum_repo(yumdir): - yumdir.join("yum.repos.d", "dummy.repo").write( - "[dummy]\nname=Dummy\nbaseurl=http://example.invalid/dummy\nenabled=0\n", - ensure=True, + yumdir.joinpath("dummy.repo").write_text( + "[dummy]\nname=Dummy\nbaseurl=http://example.invalid/dummy\nenabled=0\n" ) repos = list(Repo.from_yum_config()) @@ -85,9 +100,8 @@ def test_skips_disabled_system_yum_repo(yumdir): def test_loads_system_yum_repo_with_substitutions(yumdir, monkeypatch): - yumdir.join("yum.repos.d", "dummy.repo").write( - "[dummy]\nname=Dummy\nbaseurl=http://example.invalid/$releasever/$basearch/\n", - ensure=True, + yumdir.joinpath("dummy.repo").write_text( + "[dummy]\nname=Dummy\nbaseurl=http://example.invalid/$releasever/$basearch/\n" ) monkeypatch.setattr( "rpmdeplint.repodata.Repo.get_yumvars", @@ -113,7 +127,7 @@ def test_yumvars(): # The common case on developer's machines assert yumvars["arch"] == platform.machine() assert yumvars["basearch"] == platform.machine() - assert int(yumvars["releasever"]) + assert yumvars["releasever"] else: # Everywhere else, just assume it's fine assert "arch" in yumvars @@ -122,9 +136,8 @@ def test_yumvars(): def test_bad_repo_url_raises_error(yumdir): - yumdir.join("yum.repos.d", "dummy.repo").write( + yumdir.joinpath("dummy.repo").write_text( "[dummy]\nname=Dummy\nbaseurl=http://example.invalid/dummy\nenabled=1\n", - ensure=True, ) repos = list(Repo.from_yum_config()) @@ -136,10 +149,9 @@ def test_bad_repo_url_raises_error(yumdir): def test_skip_if_unavailable_is_obeyed(yumdir): - yumdir.join("yum.repos.d", "dummy.repo").write( + yumdir.joinpath("dummy.repo").write_text( "[dummy]\nname=Dummy\nbaseurl=http://example.invalid/dummy\n" "enabled=1\nskip_if_unavailable=1\n", - ensure=True, ) repos = list(Repo.from_yum_config()) From cc2f584b523eb2408893382b82a3969431c94023 Mon Sep 17 00:00:00 2001 From: Jiri Popelka Date: Fri, 29 Sep 2023 11:11:00 +0200 Subject: [PATCH 17/18] Move tests to tests/ --- docs/CHANGES.rst | 1 + plans/acceptance.fmf | 2 +- plans/unit.fmf | 2 +- rpmdeplint.spec | 2 +- {acceptance_tests => tests/acceptance}/conftest.py | 0 .../acceptance}/data/b-0.1-1.i386.rpm | Bin .../acceptance}/data_setup.py | 0 .../acceptance}/test_check.py | 0 .../acceptance}/test_check_conflicts.py | 0 .../acceptance}/test_check_repoclosure.py | 0 .../acceptance}/test_check_sat.py | 0 .../acceptance}/test_check_upgrade.py | 0 .../acceptance}/test_list_deps.py | 0 .../acceptance}/test_usage.py | 0 {rpmdeplint/tests => tests/unit}/__init__.py | 0 .../unit}/test_dependency_analyzer.py | 0 .../tests => tests/unit}/test_dependency_set.py | 0 {rpmdeplint/tests => tests/unit}/test_repodata.py | 0 18 files changed, 4 insertions(+), 3 deletions(-) rename {acceptance_tests => tests/acceptance}/conftest.py (100%) rename {acceptance_tests => tests/acceptance}/data/b-0.1-1.i386.rpm (100%) rename {acceptance_tests => tests/acceptance}/data_setup.py (100%) rename {acceptance_tests => tests/acceptance}/test_check.py (100%) rename {acceptance_tests => tests/acceptance}/test_check_conflicts.py (100%) rename {acceptance_tests => tests/acceptance}/test_check_repoclosure.py (100%) rename {acceptance_tests => tests/acceptance}/test_check_sat.py (100%) rename {acceptance_tests => tests/acceptance}/test_check_upgrade.py (100%) rename {acceptance_tests => tests/acceptance}/test_list_deps.py (100%) rename {acceptance_tests => tests/acceptance}/test_usage.py (100%) rename {rpmdeplint/tests => tests/unit}/__init__.py (100%) rename {rpmdeplint/tests => tests/unit}/test_dependency_analyzer.py (100%) rename {rpmdeplint/tests => tests/unit}/test_dependency_set.py (100%) rename {rpmdeplint/tests => tests/unit}/test_repodata.py (100%) diff --git a/docs/CHANGES.rst b/docs/CHANGES.rst index 61fed52..0116bd2 100644 --- a/docs/CHANGES.rst +++ b/docs/CHANGES.rst @@ -13,6 +13,7 @@ Changelog * Allow passing only a configured repo name for ``--repo``. * Modernize deprecated stuff. * Refactoring. +* Unit and acceptance tests moved to :file:`tests/`. 2.0rc2 ~~~~~~ diff --git a/plans/acceptance.fmf b/plans/acceptance.fmf index 63f0658..84bceda 100644 --- a/plans/acceptance.fmf +++ b/plans/acceptance.fmf @@ -10,4 +10,4 @@ prepare: - 'glibc-devel*i686' execute: script: - - pytest --verbose --showlocals acceptance_tests/ + - pytest --verbose --showlocals tests/acceptance/ diff --git a/plans/unit.fmf b/plans/unit.fmf index 4eb04af..1006823 100644 --- a/plans/unit.fmf +++ b/plans/unit.fmf @@ -7,4 +7,4 @@ prepare: - python3-rpmfluff execute: script: - - pytest --verbose --showlocals rpmdeplint/ + - pytest --verbose --showlocals tests/unit/ diff --git a/rpmdeplint.spec b/rpmdeplint.spec index be935a1..9bad31d 100644 --- a/rpmdeplint.spec +++ b/rpmdeplint.spec @@ -54,7 +54,7 @@ mv docs/_build/man/rpmdeplint.1 %{buildroot}%{_mandir}/man1/ %pyproject_save_files rpmdeplint %check -py.test-3 rpmdeplint -k "not TestDependencyAnalyzer" +%pytest tests/unit/ -k "not TestDependencyAnalyzer" # Acceptance tests do not work in mock because they require .i686 packages. %files diff --git a/acceptance_tests/conftest.py b/tests/acceptance/conftest.py similarity index 100% rename from acceptance_tests/conftest.py rename to tests/acceptance/conftest.py diff --git a/acceptance_tests/data/b-0.1-1.i386.rpm b/tests/acceptance/data/b-0.1-1.i386.rpm similarity index 100% rename from acceptance_tests/data/b-0.1-1.i386.rpm rename to tests/acceptance/data/b-0.1-1.i386.rpm diff --git a/acceptance_tests/data_setup.py b/tests/acceptance/data_setup.py similarity index 100% rename from acceptance_tests/data_setup.py rename to tests/acceptance/data_setup.py diff --git a/acceptance_tests/test_check.py b/tests/acceptance/test_check.py similarity index 100% rename from acceptance_tests/test_check.py rename to tests/acceptance/test_check.py diff --git a/acceptance_tests/test_check_conflicts.py b/tests/acceptance/test_check_conflicts.py similarity index 100% rename from acceptance_tests/test_check_conflicts.py rename to tests/acceptance/test_check_conflicts.py diff --git a/acceptance_tests/test_check_repoclosure.py b/tests/acceptance/test_check_repoclosure.py similarity index 100% rename from acceptance_tests/test_check_repoclosure.py rename to tests/acceptance/test_check_repoclosure.py diff --git a/acceptance_tests/test_check_sat.py b/tests/acceptance/test_check_sat.py similarity index 100% rename from acceptance_tests/test_check_sat.py rename to tests/acceptance/test_check_sat.py diff --git a/acceptance_tests/test_check_upgrade.py b/tests/acceptance/test_check_upgrade.py similarity index 100% rename from acceptance_tests/test_check_upgrade.py rename to tests/acceptance/test_check_upgrade.py diff --git a/acceptance_tests/test_list_deps.py b/tests/acceptance/test_list_deps.py similarity index 100% rename from acceptance_tests/test_list_deps.py rename to tests/acceptance/test_list_deps.py diff --git a/acceptance_tests/test_usage.py b/tests/acceptance/test_usage.py similarity index 100% rename from acceptance_tests/test_usage.py rename to tests/acceptance/test_usage.py diff --git a/rpmdeplint/tests/__init__.py b/tests/unit/__init__.py similarity index 100% rename from rpmdeplint/tests/__init__.py rename to tests/unit/__init__.py diff --git a/rpmdeplint/tests/test_dependency_analyzer.py b/tests/unit/test_dependency_analyzer.py similarity index 100% rename from rpmdeplint/tests/test_dependency_analyzer.py rename to tests/unit/test_dependency_analyzer.py diff --git a/rpmdeplint/tests/test_dependency_set.py b/tests/unit/test_dependency_set.py similarity index 100% rename from rpmdeplint/tests/test_dependency_set.py rename to tests/unit/test_dependency_set.py diff --git a/rpmdeplint/tests/test_repodata.py b/tests/unit/test_repodata.py similarity index 100% rename from rpmdeplint/tests/test_repodata.py rename to tests/unit/test_repodata.py From 0566086c17252b04c68ad285fdfc7d821257c31e Mon Sep 17 00:00:00 2001 From: Jiri Popelka Date: Fri, 29 Sep 2023 11:52:37 +0200 Subject: [PATCH 18/18] Move code from __init__.py to analyzer.py --- docs/CHANGES.rst | 1 + rpmdeplint/__init__.py | 503 +----------------------- rpmdeplint/analyzer.py | 504 +++++++++++++++++++++++++ rpmdeplint/cli.py | 10 +- tests/unit/test_dependency_analyzer.py | 2 +- tests/unit/test_dependency_set.py | 2 +- 6 files changed, 522 insertions(+), 500 deletions(-) create mode 100644 rpmdeplint/analyzer.py diff --git a/docs/CHANGES.rst b/docs/CHANGES.rst index 0116bd2..d62bcd1 100644 --- a/docs/CHANGES.rst +++ b/docs/CHANGES.rst @@ -14,6 +14,7 @@ Changelog * Modernize deprecated stuff. * Refactoring. * Unit and acceptance tests moved to :file:`tests/`. +* Code from :file:`__init__.py` moved to :file:`analyzer.py`. 2.0rc2 ~~~~~~ diff --git a/rpmdeplint/__init__.py b/rpmdeplint/__init__.py index 9348d0d..7bef063 100644 --- a/rpmdeplint/__init__.py +++ b/rpmdeplint/__init__.py @@ -4,501 +4,18 @@ # (at your option) any later version. -from collections import defaultdict -from collections.abc import Iterable -from logging import getLogger -from typing import Optional +from contextlib import suppress +from importlib.metadata import PackageNotFoundError, version -import rpm -from solv import ( - Dataiterator, - Job, - Pool, - Problem, - Selection, - XSolvable, - xfopen_fd, -) -from solv import ( - Repo as SolvRepo, -) +from rpmdeplint.analyzer import DependencyAnalyzer, DependencySet +from rpmdeplint.repodata import Repo -logger = getLogger(__name__) +with suppress(PackageNotFoundError): + __version__ = version("rpmdeplint") -installonlypkgs = [ - # The default 'installonlypkgs' from dnf - # https://github.com/rpm-software-management/dnf/blob/dnf-2.5.1-1/dnf/const.py.in#L28 - "kernel", - "kernel-PAE", - "installonlypkg(kernel)", - "installonlypkg(kernel-module)", - "installonlypkg(vm)", - # Additional names which yum 3.4.3 (RHEL7) has in its default 'installonlypkgs' - # https://github.com/rpm-software-management/yum/blob/cf8a5669165e958d56157abf40d0cdd552c8fbf9/yum/config.py#L650 - "kernel-bigmem", - "kernel-enterprise", - "kernel-smp", - "kernel-modules", - "kernel-debug", - "kernel-unsupported", - "kernel-source", - "kernel-devel", - "kernel-PAE-debug", +__all__ = [ + DependencyAnalyzer.__name__, + DependencySet.__name__, + Repo.__name__, ] - - -class UnreadablePackageError(Exception): - """ - Raised if an RPM package cannot be read from disk (it's corrupted, or the - file is not a valid RPM package, etc). - """ - - -class DependencySet: - """ - Contains dependency information from trying to install the packages under test. - """ - - def __init__(self) -> None: - self._packagedeps: defaultdict = defaultdict( - lambda: {"dependencies": [], "problems": []} - ) - self._packages_with_problems: set[str] = set() - self._overall_problems: set[str] = set() - - def add_package( - self, pkg: XSolvable, dependencies: Iterable, problems: list[Problem] - ): - nevra: str = str(pkg) - self._packagedeps[nevra]["dependencies"].extend(map(str, dependencies)) - if problems: - all_problems = [] - # For each problem, find all the problematic RPM "rules" which - # lead to the problem and also include them in - # the `overall_problems` description. - for problem in problems: - all_problems.append(str(problem)) - all_problems.extend( - rule.info().problemstr() for rule in problem.findallproblemrules() - ) - self._packagedeps[nevra]["problems"].extend(all_problems) - self._packages_with_problems.add(nevra) - self._overall_problems.update(all_problems) - - @property - def is_ok(self) -> bool: - return len(self._overall_problems) == 0 - - @property - def packages(self) -> list[str]: - return sorted(self._packagedeps.keys()) - - @property - def overall_problems(self) -> list[str]: - """ - List of str dependency problems found (if any) - """ - return sorted(self._overall_problems) - - @property - def packages_with_problems(self) -> list[str]: - """ - List of :py:class:`solv.Solvable` which had dependency problems - """ - return sorted(self._packages_with_problems) - - @property - def package_dependencies(self) -> dict[str, dict[str, list]]: - """ - Dict in the form {package: {'dependencies': list of packages, 'problems': list of problems}} - """ - return dict(self._packagedeps) - - -class DependencyAnalyzer: - """An object which checks packages against provided repos - for dependency satisfiability. - - Construct an instance for a particular set of packages you want to test, - with the repos you want to test against. Then call the individual checking - methods to perform each check. - """ - - def __init__( - self, - repos: Iterable, - packages: Iterable[str], - arch: Optional[str] = None, - ): - """ - :param repos: An iterable of :py:class:`rpmdeplint.repodata.Repo` instances - :param packages: An iterable of RPM package paths to be tested - """ - # delayed import to avoid circular dependency - from rpmdeplint.repodata import RepoDownloadError - - self.pool = Pool() - self.pool.setarch(arch) - - #: List of :py:class:`solv.Solvable` to be tested (corresponding to *packages* parameter) - self.solvables: list[XSolvable] = [] - self.commandline_repo = self.pool.add_repo("@commandline") - for rpmpath in packages: - solvable = self.commandline_repo.add_rpm(rpmpath) - if solvable is None: - # pool.errstr is already prefixed with the filename - raise UnreadablePackageError( - f"Failed to read package: {self.pool.errstr}" - ) - self.solvables.append(solvable) - - self.repos_by_name = ( - {} - ) #: Mapping of {repo name: :py:class:`rpmdeplint.repodata.Repo`} - for repo in repos: - try: - repo.download_repodata() - except RepoDownloadError as e: - if repo.skip_if_unavailable: - logger.warning("Skipping repo %s: %s", repo.name, e) - continue - raise - solv_repo = self.pool.add_repo(repo.name) - # solv.xfopen does not accept unicode filenames on Python 2 - solv_repo.add_rpmmd( - xfopen_fd(repo.primary_urls[0], repo.primary.fileno()), None - ) - solv_repo.add_rpmmd( - xfopen_fd(repo.filelists_urls[0], repo.filelists.fileno()), - None, - SolvRepo.REPO_EXTEND_SOLVABLES, - ) - self.repos_by_name[repo.name] = repo - - self.pool.addfileprovides() - self.pool.createwhatprovides() - - # Special handling for "installonly" packages: we create jobs to mark - # installonly package names as "multiversion" and then set those as - # pool jobs, which means the jobs are automatically applied whenever we - # run the solver on this pool. - multiversion_jobs = [] - for name in installonlypkgs: - selection = self.pool.select(name, Selection.SELECTION_PROVIDES) - multiversion_jobs.extend(selection.jobs(Job.SOLVER_MULTIVERSION)) - self.pool.setpooljobs(multiversion_jobs) - - # Context manager protocol is only implemented for backwards compatibility. - # There are actually no resources to acquire or release. - - def __enter__(self): - return self - - def __exit__(self, type, value, tb): - return - - def download_package_header(self, solvable: XSolvable) -> str: - if solvable in self.solvables: - # It's a package under test, nothing to download - return solvable.lookup_location()[0] - href = solvable.lookup_location()[0] - baseurl = solvable.lookup_str(self.pool.str2id("solvable:mediabase")) - repo = self.repos_by_name[solvable.repo.name] - return repo.download_package_header(href, baseurl) - - def try_to_install_all(self) -> DependencySet: - """ - Try to solve the goal of installing each of the packages under test, - starting from an empty package set. - - :return: dependency set - """ - solver = self.pool.Solver() - ds = DependencySet() - for solvable in self.solvables: - logger.debug("Solving install jobs for %s", solvable) - jobs = solvable.Selection().jobs(Job.SOLVER_INSTALL) - if problems := solver.solve(jobs): - ds.add_package(solvable, [], problems) - else: - ds.add_package(solvable, solver.transaction().newsolvables(), []) - return ds - - def _select_obsoleted_by(self, solvables: Iterable[XSolvable]) -> Selection: - """ - Returns a Selection matching every solvable which is "obsoleted" - by some solvable in the given list -- either due to an explicit - Obsoletes relationship, or because we have a solvable with the same - name with a higher epoch-version-release. - """ - # Start with an empty selection. - sel = self.pool.Selection() - for solvable in solvables: - # Select every solvable with the same name and lower EVR. - # XXX are there some special cases with arch-noarch upgrades which this does not handle? - sel.add( - self.pool.select( - f"{solvable.name}.{solvable.arch} < {solvable.evr}", - Selection.SELECTION_NAME - | Selection.SELECTION_DOTARCH - | Selection.SELECTION_REL, - ) - ) - for obsoletes_rel in solvable.lookup_deparray( - self.pool.str2id("solvable:obsoletes") - ): - # Select every solvable matching the obsoletes relationship by name. - sel.add(obsoletes_rel.Selection_name()) - return sel - - def find_repoclosure_problems(self) -> list[str]: - """ - Checks for any package in the repos which would have unsatisfied - dependencies, if the packages under test were added to the repos. - - This applies some extra constraints to prevent the solver from finding - a solution which involves downgrading or installing an older package, - which is technically a valid solution but is not expected if the - packages are supposed to be updates. - - :return: List of str problem descriptions if any problems were found - """ - problems = [] - solver = self.pool.Solver() - # This selection matches packages obsoleted by our packages under test. - obs_sel = self._select_obsoleted_by(self.solvables) - # This selection matches packages obsoleted by other existing packages in the repo. - existing_obs_sel = self._select_obsoleted_by( - s for s in self.pool.solvables if s.repo.name != "@commandline" - ) - obsoleted = obs_sel.solvables() + existing_obs_sel.solvables() - logger.debug( - "Excluding the following obsoleted packages:\n%s", - "\n".join(f" {s}" for s in obsoleted), - ) - for solvable in self.pool.solvables: - if solvable in self.solvables: - continue # checked by check-sat command instead - if solvable in obsoleted: - continue # no reason to check it - if not self.pool.isknownarch(solvable.archid): - logger.debug( - "Skipping requirements for package %s arch does not match " - "Architecture under test", - str(solvable), - ) - continue - logger.debug("Checking requires for %s", solvable) - # XXX limit available packages to compatible arches? - # (use libsolv archpolicies somehow) - jobs = ( - solvable.Selection().jobs(Job.SOLVER_INSTALL) - + obs_sel.jobs(Job.SOLVER_ERASE) - + existing_obs_sel.jobs(Job.SOLVER_ERASE) - ) - if solver_problems := solver.solve(jobs): - problem_msgs = [str(p) for p in solver_problems] - # If it's a pre-existing problem with repos (that is, the - # problem also exists when the packages under test are - # excluded) then warn about it here but don't consider it - # a problem. - jobs = solvable.Selection().jobs( - Job.SOLVER_INSTALL - ) + existing_obs_sel.jobs(Job.SOLVER_ERASE) - if existing_problems := solver.solve(jobs): - for p in existing_problems: - logger.warning( - "Ignoring pre-existing repoclosure problem: %s", p - ) - else: - problems.extend(problem_msgs) - return problems - - def _files_in_solvable(self, solvable: XSolvable) -> set[str]: - iterator = solvable.Dataiterator( - self.pool.str2id("solvable:filelist"), - None, - Dataiterator.SEARCH_FILES | Dataiterator.SEARCH_COMPLETE_FILELIST, - ) - return {match.str for match in iterator} - - def _packages_can_be_installed_together( - self, left: XSolvable, right: XSolvable - ) -> bool: - """ - Returns True if the given packages can be installed together. - """ - solver = self.pool.Solver() - left_install_jobs = left.Selection().jobs(Job.SOLVER_INSTALL) - right_install_jobs = right.Selection().jobs(Job.SOLVER_INSTALL) - # First check if each one can be installed on its own. If either of - # these fails it is a warning, because it means we have no way to know - # if they can be installed together or not. - if problems := solver.solve(left_install_jobs): - logger.warning( - "Ignoring conflict candidate %s " - "with pre-existing dependency problems: %s", - left, - problems[0], - ) - return False - if problems := solver.solve(right_install_jobs): - logger.warning( - "Ignoring conflict candidate %s " - "with pre-existing dependency problems: %s", - right, - problems[0], - ) - return False - if problems := solver.solve(left_install_jobs + right_install_jobs): - logger.debug( - "Conflict candidates %s and %s cannot be installed together: %s", - left, - right, - problems[0], - ) - return False - return True - - def _file_conflict_is_permitted( - self, left: XSolvable, right: XSolvable, filename: str - ) -> bool: - """ - Returns True if rpm would allow both the given packages to share - ownership of the given filename. - """ - ts = rpm.TransactionSet() - ts.setVSFlags(rpm._RPMVSF_NOSIGNATURES) - - left_hdr = ts.hdrFromFdno(open(left.lookup_location()[0], "rb")) - right_hdr = ts.hdrFromFdno(open(self.download_package_header(right), "rb")) - left_files = rpm.files(left_hdr) - right_files = rpm.files(right_hdr) - if left_files[filename].matches(right_files[filename]): - logger.debug( - "Conflict on %s between %s and %s permitted because files match", - filename, - left, - right, - ) - return True - if left_files[filename].color != right_files[filename].color: - logger.debug( - "Conflict on %s between %s and %s permitted because colors differ", - filename, - left, - right, - ) - return True - return False - - def find_conflicts(self) -> list[str]: - """ - Find undeclared file conflicts in the packages under test. - - :return: List of str describing each conflict found - (or empty list if no conflicts were found) - """ - # solver = self.pool.Solver() - problems = [] - for solvable in self.solvables: - logger.debug("Checking all files in %s for conflicts", solvable) - filenames = self._files_in_solvable(solvable) - # In libsolv, iterating all solvables is fast, and listing all - # files in a solvable is fast, but finding solvables which contain - # a given file is *very slow* (see bug 1465736). - # Hence this approach, where we visit each solvable and use Python - # set operations to look for any overlapping filenames. - for conflicting in self.pool.solvables: - # Conflicts cannot happen between identical solvables and also - # between solvables with the same name - such solvables cannot - # be installed next to each other. - if conflicting == solvable or conflicting.name == solvable.name: - continue - conflict_filenames = filenames.intersection( - self._files_in_solvable(conflicting) - ) - if not conflict_filenames: - continue - if not self._packages_can_be_installed_together(solvable, conflicting): - continue - for filename in conflict_filenames: - logger.debug( - "Considering conflict on %s with %s", filename, conflicting - ) - if not self._file_conflict_is_permitted( - solvable, conflicting, filename - ): - msg = ( - f"{solvable} provides {filename} " - f"which is also provided by {conflicting}" - ) - problems.append(msg) - if conflicting not in self.solvables: - # For each filename we are checking, we only want to - # check at most *one* package from the remote - # repositories. This is purely an optimization to save - # network bandwidth and time. We *are* potentially - # missing some real conflicts by doing this, but the - # cost of downloading every package in the distro for - # common directories like /usr/lib/debug is too high. - # Note however that we do always ensure at least one - # *remote* candidate is checked (that is, not from the - # set of packages under test) to catch problems like - # bug 1502458. - logger.debug( - "Skipping further checks on %s " - "to save network bandwidth", - filename, - ) - filenames.remove(filename) - return sorted(problems) - - def find_upgrade_problems(self) -> list[str]: - """ - Checks for any package in the repos which would upgrade or obsolete the - packages under test. - - :return: List of str describing each upgrade problem found (or - empty list if no problems were found) - """ - # Pretend the packages under test are installed, then solve a distupgrade. - # If any package under test would be erased, then it means some other - # package in the repos is better than it, and we have a problem. - self.pool.installed = self.commandline_repo - try: - jobs = self.pool.Selection_all().jobs(Job.SOLVER_UPDATE) - solver = self.pool.Solver() - solver.set_flag(solver.SOLVER_FLAG_ALLOW_UNINSTALL, True) - solver_problems = solver.solve(jobs) - for problem in solver_problems: - # This is a warning, not an error, because it means there are - # some *other* problems with existing packages in the - # repository, not our packages under test. But it means our - # results here might not be valid. - logger.warning( - "Upgrade candidate has pre-existing dependency problem: %s", problem - ) - transaction = solver.transaction() - problems = [] - for solvable in self.solvables: - action = transaction.steptype( - solvable, transaction.SOLVER_TRANSACTION_SHOW_OBSOLETES - ) - other = transaction.othersolvable(solvable) - if action == transaction.SOLVER_TRANSACTION_IGNORE: - continue # it's kept, so no problem here - if action == transaction.SOLVER_TRANSACTION_UPGRADED: - problems.append( - f"{solvable} would be upgraded by {other} from repo {other.repo.name}" - ) - elif action == transaction.SOLVER_TRANSACTION_OBSOLETED: - problems.append( - f"{solvable} would be obsoleted by {other} from repo {other.repo.name}" - ) - else: - raise RuntimeError(f"Unrecognised transaction step type {action}") - return problems - finally: - self.pool.installed = None diff --git a/rpmdeplint/analyzer.py b/rpmdeplint/analyzer.py new file mode 100644 index 0000000..9348d0d --- /dev/null +++ b/rpmdeplint/analyzer.py @@ -0,0 +1,504 @@ +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. + + +from collections import defaultdict +from collections.abc import Iterable +from logging import getLogger +from typing import Optional + +import rpm +from solv import ( + Dataiterator, + Job, + Pool, + Problem, + Selection, + XSolvable, + xfopen_fd, +) +from solv import ( + Repo as SolvRepo, +) + +logger = getLogger(__name__) + + +installonlypkgs = [ + # The default 'installonlypkgs' from dnf + # https://github.com/rpm-software-management/dnf/blob/dnf-2.5.1-1/dnf/const.py.in#L28 + "kernel", + "kernel-PAE", + "installonlypkg(kernel)", + "installonlypkg(kernel-module)", + "installonlypkg(vm)", + # Additional names which yum 3.4.3 (RHEL7) has in its default 'installonlypkgs' + # https://github.com/rpm-software-management/yum/blob/cf8a5669165e958d56157abf40d0cdd552c8fbf9/yum/config.py#L650 + "kernel-bigmem", + "kernel-enterprise", + "kernel-smp", + "kernel-modules", + "kernel-debug", + "kernel-unsupported", + "kernel-source", + "kernel-devel", + "kernel-PAE-debug", +] + + +class UnreadablePackageError(Exception): + """ + Raised if an RPM package cannot be read from disk (it's corrupted, or the + file is not a valid RPM package, etc). + """ + + +class DependencySet: + """ + Contains dependency information from trying to install the packages under test. + """ + + def __init__(self) -> None: + self._packagedeps: defaultdict = defaultdict( + lambda: {"dependencies": [], "problems": []} + ) + self._packages_with_problems: set[str] = set() + self._overall_problems: set[str] = set() + + def add_package( + self, pkg: XSolvable, dependencies: Iterable, problems: list[Problem] + ): + nevra: str = str(pkg) + self._packagedeps[nevra]["dependencies"].extend(map(str, dependencies)) + if problems: + all_problems = [] + # For each problem, find all the problematic RPM "rules" which + # lead to the problem and also include them in + # the `overall_problems` description. + for problem in problems: + all_problems.append(str(problem)) + all_problems.extend( + rule.info().problemstr() for rule in problem.findallproblemrules() + ) + self._packagedeps[nevra]["problems"].extend(all_problems) + self._packages_with_problems.add(nevra) + self._overall_problems.update(all_problems) + + @property + def is_ok(self) -> bool: + return len(self._overall_problems) == 0 + + @property + def packages(self) -> list[str]: + return sorted(self._packagedeps.keys()) + + @property + def overall_problems(self) -> list[str]: + """ + List of str dependency problems found (if any) + """ + return sorted(self._overall_problems) + + @property + def packages_with_problems(self) -> list[str]: + """ + List of :py:class:`solv.Solvable` which had dependency problems + """ + return sorted(self._packages_with_problems) + + @property + def package_dependencies(self) -> dict[str, dict[str, list]]: + """ + Dict in the form {package: {'dependencies': list of packages, 'problems': list of problems}} + """ + return dict(self._packagedeps) + + +class DependencyAnalyzer: + """An object which checks packages against provided repos + for dependency satisfiability. + + Construct an instance for a particular set of packages you want to test, + with the repos you want to test against. Then call the individual checking + methods to perform each check. + """ + + def __init__( + self, + repos: Iterable, + packages: Iterable[str], + arch: Optional[str] = None, + ): + """ + :param repos: An iterable of :py:class:`rpmdeplint.repodata.Repo` instances + :param packages: An iterable of RPM package paths to be tested + """ + # delayed import to avoid circular dependency + from rpmdeplint.repodata import RepoDownloadError + + self.pool = Pool() + self.pool.setarch(arch) + + #: List of :py:class:`solv.Solvable` to be tested (corresponding to *packages* parameter) + self.solvables: list[XSolvable] = [] + self.commandline_repo = self.pool.add_repo("@commandline") + for rpmpath in packages: + solvable = self.commandline_repo.add_rpm(rpmpath) + if solvable is None: + # pool.errstr is already prefixed with the filename + raise UnreadablePackageError( + f"Failed to read package: {self.pool.errstr}" + ) + self.solvables.append(solvable) + + self.repos_by_name = ( + {} + ) #: Mapping of {repo name: :py:class:`rpmdeplint.repodata.Repo`} + for repo in repos: + try: + repo.download_repodata() + except RepoDownloadError as e: + if repo.skip_if_unavailable: + logger.warning("Skipping repo %s: %s", repo.name, e) + continue + raise + solv_repo = self.pool.add_repo(repo.name) + # solv.xfopen does not accept unicode filenames on Python 2 + solv_repo.add_rpmmd( + xfopen_fd(repo.primary_urls[0], repo.primary.fileno()), None + ) + solv_repo.add_rpmmd( + xfopen_fd(repo.filelists_urls[0], repo.filelists.fileno()), + None, + SolvRepo.REPO_EXTEND_SOLVABLES, + ) + self.repos_by_name[repo.name] = repo + + self.pool.addfileprovides() + self.pool.createwhatprovides() + + # Special handling for "installonly" packages: we create jobs to mark + # installonly package names as "multiversion" and then set those as + # pool jobs, which means the jobs are automatically applied whenever we + # run the solver on this pool. + multiversion_jobs = [] + for name in installonlypkgs: + selection = self.pool.select(name, Selection.SELECTION_PROVIDES) + multiversion_jobs.extend(selection.jobs(Job.SOLVER_MULTIVERSION)) + self.pool.setpooljobs(multiversion_jobs) + + # Context manager protocol is only implemented for backwards compatibility. + # There are actually no resources to acquire or release. + + def __enter__(self): + return self + + def __exit__(self, type, value, tb): + return + + def download_package_header(self, solvable: XSolvable) -> str: + if solvable in self.solvables: + # It's a package under test, nothing to download + return solvable.lookup_location()[0] + href = solvable.lookup_location()[0] + baseurl = solvable.lookup_str(self.pool.str2id("solvable:mediabase")) + repo = self.repos_by_name[solvable.repo.name] + return repo.download_package_header(href, baseurl) + + def try_to_install_all(self) -> DependencySet: + """ + Try to solve the goal of installing each of the packages under test, + starting from an empty package set. + + :return: dependency set + """ + solver = self.pool.Solver() + ds = DependencySet() + for solvable in self.solvables: + logger.debug("Solving install jobs for %s", solvable) + jobs = solvable.Selection().jobs(Job.SOLVER_INSTALL) + if problems := solver.solve(jobs): + ds.add_package(solvable, [], problems) + else: + ds.add_package(solvable, solver.transaction().newsolvables(), []) + return ds + + def _select_obsoleted_by(self, solvables: Iterable[XSolvable]) -> Selection: + """ + Returns a Selection matching every solvable which is "obsoleted" + by some solvable in the given list -- either due to an explicit + Obsoletes relationship, or because we have a solvable with the same + name with a higher epoch-version-release. + """ + # Start with an empty selection. + sel = self.pool.Selection() + for solvable in solvables: + # Select every solvable with the same name and lower EVR. + # XXX are there some special cases with arch-noarch upgrades which this does not handle? + sel.add( + self.pool.select( + f"{solvable.name}.{solvable.arch} < {solvable.evr}", + Selection.SELECTION_NAME + | Selection.SELECTION_DOTARCH + | Selection.SELECTION_REL, + ) + ) + for obsoletes_rel in solvable.lookup_deparray( + self.pool.str2id("solvable:obsoletes") + ): + # Select every solvable matching the obsoletes relationship by name. + sel.add(obsoletes_rel.Selection_name()) + return sel + + def find_repoclosure_problems(self) -> list[str]: + """ + Checks for any package in the repos which would have unsatisfied + dependencies, if the packages under test were added to the repos. + + This applies some extra constraints to prevent the solver from finding + a solution which involves downgrading or installing an older package, + which is technically a valid solution but is not expected if the + packages are supposed to be updates. + + :return: List of str problem descriptions if any problems were found + """ + problems = [] + solver = self.pool.Solver() + # This selection matches packages obsoleted by our packages under test. + obs_sel = self._select_obsoleted_by(self.solvables) + # This selection matches packages obsoleted by other existing packages in the repo. + existing_obs_sel = self._select_obsoleted_by( + s for s in self.pool.solvables if s.repo.name != "@commandline" + ) + obsoleted = obs_sel.solvables() + existing_obs_sel.solvables() + logger.debug( + "Excluding the following obsoleted packages:\n%s", + "\n".join(f" {s}" for s in obsoleted), + ) + for solvable in self.pool.solvables: + if solvable in self.solvables: + continue # checked by check-sat command instead + if solvable in obsoleted: + continue # no reason to check it + if not self.pool.isknownarch(solvable.archid): + logger.debug( + "Skipping requirements for package %s arch does not match " + "Architecture under test", + str(solvable), + ) + continue + logger.debug("Checking requires for %s", solvable) + # XXX limit available packages to compatible arches? + # (use libsolv archpolicies somehow) + jobs = ( + solvable.Selection().jobs(Job.SOLVER_INSTALL) + + obs_sel.jobs(Job.SOLVER_ERASE) + + existing_obs_sel.jobs(Job.SOLVER_ERASE) + ) + if solver_problems := solver.solve(jobs): + problem_msgs = [str(p) for p in solver_problems] + # If it's a pre-existing problem with repos (that is, the + # problem also exists when the packages under test are + # excluded) then warn about it here but don't consider it + # a problem. + jobs = solvable.Selection().jobs( + Job.SOLVER_INSTALL + ) + existing_obs_sel.jobs(Job.SOLVER_ERASE) + if existing_problems := solver.solve(jobs): + for p in existing_problems: + logger.warning( + "Ignoring pre-existing repoclosure problem: %s", p + ) + else: + problems.extend(problem_msgs) + return problems + + def _files_in_solvable(self, solvable: XSolvable) -> set[str]: + iterator = solvable.Dataiterator( + self.pool.str2id("solvable:filelist"), + None, + Dataiterator.SEARCH_FILES | Dataiterator.SEARCH_COMPLETE_FILELIST, + ) + return {match.str for match in iterator} + + def _packages_can_be_installed_together( + self, left: XSolvable, right: XSolvable + ) -> bool: + """ + Returns True if the given packages can be installed together. + """ + solver = self.pool.Solver() + left_install_jobs = left.Selection().jobs(Job.SOLVER_INSTALL) + right_install_jobs = right.Selection().jobs(Job.SOLVER_INSTALL) + # First check if each one can be installed on its own. If either of + # these fails it is a warning, because it means we have no way to know + # if they can be installed together or not. + if problems := solver.solve(left_install_jobs): + logger.warning( + "Ignoring conflict candidate %s " + "with pre-existing dependency problems: %s", + left, + problems[0], + ) + return False + if problems := solver.solve(right_install_jobs): + logger.warning( + "Ignoring conflict candidate %s " + "with pre-existing dependency problems: %s", + right, + problems[0], + ) + return False + if problems := solver.solve(left_install_jobs + right_install_jobs): + logger.debug( + "Conflict candidates %s and %s cannot be installed together: %s", + left, + right, + problems[0], + ) + return False + return True + + def _file_conflict_is_permitted( + self, left: XSolvable, right: XSolvable, filename: str + ) -> bool: + """ + Returns True if rpm would allow both the given packages to share + ownership of the given filename. + """ + ts = rpm.TransactionSet() + ts.setVSFlags(rpm._RPMVSF_NOSIGNATURES) + + left_hdr = ts.hdrFromFdno(open(left.lookup_location()[0], "rb")) + right_hdr = ts.hdrFromFdno(open(self.download_package_header(right), "rb")) + left_files = rpm.files(left_hdr) + right_files = rpm.files(right_hdr) + if left_files[filename].matches(right_files[filename]): + logger.debug( + "Conflict on %s between %s and %s permitted because files match", + filename, + left, + right, + ) + return True + if left_files[filename].color != right_files[filename].color: + logger.debug( + "Conflict on %s between %s and %s permitted because colors differ", + filename, + left, + right, + ) + return True + return False + + def find_conflicts(self) -> list[str]: + """ + Find undeclared file conflicts in the packages under test. + + :return: List of str describing each conflict found + (or empty list if no conflicts were found) + """ + # solver = self.pool.Solver() + problems = [] + for solvable in self.solvables: + logger.debug("Checking all files in %s for conflicts", solvable) + filenames = self._files_in_solvable(solvable) + # In libsolv, iterating all solvables is fast, and listing all + # files in a solvable is fast, but finding solvables which contain + # a given file is *very slow* (see bug 1465736). + # Hence this approach, where we visit each solvable and use Python + # set operations to look for any overlapping filenames. + for conflicting in self.pool.solvables: + # Conflicts cannot happen between identical solvables and also + # between solvables with the same name - such solvables cannot + # be installed next to each other. + if conflicting == solvable or conflicting.name == solvable.name: + continue + conflict_filenames = filenames.intersection( + self._files_in_solvable(conflicting) + ) + if not conflict_filenames: + continue + if not self._packages_can_be_installed_together(solvable, conflicting): + continue + for filename in conflict_filenames: + logger.debug( + "Considering conflict on %s with %s", filename, conflicting + ) + if not self._file_conflict_is_permitted( + solvable, conflicting, filename + ): + msg = ( + f"{solvable} provides {filename} " + f"which is also provided by {conflicting}" + ) + problems.append(msg) + if conflicting not in self.solvables: + # For each filename we are checking, we only want to + # check at most *one* package from the remote + # repositories. This is purely an optimization to save + # network bandwidth and time. We *are* potentially + # missing some real conflicts by doing this, but the + # cost of downloading every package in the distro for + # common directories like /usr/lib/debug is too high. + # Note however that we do always ensure at least one + # *remote* candidate is checked (that is, not from the + # set of packages under test) to catch problems like + # bug 1502458. + logger.debug( + "Skipping further checks on %s " + "to save network bandwidth", + filename, + ) + filenames.remove(filename) + return sorted(problems) + + def find_upgrade_problems(self) -> list[str]: + """ + Checks for any package in the repos which would upgrade or obsolete the + packages under test. + + :return: List of str describing each upgrade problem found (or + empty list if no problems were found) + """ + # Pretend the packages under test are installed, then solve a distupgrade. + # If any package under test would be erased, then it means some other + # package in the repos is better than it, and we have a problem. + self.pool.installed = self.commandline_repo + try: + jobs = self.pool.Selection_all().jobs(Job.SOLVER_UPDATE) + solver = self.pool.Solver() + solver.set_flag(solver.SOLVER_FLAG_ALLOW_UNINSTALL, True) + solver_problems = solver.solve(jobs) + for problem in solver_problems: + # This is a warning, not an error, because it means there are + # some *other* problems with existing packages in the + # repository, not our packages under test. But it means our + # results here might not be valid. + logger.warning( + "Upgrade candidate has pre-existing dependency problem: %s", problem + ) + transaction = solver.transaction() + problems = [] + for solvable in self.solvables: + action = transaction.steptype( + solvable, transaction.SOLVER_TRANSACTION_SHOW_OBSOLETES + ) + other = transaction.othersolvable(solvable) + if action == transaction.SOLVER_TRANSACTION_IGNORE: + continue # it's kept, so no problem here + if action == transaction.SOLVER_TRANSACTION_UPGRADED: + problems.append( + f"{solvable} would be upgraded by {other} from repo {other.repo.name}" + ) + elif action == transaction.SOLVER_TRANSACTION_OBSOLETED: + problems.append( + f"{solvable} would be obsoleted by {other} from repo {other.repo.name}" + ) + else: + raise RuntimeError(f"Unrecognised transaction step type {action}") + return problems + finally: + self.pool.installed = None diff --git a/rpmdeplint/cli.py b/rpmdeplint/cli.py index ef1c5e8..a1b474d 100644 --- a/rpmdeplint/cli.py +++ b/rpmdeplint/cli.py @@ -9,15 +9,13 @@ import sys from collections.abc import Callable from enum import IntEnum -from importlib import metadata -from rpmdeplint import DependencyAnalyzer, UnreadablePackageError +from rpmdeplint import __version__ +from rpmdeplint.analyzer import DependencyAnalyzer, UnreadablePackageError from rpmdeplint.repodata import PackageDownloadError, Repo, RepoDownloadError logger = logging.getLogger(__name__) -version = metadata.version("rpmdeplint") - class ExitCode(IntEnum): OK = 0 @@ -216,7 +214,9 @@ def add_subparser( "--debug", action="store_true", help="Show detailed progress messages" ) parser.add_argument("--quiet", action="store_true", help="Show only errors") - parser.add_argument("--version", action="version", version=f"%(prog)s {version}") + parser.add_argument( + "--version", action="version", version=f"%(prog)s {__version__}" + ) subparsers = parser.add_subparsers(dest="subcommand", title="subcommands") subparsers.required = True diff --git a/tests/unit/test_dependency_analyzer.py b/tests/unit/test_dependency_analyzer.py index b5b6625..2618b3c 100644 --- a/tests/unit/test_dependency_analyzer.py +++ b/tests/unit/test_dependency_analyzer.py @@ -8,7 +8,7 @@ from rpmfluff import SimpleRpmBuild from rpmfluff.yumrepobuild import YumRepoBuild -from rpmdeplint import DependencyAnalyzer +from rpmdeplint.analyzer import DependencyAnalyzer from rpmdeplint.repodata import Repo diff --git a/tests/unit/test_dependency_set.py b/tests/unit/test_dependency_set.py index 8fc6b68..a673aac 100644 --- a/tests/unit/test_dependency_set.py +++ b/tests/unit/test_dependency_set.py @@ -5,7 +5,7 @@ from typing import ClassVar from unittest import TestCase -from rpmdeplint import DependencySet +from rpmdeplint.analyzer import DependencySet class test_pkg: