Conversation
Add DnfUpdater for Fedora/RHEL-based systems with: - Auto-detection of dnf5 vs dnf commands - DNF check-update parsing (handles exit code 100) - Progress tracking during downloads and installs - DnfUpgradeProgressTracker for real-time progress Integration: - Wire DNF into app.py concurrent update loop - Add DNF section to summary output table - Update tests for new _print_summary signature
- Add tests/test_dnf_updater.py with TestDnfUpdater class covering: - check_available: dnf5 preference, dnf4 fallback, none available - check_updates: output parsing, version extraction, empty/error handling - run_update: dry_run mode, progress callback, no updates case - _get_current_versions helper method - Add DNF fixtures to tests/conftest.py: - dnf_check_update_output, dnf_no_updates_output - dnf_upgrade_output with download/transaction phases - dnf_list_installed_output for version lookup - Add DNF parsing tests to tests/test_parsing.py: - TestParseDnfCheckOutput: parse_dnf_check_output function tests - TestDnfUpgradeProgressTracker: progress tracking through phases All 139 tests pass including 14 new DNF updater tests, 7 DNF parsing tests, and 11 DNF progress tracker tests.
- Remove unused in_installing_phase variable from dnf.py - Remove unused Package import from test_dnf_updater.py - Remove unused result variable assignment in test
Add aiohttp, pytest, and pytest-asyncio to dev dependencies. These were required but missing from pyproject.toml.
There was a problem hiding this comment.
Pull request overview
This pull request adds support for the DNF package manager to enable system updates on Fedora and other RPM-based distributions. The implementation includes DNF/DNF5 detection with automatic preference for the newer dnf5, package update checking, and progress tracking during downloads and installations.
Changes:
- Added complete DNF updater implementation with dnf5 fallback support
- Integrated DNF updater into the main application alongside APT, Flatpak, and Snap
- Added comprehensive test coverage for DNF operations and output parsing
Reviewed changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| sysupdate/updaters/dnf.py | New DNF updater implementation with check_updates and run_update methods |
| sysupdate/updaters/init.py | Exported DnfUpdater class |
| sysupdate/utils/parsing.py | Added parse_dnf_check_output function and DnfUpgradeProgressTracker class |
| sysupdate/app.py | Integrated DNF updater into concurrent update orchestration and summary display |
| tests/test_dnf_updater.py | New test file with comprehensive DNF updater tests |
| tests/test_parsing.py | Added tests for DNF parsing functions and progress tracker |
| tests/test_app.py | Updated _print_summary calls to include DNF packages parameter |
| tests/conftest.py | Added DNF output fixtures for testing |
| pyproject.toml | Added conflicting [dependency-groups] section |
| uv.lock | Updated with dependency changes |
| doc/loom/knowledge/*.md | Added documentation for DNF support patterns and architecture |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def parse_dnf_check_output(output: str) -> list[Package]: | ||
| """ | ||
| Parse DNF check-update output to extract package information. | ||
|
|
||
| Looks for patterns like: | ||
| - "package.arch version repository" | ||
|
|
||
| Args: | ||
| output: Raw DNF check-update output text | ||
|
|
||
| Returns: | ||
| List of Package objects | ||
| """ | ||
| # Import here to avoid circular dependency | ||
| from ..updaters.base import Package | ||
|
|
||
| packages: dict[str, Package] = {} | ||
|
|
||
| for line in output.splitlines(): | ||
| # Skip empty lines and metadata lines | ||
| if not line.strip() or line.startswith("Last metadata"): | ||
| continue | ||
|
|
||
| # Skip header separator lines | ||
| if line.strip().startswith("===") or line.strip().startswith("---"): | ||
| continue | ||
|
|
||
| # Check for package line format: package.arch version repository | ||
| match = _DNF_PKG_PATTERN.match(line) | ||
| if match: | ||
| name, arch, version, repository = match.groups() | ||
| # Store with original name (without arch), version is new_version | ||
| packages[name] = Package( | ||
| name=name, | ||
| new_version=version, | ||
| status="pending" | ||
| ) | ||
|
|
||
| return list(packages.values()) | ||
|
|
||
|
|
There was a problem hiding this comment.
The parse_dnf_check_output function is defined but never used in the DnfUpdater class. The check_updates method in dnf.py (lines 39-82) implements its own parsing logic instead of using this function. This deviates from the established pattern where AptUpdater uses parse_apt_output. Consider either using parse_dnf_check_output in the check_updates method, or removing it if it's not needed.
| def parse_dnf_check_output(output: str) -> list[Package]: | |
| """ | |
| Parse DNF check-update output to extract package information. | |
| Looks for patterns like: | |
| - "package.arch version repository" | |
| Args: | |
| output: Raw DNF check-update output text | |
| Returns: | |
| List of Package objects | |
| """ | |
| # Import here to avoid circular dependency | |
| from ..updaters.base import Package | |
| packages: dict[str, Package] = {} | |
| for line in output.splitlines(): | |
| # Skip empty lines and metadata lines | |
| if not line.strip() or line.startswith("Last metadata"): | |
| continue | |
| # Skip header separator lines | |
| if line.strip().startswith("===") or line.strip().startswith("---"): | |
| continue | |
| # Check for package line format: package.arch version repository | |
| match = _DNF_PKG_PATTERN.match(line) | |
| if match: | |
| name, arch, version, repository = match.groups() | |
| # Store with original name (without arch), version is new_version | |
| packages[name] = Package( | |
| name=name, | |
| new_version=version, | |
| status="pending" | |
| ) | |
| return list(packages.values()) |
| class DnfUpgradeProgressTracker: | ||
| """Tracks progress during dnf upgrade by parsing output lines. | ||
|
|
||
| This class encapsulates the state and logic for tracking DNF upgrade progress, | ||
| parsing lines to detect downloads and installation phases. | ||
|
|
||
| Progress is allocated as follows: | ||
| - Downloading: 0-50% | ||
| - Installing: 50-100% | ||
|
|
||
| Handles edge cases: | ||
| - Unknown total: Uses estimation based on seen package count | ||
| - Multiple packages in transaction: Tracks progress through download/install phases | ||
| """ | ||
|
|
||
| def __init__(self) -> None: | ||
| """Initialize the progress tracker.""" | ||
| self.total_packages = 0 | ||
| self.download_count = 0 | ||
| self.install_count = 0 | ||
| self.current_package = "" | ||
| self.last_progress = 0.0 | ||
| self._in_download_phase = False | ||
| self._in_install_phase = False | ||
| self._download_total = 0 | ||
|
|
||
| def parse_line(self, line: str) -> dict | None: | ||
| """Parse a line of dnf output and return progress info if applicable. | ||
|
|
||
| Args: | ||
| line: A single line of dnf output. | ||
|
|
||
| Returns: | ||
| Dict with keys: phase, progress, current_package, total_packages, | ||
| completed_packages, message. Returns None if no progress update. | ||
| """ | ||
| # Check for "Downloading Packages:" header | ||
| if "Downloading Packages:" in line: | ||
| self._in_download_phase = True | ||
| return { | ||
| "phase": "downloading", | ||
| "progress": 0.0, | ||
| "current_package": "", | ||
| "total_packages": self.total_packages, | ||
| "completed_packages": 0, | ||
| "message": "Starting download...", | ||
| } | ||
|
|
||
| # Check for download progress: (1/5): package-name | ||
| download_match = _DNF_DOWNLOAD_PATTERN.search(line) | ||
| if download_match and self._in_download_phase: | ||
| current = int(download_match.group(1)) | ||
| total = int(download_match.group(2)) | ||
| pkg_name = download_match.group(3) | ||
|
|
||
| # Extract package name from filename (e.g., package-1.0.rpm -> package) | ||
| if pkg_name.endswith(".rpm"): | ||
| pkg_name = pkg_name[:-4] | ||
| # Remove version info - split on first dash if present | ||
| if "-" in pkg_name: | ||
| pkg_name = pkg_name.rsplit("-", 2)[0] | ||
|
|
||
| self.download_count = current | ||
| self._download_total = total | ||
| self.total_packages = total | ||
| self.current_package = pkg_name | ||
|
|
||
| # Progress: downloading is 0-50% | ||
| progress = (current / total) * 0.5 | ||
| if progress > self.last_progress: | ||
| self.last_progress = progress | ||
| return { | ||
| "phase": "downloading", | ||
| "progress": progress, | ||
| "current_package": self.current_package, | ||
| "total_packages": self.total_packages, | ||
| "completed_packages": current, | ||
| } | ||
|
|
||
| # Check for install/upgrade phase - only "Running transaction", not "Upgrading" | ||
| # (Upgrading appears in transaction summary before downloads) | ||
| if "Running transaction" in line: | ||
| if not self._in_install_phase: | ||
| self._in_install_phase = True | ||
| # If we never tracked downloads, start at 50% | ||
| if self.last_progress < 0.5: | ||
| self.last_progress = 0.5 | ||
| return { | ||
| "phase": "installing", | ||
| "progress": 0.5, | ||
| "current_package": "", | ||
| "total_packages": self.total_packages, | ||
| "completed_packages": 0, | ||
| "message": "Installing packages...", | ||
| } | ||
|
|
||
| # Track individual package upgrades during transaction | ||
| # Format: " Upgrading : package-version.arch N/M" | ||
| upgrading_match = _DNF_UPGRADING_PATTERN.search(line) | ||
| if upgrading_match and self._in_install_phase: | ||
| pkg_name = upgrading_match.group(1) | ||
| # Remove version info from package name | ||
| if "-" in pkg_name: | ||
| pkg_name = pkg_name.rsplit("-", 2)[0] | ||
|
|
||
| self.install_count += 1 | ||
| self.current_package = pkg_name | ||
|
|
||
| if self.total_packages > 0: | ||
| # Progress: installing is 50-100% | ||
| progress = 0.5 + (self.install_count / self.total_packages) * 0.5 | ||
| if progress > self.last_progress: | ||
| self.last_progress = progress | ||
| return { | ||
| "phase": "installing", | ||
| "progress": progress, | ||
| "current_package": self.current_package, | ||
| "total_packages": self.total_packages, | ||
| "completed_packages": self.install_count, | ||
| } | ||
|
|
||
| # Check for completion line "Upgraded:" or "Installed:" | ||
| # This marks the summary at the end | ||
| if _DNF_COMPLETED_LINE_PATTERN.match(line): | ||
| # If we haven't reached 100% yet, do so now | ||
| if self.last_progress < 1.0: | ||
| self.last_progress = 0.99 | ||
| return { | ||
| "phase": "installing", | ||
| "progress": 0.99, | ||
| "current_package": "", | ||
| "total_packages": self.total_packages, | ||
| "completed_packages": self.total_packages, | ||
| "message": "Finalizing...", | ||
| } | ||
|
|
||
| # Check for completion | ||
| if "Complete!" in line: | ||
| return { | ||
| "phase": "complete", | ||
| "progress": 1.0, | ||
| "current_package": "", | ||
| "total_packages": self.total_packages, | ||
| "completed_packages": self.total_packages, | ||
| "message": "Update complete", | ||
| } | ||
|
|
||
| return None | ||
|
|
There was a problem hiding this comment.
The DnfUpgradeProgressTracker class is defined but never used in the DnfUpdater implementation. The _run_dnf_upgrade method (lines 189-386 in dnf.py) implements its own inline progress tracking instead. This deviates from the established pattern where AptUpdater uses AptUpgradeProgressTracker. Consider either using this tracker class or removing it to avoid maintaining dead code.
| # Store with original name (without arch), version is new_version | ||
| packages[name] = Package( | ||
| name=name, |
There was a problem hiding this comment.
Inconsistent package naming between parse_dnf_check_output and the DnfUpdater. The parse_dnf_check_output function extracts only the package name without architecture (line 363: packages[name] = ...), but the DnfUpdater.check_updates method keeps the full name with architecture (line 71: name = parts[0].strip()). The tests reflect this confusion: test_parsing.py expects names without arch (e.g., "kernel"), while test_dnf_updater.py expects names with arch (e.g., "kernel.x86_64"). This inconsistency will cause issues when matching packages across different operations.
| # Store with original name (without arch), version is new_version | |
| packages[name] = Package( | |
| name=name, | |
| # Store with full name including architecture (e.g., "kernel.x86_64") | |
| full_name = f"{name}.{arch}" | |
| packages[full_name] = Package( | |
| name=full_name, |
| [dependency-groups] | ||
| dev = [ | ||
| "aiohttp>=3.13.3", | ||
| "pytest>=9.0.2", | ||
| "pytest-asyncio>=1.3.0", | ||
| ] |
There was a problem hiding this comment.
The [dependency-groups] section duplicates and conflicts with [project.optional-dependencies]. Both define a 'dev' group with overlapping dependencies but different version constraints. For example, pytest is >=8.0.0 in optional-dependencies but >=9.0.2 in dependency-groups. Additionally, aiohttp is already a main dependency (line 24) so it shouldn't be in dev dependencies. Consider consolidating these into a single source of truth to avoid version conflicts and confusion.
| [dependency-groups] | |
| dev = [ | |
| "aiohttp>=3.13.3", | |
| "pytest>=9.0.2", | |
| "pytest-asyncio>=1.3.0", | |
| ] |
| if "Installing:" in line or "Upgrading:" in line: | ||
| in_downloading_phase = False | ||
| report(UpdateProgress( | ||
| phase=UpdatePhase.INSTALLING, | ||
| progress=0.5, | ||
| message="Installing packages...", | ||
| )) | ||
| continue |
There was a problem hiding this comment.
The condition "Installing:" in line or "Upgrading:" in line at line 291 will incorrectly match the transaction summary table header "Upgrading:" which appears before the download phase begins (see line 189 in the test fixture). This will cause the phase to be set to INSTALLING prematurely, before packages are downloaded. The correct marker for the installation phase is "Running transaction" which appears after downloads are complete (line 207 in test fixture). This is why DnfUpgradeProgressTracker in parsing.py correctly checks for "Running transaction" at line 453.
| # Progress calculation: use current package index and percentage | ||
| progress = (current_idx - 1 + pct / 100.0) / max(total_idx, 1) | ||
| # Scale to 0.1-0.5 (downloading phase) | ||
| progress = 0.1 + (progress * 0.4) | ||
|
|
||
| if progress > last_progress_report + 0.01: | ||
| last_progress_report = progress | ||
| report(UpdateProgress( | ||
| phase=UpdatePhase.DOWNLOADING, | ||
| progress=progress, | ||
| total_packages=total_packages, | ||
| completed_packages=completed, | ||
| current_package=current_package, | ||
| )) |
There was a problem hiding this comment.
Progress values are double-scaled. The code at lines 286 and 316 manually scales progress to the 0.1-0.5 range, but then scaled_callback (configured at lines 150-155 in run_update) scales DOWNLOADING phase from 0.1 to 1.0 again. This means a progress value of 0.1 becomes 0.19, and 0.5 becomes 0.55, not the intended 10-50% range. The inner code should report progress in 0.0-1.0 range and let scaled_callback handle all scaling, as done in the unused DnfUpgradeProgressTracker.
| progress = 0.5 + (completed / max(total_packages, 1)) * 0.5 | ||
| last_progress_report = max(last_progress_report, progress) | ||
| report(UpdateProgress( | ||
| phase=UpdatePhase.INSTALLING, | ||
| progress=progress, | ||
| total_packages=total_packages, | ||
| completed_packages=completed, | ||
| current_package=current_package, | ||
| )) |
There was a problem hiding this comment.
Progress values for INSTALLING phase are also double-scaled. Line 359 manually scales to 0.5-1.0 range, then scaled_callback scales INSTALLING phase again from 0.1 to 1.0. The inner code should report progress in 0.0-1.0 range without manual scaling.
| new_version=version, | ||
| )) | ||
|
|
||
| except Exception: |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
| # Match against the full name (with arch suffix) or base name | ||
| if name in package_names or any(name.startswith(pkg.split('.')[0]) for pkg in package_names): | ||
| versions[name] = parts[1].strip() | ||
| except Exception: |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
No description provided.