Fix pin repr in solver error messages (#3268)

* Add solver pin test

* Improve pin repr in SAT error messages

libmamba/ext/solv-cpp/include/solv-cpp/pool.hpp

@@ -461,7 +461,7 @@ namespace solv
         // Safe optional unchecked because we iterate over available values
         return for_each_whatprovides_id(
             dep,
-            [this, func](SolvableId id) { func(get_solvable(id)).value(); }
+            [this, func](SolvableId id) { func(get_solvable(id).value()); }
         );
     }
 

libmamba/src/solver/libsolv/helpers.cpp

@@ -132,14 +132,6 @@ namespace mamba::solver::libsolv
             std::transform(feats.begin(), feats.end(), std::back_inserter(out.track_features), id_to_str);
         }
 
-        // Pins have a name like "pin-fsej43208fsd" so we set a readable name for them.
-        // This is mainly displayed in the solver error messages.
-        // Perhaps this is not the best place to put this...
-        if (s.type() == solv::SolvableType::Pin)
-        {
-            out.name = fmt::format("pin on {}", fmt::join(out.constrains, " and "));
-        }
-
         return out;
     }
 

libmamba/src/solver/libsolv/unsolvable.cpp

@@ -283,16 +283,82 @@ namespace mamba::solver::libsolv
             return added;
         }
 
+        void fixup_pin_solvable(const solv::ObjPool& pool, specs::PackageInfo& pkg)
+        {
+            // Pins solvable have a name like "pin-fsej43208fsd" and a constraint representing
+            // the pin.
+            // They are added with a install top-level dependency
+            // ``Install{ "pin-fsej43208fsd"_ms }``.
+            // We need to change their name to make them look more readable.
+            if (auto id = pool.find_string(pkg.name))
+            {
+                pool.for_each_whatprovides(
+                    *id,
+                    [&](const solv::ObjSolvableViewConst& solv_pin)
+                    {
+                        if (solv_pin.type() == solv::SolvableType::Pin)
+                        {
+                            // There should really just be one constraint
+                            pkg.name = fmt::format("pin on {}", fmt::join(pkg.constrains, " and "));
+                        }
+                        return solv::LoopControl::Break;
+                    }
+                );
+            }
+        }
+
+        void fixup_dep_on_pin_solvable(const solv::ObjPool& pool, specs::MatchSpec& ms)
+        {
+            // Pins solvable have a name like "pin-fsej43208fsd" and a constraint representing
+            // the pin.
+            // They are added with a install top-level dependency
+            // ``Install{ "pin-fsej43208fsd"_ms }``.
+            // We need to change their name to make them look more readable.
+            if (auto id = pool.find_string(ms.name().str()))
+            {
+                pool.for_each_whatprovides(
+                    *id,
+                    [&](const solv::ObjSolvableViewConst& solv_pin)
+                    {
+                        if (solv_pin.type() == solv::SolvableType::Pin)
+                        {
+                            auto pin = make_package_info(pool, solv_pin);
+                            // There should really just be one constraint
+                            ms.set_name(specs::MatchSpec::NameSpec(
+                                fmt::format("pin on {}", fmt::join(pin.constrains, " and "))
+                            ));
+                        }
+                        return solv::LoopControl::Break;
+                    }
+                );
+            }
+        }
+
         void ProblemsGraphCreator::parse_problems()
         {
             // TODO Throwing error for now but we should use expected in UnSolvable API
-            constexpr auto make_match_spec = [](std::string_view str) -> specs::MatchSpec
+            auto make_match_spec = [&](std::string_view str) -> specs::MatchSpec
             {
                 return specs::MatchSpec::parse(str)
                     .or_else([](specs::ParseError&& err) { throw std::move(err); })
+                    .transform(
+                        [&](specs::MatchSpec&& ms) -> specs::MatchSpec
+                        {
+                            fixup_dep_on_pin_solvable(m_pool, ms);
+                            return std::move(ms);
+                        }
+                    )
                     .value();
             };
 
+            // FIXME in practice we should extend the data structure with a PinNode rather than
+            // patching the name.
+            auto fixup_pkg = [&](specs::PackageInfo pkg) -> specs::PackageInfo
+            {
+                fixup_pin_solvable(m_pool, pkg);
+                return pkg;
+            };
+
             for (auto& problem : all_problems_structured(m_pool, m_solver))
             {
                 std::optional<specs::PackageInfo>& source = problem.source;
@@ -315,11 +381,11 @@ namespace mamba::solver::libsolv
                         }
                         auto src_id = add_solvable(
                             problem.source_id,
-                            PackageNode{ std::move(source).value() }
+                            PackageNode{ fixup_pkg(std::move(source).value()) }
                         );
                         node_id tgt_id = add_solvable(
                             problem.target_id,
-                            PackageNode{ std::move(target).value() }
+                            PackageNode{ fixup_pkg(std::move(target).value()) }
                         );
                         node_id cons_id = add_solvable(
                             problem.dep_id,
@@ -343,7 +409,7 @@ namespace mamba::solver::libsolv
                         }
                         auto src_id = add_solvable(
                             problem.source_id,
-                            PackageNode{ std::move(source).value() }
+                            PackageNode{ fixup_pkg(std::move(source).value()) }
                         );
                         auto edge = make_match_spec(dep.value());
                         bool added = add_expanded_deps_edges(src_id, problem.dep_id, edge);
@@ -406,7 +472,7 @@ namespace mamba::solver::libsolv
                         auto edge = make_match_spec(dep.value());
                         node_id src_id = add_solvable(
                             problem.source_id,
-                            PackageNode{ std::move(source).value() }
+                            PackageNode{ fixup_pkg(std::move(source).value()) }
                         );
                         node_id dep_id = add_solvable(
                             problem.dep_id,
@@ -429,11 +495,11 @@ namespace mamba::solver::libsolv
                         }
                         node_id src_id = add_solvable(
                             problem.source_id,
-                            PackageNode{ std::move(source).value() }
+                            PackageNode{ fixup_pkg(std::move(source).value()) }
                         );
                         node_id tgt_id = add_solvable(
                             problem.target_id,
-                            PackageNode{ std::move(target).value() }
+                            PackageNode{ fixup_pkg(std::move(target).value()) }
                         );
                         add_conflict(src_id, tgt_id);
                         break;
@@ -449,7 +515,7 @@ namespace mamba::solver::libsolv
                             break;
                         }
 
