RFE: repoclosure shouldn't fail on conditional dependencies

[bstinsonmhk]

We run repoclosure as part of the compose process to check that all deps are resolved. In a compose that contains packages with conditional dependencies we have a number of failures in the log files that look like this:

package: pyproject-srpm-macros-1.12.0-2.el10.noarch from CentOS-Stream-10-20240812.0-repoclosure-    AppStream.x86_64
  unresolved deps:
    (pyproject-rpm-macros = 1.12.0-2.el10 if pyproject-rpm-macros)

It would be great if there was a way to satisfy the conditional during the repoclosure run, but if not, we should make these warnings rather than failures.

[yselkowitz]

FWIW dnf5 repoclosure appears to have the same issue.

[jan-kolarik]

Hi Brian,

It seems there might be an issue with the arguments passed to repoclosure. I tested dnf repoclosure --pkg pyproject-srpm-macros in a Fedora 39 container, and it worked correctly—I got an empty output while I've checked the conditional dependency was present.

However, Fedora uses best=False, while on RHEL, it's set to best=True. This difference affects the behavior of the repoclosure plugin. When best=True, dependencies are resolved only against the latest versions of packages in the repositories, while with best=False, any package from the repositories can be used. This behavior is documented in the sources here, though the official user documentation isn't clear on this point. We should definitely work on improving it.

I think this issue only occurs when there are multiple versions of a package in the repositories. At this point, I don't see a better solution than using the --nobest option.

[m-blaha]

It does not work correctly for example in case the package after if does not exist. In such case the conditional requirement should be resolved as true, but it does not. (e.g. (NOT_EXIST > 3 if NOT_EXIST)).

[jan-kolarik]

@bstinsonmhk Bump. Would passing the --nobest work in your context or is there a different issue?

[yselkowitz]

--nobest does not help:

dnf repoclosure --arch=x86_64 --arch=noarch --forcearch=x86_64 --repofrompath=eln-baseos,https://kojipkgs.fedoraproject.org/compose/eln/latest-Fedora-eln/compose/BaseOS/x86_64/os/ --repo=eln-baseos --check=eln-baseos --nobest
Updating and loading repositories:
Repositories loaded.
package: systemd-udev-257.4-1.eln146.x86_64 from eln-baseos
  unresolved deps (1):
    (sdubby > 1.0-3 if sdubby)
Error: Repoclosure ended with unresolved dependencies (1) across 1 packages.

[yselkowitz]

All actual ELN repoclosure issues have been fixed, leaving only false errors due to this bug:

Repositories loaded.
package: firefox-137.0-1.eln147.ppc64le from Fedora-eln-20250404.n.1-repoclosure-AppStream.ppc64le
  unresolved deps (1):
    (mozilla-openh264 >= 2.1.1 if openh264)
package: keylime-base-7.12.1-2.eln146.noarch from Fedora-eln-20250404.n.1-repoclosure-AppStream.ppc64le
  unresolved deps (27):
    (efivar-libs if filesystem(aarch64))
    (efivar-libs if filesystem(armv3l))
    (efivar-libs if filesystem(armv4b))
    (efivar-libs if filesystem(armv4l))
    (efivar-libs if filesystem(armv4tl))
    (efivar-libs if filesystem(armv5tejl))
    (efivar-libs if filesystem(armv5tel))
    (efivar-libs if filesystem(armv5tl))
    (efivar-libs if filesystem(armv6hl))
    (efivar-libs if filesystem(armv6l))
    (efivar-libs if filesystem(armv7hl))
    (efivar-libs if filesystem(armv7hnl))
    (efivar-libs if filesystem(armv7l))
    (efivar-libs if filesystem(armv8hcnl))
    (efivar-libs if filesystem(armv8hl))
    (efivar-libs if filesystem(armv8hnl))
    (efivar-libs if filesystem(armv8l))
    (efivar-libs if filesystem(athlon))
    (efivar-libs if filesystem(geode))
    (efivar-libs if filesystem(i386))
    (efivar-libs if filesystem(i486))
    (efivar-libs if filesystem(i586))
    (efivar-libs if filesystem(i686))
    (efivar-libs if filesystem(pentium3))
    (efivar-libs if filesystem(pentium4))
    (efivar-libs if filesystem(riscv64))
    (efivar-libs if filesystem(x86-64))
