DNF4 to DNF5 - #121
Conversation
📝 WalkthroughWalkthroughThe pull request migrates Content Resolver from DNF4 to DNF5 for repository, package, environment, workload, and buildroot analysis. It also updates root.log handling, runtime dependencies, generated identifiers, configuration messages, and documentation. ChangesDNF5 resolution and analysis
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Analyzer
participant libdnf5
participant Repositories
Analyzer->>libdnf5: Create repository and package queries
libdnf5->>Repositories: Load repository metadata
Analyzer->>libdnf5: Resolve packages with Goal.resolve()
libdnf5-->>Analyzer: Return transaction packages and problems
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@yselkowitz Would you mind reviewing this PR? Thanks! |
| filename = ("{page_name}.html".format( | ||
| page_name=page_name.replace(":", "--") | ||
| )) | ||
| filename = (f"{page_name.replace(":", "--")}.html") |
| workload = {} | ||
| workload["workload_conf_id"] = workload_conf["id"] | ||
| workload["env_conf_id"] = env_conf["id"] | ||
| workload["repo_id"] = repo["id"] | ||
| workload["arch"] = arch | ||
| workload["pkg_env_ids"] = [] | ||
| workload["pkg_added_ids"] = [] | ||
| workload["pkg_placeholder_ids"] = [] | ||
| workload["srpm_placeholder_names"] = [] | ||
| workload["pkg_relations"] = [] | ||
| workload["errors"] = {} | ||
| workload["errors"]["non_existing_pkgs"] = [] | ||
| workload["errors"]["non_existing_placeholder_deps"] = [] | ||
| workload["errors"]["message"] = f"Workload analysis timed out after 222 seconds or subprocess crashed" | ||
| workload["warnings"] = {} | ||
| workload["warnings"]["non_existing_pkgs"] = [] | ||
| workload["warnings"]["non_existing_placeholder_deps"] = [] | ||
| workload["warnings"]["message"] = None | ||
| workload["succeeded"] = False | ||
| workload["env_succeeded"] = False | ||
| workload["labels"] = list(set(workload_conf["labels"]) & set(env_conf["labels"])) | ||
| results[workload_id] = workload |
There was a problem hiding this comment.
Based on commit 8274128 the style should be:
results[workload_id] = {
"workload_conf_id": workload_conf["id"],etc.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
content_resolver/analyzer.py (2)
3300-3308: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
categorycan still beNonehere.A package that is in no workload list and only has
in_buildroot_of_srpm_id_req/depwithlevel_number == 0(or the default999… which does hit> 1) leavescategory = None, andview_all_arches["numbers"]["pkgs"][None]raisesKeyError. Same shape at Lines 3330-3336 for SRPMs. Worth aif category is None: continueguard now that the classification inputs changed with the buildroot rework.🛡️ Suggested guard
- view_all_arches["numbers"]["pkgs"][category] += 1 + if category: + view_all_arches["numbers"]["pkgs"][category] += 1🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@content_resolver/analyzer.py` around lines 3300 - 3308, Add a guard before the `view_all_arches["numbers"]["pkgs"][category]` increment in the package classification flow so entries with no assigned category are skipped rather than indexed with `None`; apply the same guard to the corresponding SRPM classification block around the analogous increment.
93-158: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRoot cause:
_get_build_deps_from_a_root_logparses by token position/count without validating shape. Every access (split_line[2],split_line.index("already"), theline_len in (...)branches) assumes a layout that varies between DNF4 and DNF5 logs; any mismatch raisesIndexError/ValueError, andprocess_single_srpm_root_logturns that intoParse failedwith zero deps for the whole SRPM — exactly the "zero dependencies" condition the new warning machinery then reports.
content_resolver/analyzer.py#L93-L158: guard"already" in split_linebeforeindex()in state 2, and requireline_len > 2before comparingsplit_line[2]in state 3.content_resolver/analyzer.py#L191-L217: addand "already" in split_lineto theline_len in (10, 11)branch, and resolve the# TODO: line_len == 9 ??gap so theelse: raise KojiRootLogErrorfallback isn't reached for valid logs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@content_resolver/analyzer.py` around lines 93 - 158, The root cause is unvalidated DNF log token parsing causing malformed or variant lines to abort dependency extraction. In _get_build_deps_from_a_root_log, guard the state-2 split_line.index("already") call with an "already" membership check, require split_line to contain at least three tokens before accessing split_line[2] in state 3, and add the same "already" validation to the line_len in (10, 11) branch. Resolve the valid line_len == 9 case so it is parsed correctly instead of reaching the KojiRootLogError fallback; apply these changes at content_resolver/analyzer.py lines 93-158 and 191-217.
🧹 Nitpick comments (5)
content_resolver/analyzer.py (4)
2619-2619: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDebug artifact: logs an always-empty dict.
fake_workload_resultsis populated by_analyze_workloads_asyncon the next line, so this only ever prints{}. Remove it.🧹 Remove
- log(f"fake_workload_results -> {fake_workload_results}") -🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@content_resolver/analyzer.py` at line 2619, Remove the debug log of fake_workload_results before the _analyze_workloads_async call; retain the workload analysis and subsequent result handling unchanged.
1547-1547: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the extraneous
fprefix (Ruff F541). Same at Line 1728.🧹 Fix
- log(f" Ignoring repository priority conflict (version mismatch between repos)") + log(" Ignoring repository priority conflict (version mismatch between repos)")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@content_resolver/analyzer.py` at line 1547, Remove the unnecessary f-string prefix from the repository priority conflict log message in the affected analyzer code, and make the same change to the corresponding log statement near the other reported occurrence. Keep both message text and logging behavior unchanged.Source: Linters/SAST tools
1471-1500: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
except (DnfErr, RuntimeError, Exception)collapses to bareException.Listing
Exceptionmakes the first two entries dead and silently swallows programming errors (e.g.NameError,KeyError) as "dependency resolution failure", which makes DNF5 migration bugs look like config problems. Narrow it to the libdnf5 error types you actually expect.♻️ Suggested change
- except (DnfErr, RuntimeError, Exception) as err: + except (DnfErr, UserAssertionError, RuntimeError) as err:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@content_resolver/analyzer.py` around lines 1471 - 1500, Update the exception handler around goal.resolve() to catch only the specific libdnf5/DNF error types expected from dependency resolution, removing the broad Exception entry and any redundant types. Preserve the existing workload error-message construction, logging, and return behavior for those expected resolution failures, while allowing programming errors to propagate.Source: Linters/SAST tools
486-548: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueVerify the intended verbosity of the new diagnostics blocks.
_validate_root_log_cache()and the root.log summary report add a lot of unconditional banner output (emoji separators, up to 20-item listings) on every run, which will dominate CI logs. The inline# TODO: Enable some form of log levelacknowledges this. Happy to help wire these behind a verbosity/--debugflag orlog_debug()helper if you want — want me to open an issue to track it?Also applies to: 2321-2435
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@content_resolver/analyzer.py` around lines 486 - 548, Gate the diagnostic output in _validate_root_log_cache and the root.log summary report behind the existing verbosity/debug setting or a dedicated log_debug helper. Keep validation and summary data collection unchanged, but suppress banners, separators, and item listings during normal runs while preserving them when debug verbosity is enabled.content_resolver/utils.py (1)
91-97: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
try/finally: passadds nothing.The
finallyblock is a no-op, so the wrapper is equivalent toyield libdnf5.base.Base(). Keeping it is fine as a transitional shim, but consider dropping the dead block (or leaving a# TODOif libdnf5 later gains explicit teardown) to avoid implying resource management that doesn't happen.♻️ Simplify
- base = libdnf5.base.Base() - try: - yield base - finally: - # DNF5 Base cleanup is handled by Python's garbage collector - # No explicit cleanup needed, but the finally block ensures - # proper exception handling and resource cleanup if needed in future - pass + # DNF5 Base cleanup is handled by Python's garbage collector. + # TODO: add explicit teardown here if libdnf5 gains a close()/reset() API. + yield libdnf5.base.Base()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@content_resolver/utils.py` around lines 91 - 97, Remove the no-op finally block from the context manager around the yielded DNF5 base in content_resolver/utils.py, leaving the wrapper to yield base directly; if retaining the transitional structure, replace it with a concise TODO rather than implying active cleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@content_resolver/analyzer.py`:
- Line 1634: Update the exception-handling call to err_log in the workload
analysis flow to remove the unsupported file=sys.stderr keyword; pass only the
formatted error message, relying on err_log’s existing stderr behavior while
preserving the queued failure result.
- Around line 1-77: Add the missing traceback module import in analyzer.py so
the exception-handling path that calls traceback.format_exc() can execute
correctly and report handled workload failures to the queue. Do not alter the
surrounding workload error-handling behavior.
- Around line 758-798: Bind repo_sack before the per-repo filtering loop so it
is always available when the retry block calls repo_sack.load_repos(). Reuse
that single sack for the selected repositories, preserving the existing skip
behavior while ensuring no-repository cases raise the intended RepoDownloadError
rather than UnboundLocalError.
In `@content_resolver/config_manager.py`:
- Line 826: Update the ConfigError raise in the yaml.YAMLError handler to
explicitly chain the caught err with from err, preserving the existing error
message and exception context.
- Line 123: Update the missing-baseurl error raised by _load_config_repo_v2() to
use the existing document_id value instead of the undefined yml_file variable,
while preserving the intended ConfigError and message context.
- Line 934: Update the JSON-loading exception handler surrounding load_data() to
catch the expected exception type(s) as err, then raise ConfigError with the
existing contextual message and chain the original exception. Preserve the
current error message while ensuring load_data() failures do not reference an
undefined err.
In `@content_resolver/page_generation.py`:
- Around line 23-26: Update the filename construction in the page-generation
flow to prevent path traversal from page_name values. Sanitize or validate the
generated basename and resolve the target path before the open call, ensuring it
remains within settings["output"] and rejecting absolute paths, separators, and
traversal components while preserving valid filenames.
In `@README.md`:
- Line 21: Add the text language identifier to each new fenced code block in
README.md, including the directory tree and phase-description blocks referenced
by the comment, so all fences satisfy markdownlint MD040.
- Line 28: Update the Code Structure tree entry for the history module from
history_data.py to historia_data.py, matching the repository’s
content_resolver/historia_data.py module name while preserving its existing
description.
- Around line 221-228: Update the repository-priority YAML example so each
repository key maps to a nested priority value, making entries such as BaseOS,
AppStream, CRB, Extras, buildroot, and Rawhide valid YAML while preserving their
existing priority numbers and comments.
---
Outside diff comments:
In `@content_resolver/analyzer.py`:
- Around line 3300-3308: Add a guard before the
`view_all_arches["numbers"]["pkgs"][category]` increment in the package
classification flow so entries with no assigned category are skipped rather than
indexed with `None`; apply the same guard to the corresponding SRPM
classification block around the analogous increment.
- Around line 93-158: The root cause is unvalidated DNF log token parsing
causing malformed or variant lines to abort dependency extraction. In
_get_build_deps_from_a_root_log, guard the state-2 split_line.index("already")
call with an "already" membership check, require split_line to contain at least
three tokens before accessing split_line[2] in state 3, and add the same
"already" validation to the line_len in (10, 11) branch. Resolve the valid
line_len == 9 case so it is parsed correctly instead of reaching the
KojiRootLogError fallback; apply these changes at content_resolver/analyzer.py
lines 93-158 and 191-217.
---
Nitpick comments:
In `@content_resolver/analyzer.py`:
- Line 2619: Remove the debug log of fake_workload_results before the
_analyze_workloads_async call; retain the workload analysis and subsequent
result handling unchanged.
- Line 1547: Remove the unnecessary f-string prefix from the repository priority
conflict log message in the affected analyzer code, and make the same change to
the corresponding log statement near the other reported occurrence. Keep both
message text and logging behavior unchanged.
- Around line 1471-1500: Update the exception handler around goal.resolve() to
catch only the specific libdnf5/DNF error types expected from dependency
resolution, removing the broad Exception entry and any redundant types. Preserve
the existing workload error-message construction, logging, and return behavior
for those expected resolution failures, while allowing programming errors to
propagate.
- Around line 486-548: Gate the diagnostic output in _validate_root_log_cache
and the root.log summary report behind the existing verbosity/debug setting or a
dedicated log_debug helper. Keep validation and summary data collection
unchanged, but suppress banners, separators, and item listings during normal
runs while preserving them when debug verbosity is enabled.
In `@content_resolver/utils.py`:
- Around line 91-97: Remove the no-op finally block from the context manager
around the yielded DNF5 base in content_resolver/utils.py, leaving the wrapper
to yield base directly; if retaining the transitional structure, replace it with
a concise TODO rather than implying active cleanup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b19c93c9-999b-4895-8299-11ad32c9c814
📒 Files selected for processing (9)
.github/workflows/docker-image.yml.github/workflows/fork-only-workflow.ymlDockerfileREADME.mdcontent_resolver/analyzer.pycontent_resolver/config_manager.pycontent_resolver/historia_data.pycontent_resolver/page_generation.pycontent_resolver/utils.py
| } | ||
| queue_result.put(workload) | ||
| # Log error to stderr so it appears in logs | ||
| err_log(f" ERROR analyzing workload {workload_conf['id']}:{env_conf['id']}:{repo['id']}:{arch}-> {e}", file=sys.stderr) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
err_log() doesn't accept a file keyword.
content_resolver/utils.py:31-32 defines err_log(msg) and already writes to sys.stderr. Passing file=sys.stderr raises TypeError inside the exception handler, after queue_result.put(workload) — so the failure result is queued but the subprocess still dies with a confusing secondary error.
🐛 Fix
- err_log(f" ERROR analyzing workload {workload_conf['id']}:{env_conf['id']}:{repo['id']}:{arch}-> {e}", file=sys.stderr)
+ err_log(f" ERROR analyzing workload {workload_conf['id']}:{env_conf['id']}:{repo['id']}:{arch} -> {e}")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| err_log(f" ERROR analyzing workload {workload_conf['id']}:{env_conf['id']}:{repo['id']}:{arch}-> {e}", file=sys.stderr) | |
| err_log(f" ERROR analyzing workload {workload_conf['id']}:{env_conf['id']}:{repo['id']}:{arch} -> {e}") |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@content_resolver/analyzer.py` at line 1634, Update the exception-handling
call to err_log in the workload analysis flow to remove the unsupported
file=sys.stderr keyword; pass only the formatted error message, relying on
err_log’s existing stderr behavior while preserving the queued failure result.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
content_resolver/analyzer.py (8)
1664-1717: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winTerminate and join timed-out workload processes.
The timeout path creates a failed result but leaves the child process running, decrements
current_subprocesses, and starts more work. A timed-out DNF process can therefore bypass the concurrency limit and exhaust memory or file descriptors. Join successful children too; terminate and join timed-out ones.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@content_resolver/analyzer.py` around lines 1664 - 1717, Update the workload process lifecycle around _analyze_workload_process: after the wait completes, join the child when it exits successfully; when queue_result remains empty, terminate the timed-out process, then join it before decrementing current_subprocesses and continuing with the failed workload result. Ensure all child processes are reaped before scheduling more work.
1181-1201: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winKeep failed workload results schema-compatible.
This result omits
warningsandlabels, but downstream code readsworkload["labels"]at Line 1989 andworkload["warnings"]["message"]at Line 3180. Any failed environment will therefore crash view generation instead of producing a failed result.Proposed fix
"errors": { "non_existing_pkgs": [], "message": "...", }, + "warnings": { + "non_existing_pkgs": [], + "non_existing_placeholder_deps": [], + "message": None, + }, "succeeded": False, "env_succeeded": False, + "labels": list(set(workload_conf["labels"]) & set(env_conf["labels"])),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@content_resolver/analyzer.py` around lines 1181 - 1201, Update _return_failed_workload_env_err to include the same warnings and labels fields expected by downstream workload consumers, using the established result schema and empty/default values appropriate for a failed environment. Preserve the existing error details and failure flags so view generation can safely access workload["labels"] and workload["warnings"]["message"].
2382-2388: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not cache failed root.log results as empty dependency sets.
The failure fallback supplies
deps=[], and_apply_srpm_result()persists that empty list. On the next run, the cache-hit path treats it as valid and skips downloading the root.log again, permanently converting transient failures into zero-dependency results.Proposed fix
deps = result["deps"] + if result.get("error"): + return + self.cache["root_log_deps"]["next"][koji_id][arch][srpm_id] = depsAlso applies to: 2450-2456
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@content_resolver/analyzer.py` around lines 2382 - 2388, Update the failure handling around _apply_srpm_result() at both referenced call sites so failed root.log resolution is not persisted as a successful empty dependency set. Preserve the error result for reporting, but bypass or adjust cache persistence and ensure subsequent runs retry downloading and analyzing root.log instead of treating the failure as a valid cache hit.
891-937: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftDo not ship empty package relationship data.
_analyze_package_relations()still only creates emptyrequired_by,recommended_by,suggested_by, andsupplementslists, and those values are propagated into view aggregates and used by maintainer-recommendation logic. Populate the dependency graph before returning the relation payload.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@content_resolver/analyzer.py` around lines 891 - 937, The method _analyze_package_relations must populate actual package dependency relationships instead of returning empty relation lists. Build the reverse dependency graph by iterating the supplied packages and recording required, recommended, suggested, and supplement relationships for matching package IDs, while preserving metadata and placeholder entries before returning relations.Source: MCP tools
740-744: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winSet package exclusions in
_analyze_pkgs()before loading repositories.
_analyze_pkgs()creates/configures each new repo and callsrepo_sack.load_repos(), but it only appliesbaseurlandpriority;_load_repo_cached()already appliesexcludeviaexcludepkgs. Add the same exclusion setup here so excluded packages are not indexed intoself.data["pkgs"].Proposed fix
repo_config = new_repo.get_config() repo_config.get_baseurl_option().set([repo_data["baseurl"]]) repo_config.get_priority_option().set(repo_data["priority"]) + if repo_data["exclude"]: + repo_config.get_excludepkgs_option().set(repo_data["exclude"]) repo_names_to_load.append(repo_name)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@content_resolver/analyzer.py` around lines 740 - 744, Update _analyze_pkgs() while configuring each repository created by repo_sack.create_repo() to also apply repo_data["exclude"] through the repository configuration’s excludepkgs option before repo_sack.load_repos() runs. Match the exclusion setup already used by _load_repo_cached(), while preserving the existing baseurl, priority, and repository-loading behavior.Source: MCP tools
768-771: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftRebuild the DNF5 Base between repository-load retries.
load_repos()is a one-time operation perRepoSack. Three retries already reuse the same sack and callload_repos()repeatedly; the first retry block disables repos mid-transaction but tries the same sack again, so it cannot recover from repository metadata failures via the existing disabled-repo loop. Create a freshBase/RepoSack, reconfigure the remaining repos, and set up the sack for each failed attempt, or remove unsupported retries to avoid hidden DNF5 instability.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@content_resolver/analyzer.py` around lines 768 - 771, Rebuild the DNF5 Base and RepoSack before each retry in the repository-loading loop around load_repos, rather than reusing the sack after a failed attempt. Reinitialize the base, reapply the currently enabled/remaining repository configuration, and invoke load_repos only on the fresh sack so the disabled-repository retry flow can recover from metadata failures.Source: MCP tools
1101-1109: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winCheck transaction resolution problems before downloading the transaction.
Goal.resolve()can return a transaction containing dependency-resolution problems without raising. Addfrom libdnf5.base import GoalProblem_NO_PROBLEM, then aftertransaction = goal.resolve(), reject the environment whentransaction.get_problems() != GoalProblem_NO_PROBLEMusingtransaction.get_resolve_logs_as_strings()before callingtransaction.download().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@content_resolver/analyzer.py` around lines 1101 - 1109, Add the GoalProblem_NO_PROBLEM import and validate transaction.get_problems() immediately after goal.resolve() in the transaction-resolution flow. When problems are present, log the strings from transaction.get_resolve_logs_as_strings(), mark env as unsuccessful, populate env["errors"]["message"], and return before transaction.download(); preserve the existing DnfErr handling.Source: MCP tools
1520-1558: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not classify every
cannot install bothresult as benign.
has_cannot_install_bothmakes the whole transaction a repository conflict, whileis_real_erroronly recognizes"nothing provides"in this branch. A mixed DNF5 resolve log containing both repository conflicts and missing-provider failures can be ignored and reported as successful; use structured DNF5 resolve logs or filter only the exact cross-repository version-conflict pattern.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@content_resolver/analyzer.py` around lines 1520 - 1558, The dependency-resolution filtering around has_cannot_install_both and is_repo_conflict is too broad and can ignore mixed failures. Replace the substring-based classification with structured DNF5 resolve-log handling, or require an exact cross-repository version-conflict pattern; ensure any log containing missing-provider or other real dependency failures remains unsuccessful even when repository conflict text is also present.Source: MCP tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@content_resolver/analyzer.py`:
- Around line 1664-1717: Update the workload process lifecycle around
_analyze_workload_process: after the wait completes, join the child when it
exits successfully; when queue_result remains empty, terminate the timed-out
process, then join it before decrementing current_subprocesses and continuing
with the failed workload result. Ensure all child processes are reaped before
scheduling more work.
- Around line 1181-1201: Update _return_failed_workload_env_err to include the
same warnings and labels fields expected by downstream workload consumers, using
the established result schema and empty/default values appropriate for a failed
environment. Preserve the existing error details and failure flags so view
generation can safely access workload["labels"] and
workload["warnings"]["message"].
- Around line 2382-2388: Update the failure handling around _apply_srpm_result()
at both referenced call sites so failed root.log resolution is not persisted as
a successful empty dependency set. Preserve the error result for reporting, but
bypass or adjust cache persistence and ensure subsequent runs retry downloading
and analyzing root.log instead of treating the failure as a valid cache hit.
- Around line 891-937: The method _analyze_package_relations must populate
actual package dependency relationships instead of returning empty relation
lists. Build the reverse dependency graph by iterating the supplied packages and
recording required, recommended, suggested, and supplement relationships for
matching package IDs, while preserving metadata and placeholder entries before
returning relations.
- Around line 740-744: Update _analyze_pkgs() while configuring each repository
created by repo_sack.create_repo() to also apply repo_data["exclude"] through
the repository configuration’s excludepkgs option before repo_sack.load_repos()
runs. Match the exclusion setup already used by _load_repo_cached(), while
preserving the existing baseurl, priority, and repository-loading behavior.
- Around line 768-771: Rebuild the DNF5 Base and RepoSack before each retry in
the repository-loading loop around load_repos, rather than reusing the sack
after a failed attempt. Reinitialize the base, reapply the currently
enabled/remaining repository configuration, and invoke load_repos only on the
fresh sack so the disabled-repository retry flow can recover from metadata
failures.
- Around line 1101-1109: Add the GoalProblem_NO_PROBLEM import and validate
transaction.get_problems() immediately after goal.resolve() in the
transaction-resolution flow. When problems are present, log the strings from
transaction.get_resolve_logs_as_strings(), mark env as unsuccessful, populate
env["errors"]["message"], and return before transaction.download(); preserve the
existing DnfErr handling.
- Around line 1520-1558: The dependency-resolution filtering around
has_cannot_install_both and is_repo_conflict is too broad and can ignore mixed
failures. Replace the substring-based classification with structured DNF5
resolve-log handling, or require an exact cross-repository version-conflict
pattern; ensure any log containing missing-provider or other real dependency
failures remains unsuccessful even when repository conflict text is also
present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d951d737-cb0e-487b-b8cf-dbe448d82fef
📒 Files selected for processing (3)
content_resolver/analyzer.pycontent_resolver/config_manager.pycontent_resolver/page_generation.py
🚧 Files skipped from review as they are similar to previous changes (2)
- content_resolver/page_generation.py
- content_resolver/config_manager.py
Latest status of DNF-4 vs DNF-5 |
2 participants
DNF5 Migration and Code Cleanup
Summary
Key Changes
DNF5 Integration:
Improved Error Handling:
Code Quality:
Summary by CodeRabbit