-                        // We re-create an dependency. There is no dependency ready to use for
+                        // We re-create a dependency. There is no dependency ready to use for
                         // how the solver is handling this package, as this is resolved in term of
                         // installed packages and solver flags (allow downgrade...) rather than a
                         // dependency.

libmamba/src/solver/problems_graph.cpp

@@ -1309,6 +1309,34 @@ namespace mamba::solver
             std::visit(do_write, concat_nodes(tn.ids));
         }
 
+        /**
+         * Sort suffices such that if one ends with the other, the longest one is put first.
+         */
+        template <std::size_t N>
+        constexpr auto
+        sorted_suffix(std::array<std::string_view, N> arr) -> std::array<std::string_view, N>
+        {
+            std::sort(
+                arr.begin(),
+                arr.end(),
+                [](const auto& str1, const auto& str2) { return util::ends_with(str1, str2); }
+            );
+            return arr;
+        }
+
+        auto rstrip_excessive_free(std::string_view str) -> std::string_view
+        {
+            str = util::rstrip(str);
+            str = util::remove_suffix(str, specs::GlobSpec::free_pattern);
+            str = util::rstrip(str);
+            for (const auto& suffix : sorted_suffix(specs::VersionSpec::all_free_strs))
+            {
+                str = util::remove_suffix(str, suffix);
+            }
+            str = util::rstrip(str);
+            return str;
+        }
+
         void TreeExplainer::write_pkg_dep(const TreeNode& tn)
         {
             auto edges = concat_edges(tn.ids_from, tn.ids);
@@ -1322,8 +1350,24 @@ namespace mamba::solver
             }
             else
             {
-                write(fmt::format(style, (size == 1 ? "{} {}" : "{} [{}]"), edges.name(), vers_builds_trunc)
-                );
+                // Single depenency with only name constraint often end up looking like
+                // ``python =* *`` so we strip all this.
+                // Best would be to handle this with a richer NamedList that contains
+                // ``VersionSpecs`` to avoid flaky reliance on string modification.
+                const auto relevant_vers_builds_trunc = rstrip_excessive_free(vers_builds_trunc);
+                if (relevant_vers_builds_trunc.empty())
+                {
+                    write(fmt::format(style, "{}", edges.name()));
+                }
+                else
+                {
+                    write(fmt::format(
+                        style,
+                        (size == 1 ? "{} {}" : "{} [{}]"),
+                        edges.name(),
+                        relevant_vers_builds_trunc
+                    ));
+                }
             }
         }
 