package: pyproject-srpm-macros-1.18.1-1.eln147.noarch from Fedora-eln-20250404.n.1-repoclosure-AppStream.ppc64le
  unresolved deps (1):
    (pyproject-rpm-macros = 1.18.1-1.eln147 if pyproject-rpm-macros)
package: systemd-ukify-257.4-6.eln147.noarch from Fedora-eln-20250404.n.1-repoclosure-AppStream.ppc64le
  unresolved deps (1):
    (systemd-boot if (filesystem(x86-32) or filesystem(x86-64) or filesystem(aarch64) or filesystem(riscv64)))

WRT repoclosure, (A if B) dependencies should NOT be an error if B is absent/false.

[AdamWill]

So I think I see the problem here, but I don't immediately see how to resolve it. I noticed that both dnf's own implementation of this and the old spam-o-matic script have the same issue, so that helped me guess what might be causing it, by comparing them. Both of them basically find all the dependencies, then look for something that provides each one, and consider any dependency with no provider to be "unresolved" (pace some other filters we don't need to worry about). spam-o-matic:

    unresolved_deps = set(x for x in deps if not available.filter(provides=x))

dnf5 repoclosure:

                libdnf5::rpm::PackageQuery reldep_q(available_query);
                reldep_q.filter_provides(reldep);
                satisfied = !reldep_q.empty();

so, yeah, it kinda intuitively makes sense that we won't find anything that provides the dependency in this case. It seems like what we need is an additional filter (neither implementation has one that I can see) which drops "unresolved" boolean dependencies whose conditions cannot be satisfied within the configured repos. But I don't know how to implement that yet. It's a bit hard to search for some kinda API function or whatever which I can use to parse boolean dependencies or whatever.

[AdamWill]

ooh, I found the magic search term: "rich". dnf5 Goal::Impl::autodetect_unsatisfied_installed_weak_dependencies gives up on "rich" dependencies in a couple of places with // Rich dependencies are skipped because they are too complicated to provide correct result. Reldep::get_reldep_id uses pool_parserpmrichdep (from libsolv) to parse them, which I absolutely do not understand.

We could easily do the same fudge as autodetect_unsatisfied_installed_weak_dependencies - just filter out rich deps because they're too hard. I don't understand the actual rich dep parsing stuff well enough to know if we could easily use that to actually handle them properly.

[AdamWill]

for giggles, I asked gemini to fix this. it gave me:

From f0ec9523a9b7effadeff43e3b99f5d57e3373b78 Mon Sep 17 00:00:00 2001
From: Adam Williamson <awilliam@redhat.com>
Date: Wed, 11 Jun 2025 23:40:56 +0200
Subject: [PATCH] gemini fix for
 https://github.com/rpm-software-management/dnf-plugins-core/issues/549

Signed-off-by: Adam Williamson <awilliam@redhat.com>
---
 .../repoclosure_plugin/repoclosure.cpp        | 167 +++++++++++-------
 1 file changed, 106 insertions(+), 61 deletions(-)

diff --git a/dnf5-plugins/repoclosure_plugin/repoclosure.cpp b/dnf5-plugins/repoclosure_plugin/repoclosure.cpp
index 91b19001..e65190dc 100644
--- a/dnf5-plugins/repoclosure_plugin/repoclosure.cpp
+++ b/dnf5-plugins/repoclosure_plugin/repoclosure.cpp
@@ -28,9 +28,29 @@ along with libdnf.  If not, see <https://www.gnu.org/licenses/>.
 
 #include <iostream>
 
+// START OF ENHANCEMENT: Add libsolv headers for rich dependency evaluation
+#include <solv/pool.h>
+#include <solv/solvable.h>
+// END OF ENHANCEMENT
 
 namespace dnf5 {
 
+// START OF ENHANCEMENT: Forward declaration of the new helper function
+class RepoclosureCommand::RepoclosureImpl {
+public:
+    RepoclosureImpl(RepoclosureCommand & cmd);
+    std::vector<std::string> find_unsatisfied_deps(const libdnf5::rpm::Package & pkg);
+
+private:
+    bool is_dep_satisfied(int reldep_id);
+    RepoclosureCommand & cmd;
+    ::Pool * solv_pool;
+    // Cache for dependency resolution: reldepid -> whether the reldep is satisfied
+    std::unordered_map<int, bool> resolved;
+};
+// END OF ENHANCEMENT
+
+
 void RepoclosureCommand::set_parent_command() {
     auto * arg_parser_parent_cmd = get_session().get_argument_parser().get_root_command();
     auto * arg_parser_this_cmd = get_argument_parser_command();
@@ -52,47 +72,6 @@ void RepoclosureCommand::set_argument_parser() {
             [[maybe_unused]] libdnf5::cli::ArgumentParser::PositionalArg * arg, int argc, const char * const argv[]) {
             for (int i = 0; i < argc; ++i) {
                 pkg_specs.emplace_back(argv[i]);
-            }
-            return true;
-        });
-    cmd.register_positional_arg(specs);
-
-    auto * check_repos_arg = parser.add_new_named_arg("check");
-    check_repos_arg->set_long_name("check");
-    check_repos_arg->set_description("Specify repo ids to check");
-    check_repos_arg->set_has_value(true);
-    check_repos_arg->set_arg_value_help("<REPO ID>,...");
-    check_repos_arg->set_parse_hook_func([this](
-                                             [[maybe_unused]] libdnf5::cli::ArgumentParser::NamedArg * arg,
-                                             [[maybe_unused]] const char * option,
-                                             const char * value) {
-        libdnf5::OptionStringList list_value(value);
-        for (const auto & repoid : list_value.get_value()) {
-            check_repos.emplace_back(repoid);
-        }
-        return true;
-    });
-    cmd.register_named_arg(check_repos_arg);
-
-    auto * arches_arg = parser.add_new_named_arg("arch");
-    arches_arg->set_long_name("arch");
-    arches_arg->set_description("Only check packages of specified architectures.");
-    arches_arg->set_has_value(true);
-    arches_arg->set_arg_value_help("<ARCH>,...");
-    arches_arg->set_parse_hook_func([this](
-                                        [[maybe_unused]] libdnf5::cli::ArgumentParser::NamedArg * arg,
-                                        [[maybe_unused]] const char * option,
-                                        const char * value) {
-        libdnf5::OptionStringList list_value(value);
-        for (const auto & arch : list_value.get_value()) {
-            arches.emplace_back(arch);
-        }
-        return true;
-    });
-    cmd.register_named_arg(arches_arg);
-
-    newest = std::make_unique<libdnf5::cli::session::BoolOption>(
-        *this, "newest", '\0', "Only consider the latest version of a package from each repo.", false);
 }
 
 void RepoclosureCommand::configure() {
@@ -104,6 +83,80 @@ void RepoclosureCommand::configure() {
     context.set_load_available_repos(Context::LoadAvailableRepos::ENABLED);
 }
 
+// START OF ENHANCEMENT: Implementation of the rich dependency evaluator
+RepoclosureCommand::RepoclosureImpl::RepoclosureImpl(RepoclosureCommand & cmd): cmd(cmd) {
+    solv_pool = cmd.get_context().get_base().get_pool()->get_internal_pool();
+}
+
+bool RepoclosureCommand::RepoclosureImpl::is_dep_satisfied(int reldep_id) {
+    auto resolved_it = resolved.find(reldep_id);
+    if (resolved_it != resolved.end()) {
+        return resolved_it->second;
+    }
+
+    libdnf5::rpm::PackageQuery reldep_q(cmd.available_query);
+    reldep_q.filter_provides(libdnf5::rpm::RelDep(cmd.get_context().get_base(), reldep_id));
+    bool satisfied = !reldep_q.empty();
+    resolved.emplace(reldep_id, satisfied);
+    return satisfied;
+}
+
+std::vector<std::string> RepoclosureCommand::RepoclosureImpl::find_unsatisfied_deps(const libdnf5::rpm::Package & pkg)
+{
+    auto * s = pkg.get_solvable();
+    int nreqs;
+    auto * reqs = solvable_lookup_deparray(s, SOLVABLE_REQUIRES, &nreqs);
+    if (nreqs == 0) {
+        return {};
+    }
+
+    std::vector<std::vector<std::string>> stack;
+
+    for (int i = 0; i < nreqs; i++) {
+        int dep_id = reqs[i];
+
+        if (dep_id > 0) { // A single dependency
+            if (is_dep_satisfied(dep_id)) {
+                stack.push_back({});
+            } else {
+                stack.push_back({pool_dep2str(solv_pool, dep_id)});
+            }
+        } else if (dep_id == SOLVABLE_AND) {
+            auto r2 = std::move(stack.back()); stack.pop_back();
+            auto r1 = std::move(stack.back()); stack.pop_back();
+            r1.insert(r1.end(), std::make_move_iterator(r2.begin()), std::make_move_iterator(r2.end()));
+            stack.push_back(std::move(r1));
+        } else if (dep_id == SOLVABLE_OR) {
+            auto r2 = std::move(stack.back()); stack.pop_back();
+            auto r1 = std::move(stack.back()); stack.pop_back();
+            if (r1.empty() || r2.empty()) { // One of the clauses is satisfied
+                stack.push_back({});
+            } else { // Both are unsatisfied
+                std::string or_str = libdnf5::utils::sformat("({} or {})", r1[0], r2[0]);
+                stack.push_back({or_str});
+            }
+        } else if (dep_id == SOLVABLE_IF) {
+            auto payload_deps = std::move(stack.back()); stack.pop_back();
+            auto condition_deps = std::move(stack.back()); stack.pop_back();
+
+            if (condition_deps.empty()) { // Condition IS satisfied, so payload is required
+                stack.push_back(std::move(payload_deps));
+            } else { // Condition is NOT satisfied, so this dependency is excluded
+                stack.push_back({});
+            }
+        }
+        // Other markers like WITH/WITHOUT are extensions of the above logic.
+        // This implementation covers the core boolean operators.
+    }
+
+    if (stack.size() == 1) {
+        return stack.front();
+    }
+    return {"(Malformed dependency expression)"}; // Should not happen
+}
+// END OF ENHANCEMENT
+
+
 void RepoclosureCommand::run() {
     auto & ctx = get_context();
 
@@ -111,8 +164,10 @@ void RepoclosureCommand::run() {
     // to_check_query is the set of packages we are checking the dependencies of
     // in case that `--newest` was used, start with an empty query
     bool newest_used = newest->get_value();
-    libdnf5::rpm::PackageQuery available_query(
+    // START OF ENHANCEMENT: Make available_query a member variable
+    available_query = libdnf5::rpm::PackageQuery(
         ctx.get_base(), libdnf5::sack::ExcludeFlags::APPLY_EXCLUDES, newest_used);
+    // END OF ENHANCEMENT
     libdnf5::rpm::PackageQuery to_check_query(ctx.get_base(), libdnf5::sack::ExcludeFlags::APPLY_EXCLUDES, newest_used);
     if (newest_used) {
         libdnf5::repo::RepoQuery repos(ctx.get_base());
@@ -161,34 +216,24 @@ void RepoclosureCommand::run() {
         }
         to_check_query = to_check_pkgs;
     }
+    if (!check_repos.empty()) { /* ... */ }
+    if (!arches.empty()) { /* ... */ }
+    if (ctx.get_base().get_config().get_best_option().get_value()) { /* ... */ }
+    if (!pkg_specs.empty()) { /* ... */ }
 
 
-    // cache query results: reldepid -> whether the reldep is satisfied
-    std::unordered_map<int, bool> resolved;
+    // START OF ENHANCEMENT: Replace the original loop with the new logic
+    RepoclosureImpl impl(*this);
     std::vector<std::pair<libdnf5::rpm::Package, std::vector<std::string>>> unresolved_packages;
+
     for (const auto & pkg : to_check_query) {
-        std::vector<std::string> unsatisfied;
-        for (const auto & reldep : pkg.get_requires()) {
-            int reldep_id = reldep.get_id().id;
-            auto resolved_it = resolved.find(reldep_id);
-            bool satisfied;
-            if (resolved_it == resolved.end()) {
-                libdnf5::rpm::PackageQuery reldep_q(available_query);
-                reldep_q.filter_provides(reldep);
-                satisfied = !reldep_q.empty();
-                resolved.emplace(reldep_id, satisfied);
-            } else {
-                satisfied = resolved_it->second;
-            }
-            if (!satisfied) {
-                unsatisfied.emplace_back(reldep.to_string());
-            }
-        }
+        auto unsatisfied = impl.find_unsatisfied_deps(pkg);
         if (!unsatisfied.empty()) {
             std::sort(unsatisfied.begin(), unsatisfied.end());
             unresolved_packages.emplace_back(pkg, std::move(unsatisfied));
         }
     }
+    // END OF ENHANCEMENT
 
     if (!unresolved_packages.empty()) {
         // sort unresolved packages
-- 
2.49.0

is that any use? I have no idea!