libmamba/tests/src/solver/libsolv/test_solver.cpp

@@ -925,6 +925,74 @@ TEST_SUITE("solver::libsolv::solver")
         }
     }
 
+    TEST_CASE("Respect pins")
+    {
+        using PackageInfo = specs::PackageInfo;
+
+        auto db = libsolv::Database({});
+
+        SUBCASE("Respect pins through direct dependencies")
+        {
+            auto pkg1 = PackageInfo("foo");
+            pkg1.version = "1.0";
+            auto pkg2 = PackageInfo("foo");
+            pkg2.version = "2.0";
+
+            db.add_repo_from_packages(std::array{ pkg1, pkg2 });
+
+            auto request = Request{
+                /* .flags= */ {},
+                /* .jobs= */ { Request::Pin{ "foo=1.0"_ms }, Request::Install{ "foo"_ms } },
+            };
+            const auto outcome = libsolv::Solver().solve(db, request);
+
+            REQUIRE(outcome.has_value());
+            REQUIRE(std::holds_alternative<Solution>(outcome.value()));
+            const auto& solution = std::get<Solution>(outcome.value());
+
+            const auto foo_actions = find_actions_with_name(solution, "foo");
+            REQUIRE_EQ(foo_actions.size(), 1);
+            CHECK(std::holds_alternative<Solution::Install>(foo_actions.front()));
+            CHECK_EQ(std::get<Solution::Install>(foo_actions.front()).install.version, "1.0");
+        }
+
+        SUBCASE("Respect pins through indirect dependencies")
+        {
+            auto pkg1 = PackageInfo("foo");
+            pkg1.version = "1.0";
+            auto pkg2 = PackageInfo("foo");
+            pkg2.version = "2.0";
+            auto pkg3 = PackageInfo("bar");
+            pkg3.version = "1.0";
+            pkg3.dependencies = { "foo=1.0" };
+            auto pkg4 = PackageInfo("bar");
+            pkg4.version = "2.0";
+            pkg4.dependencies = { "foo=2.0" };
+
+            db.add_repo_from_packages(std::array{ pkg1, pkg2, pkg3, pkg4 });
+
+            auto request = Request{
+                /* .flags= */ {},
+                /* .jobs= */ { Request::Pin{ "foo=1.0"_ms }, Request::Install{ "bar"_ms } },
+            };
+            const auto outcome = libsolv::Solver().solve(db, request);
+
+            REQUIRE(outcome.has_value());
+            REQUIRE(std::holds_alternative<Solution>(outcome.value()));
+            const auto& solution = std::get<Solution>(outcome.value());
+
+            const auto foo_actions = find_actions_with_name(solution, "foo");
+            REQUIRE_EQ(foo_actions.size(), 1);
+            CHECK(std::holds_alternative<Solution::Install>(foo_actions.front()));
+            CHECK_EQ(std::get<Solution::Install>(foo_actions.front()).install.version, "1.0");
+
+            const auto bar_actions = find_actions_with_name(solution, "bar");
+            REQUIRE_EQ(bar_actions.size(), 1);
+            CHECK(std::holds_alternative<Solution::Install>(bar_actions.front()));
+            CHECK_EQ(std::get<Solution::Install>(bar_actions.front()).install.version, "1.0");
+        }
+    }
+
     TEST_CASE("Handle complex matchspecs")
     {
         using PackageInfo = specs::PackageInfo;

libmamba/tests/src/solver/test_problems_graph.cpp

@@ -272,6 +272,30 @@ namespace
         return create_pubgrub_hard_(ctx, channel_context, true);
     }
 
+    /**
+     * Create a conflict due to a pin.
+     */
+    auto create_pin_conflict(Context&, ChannelContext& channel_context)
+    {
+        return std::pair(
+            create_pkgs_database(
+                channel_context,
+                std::array{
+                    mkpkg("foo", "2.0.0", { "bar=2.0" }),
+                    mkpkg("bar", "1.0.0"),
+                    mkpkg("bar", "2.0.0"),
+                }
+            ),
+            Request{
+                {},
+                {
+                    Request::Install{ "foo"_ms },
+                    Request::Pin{ "bar=1.0"_ms },
+                },
+            }
+        );
+    }
+
     auto make_platform_channels(
         std::vector<std::string>&& channels,
         const std::vector<std::string>& platforms