pkgrrxx: Work around an issue where FETCH_USING is listed in BOOTSTRA…

…P_DEPENDS and creates a dependency cycle

NEWS.md

@@ -1,5 +1,10 @@
 # Release notes
 
+## 0.1.5 -- 2023-08-06
+
+* Work around an issue where `FETCH_USING` is listed in `BOOTSTRAP_DEPENDS`
+  and creates a dependency cycle, reported by Oskar.
+
 ## 0.1.4 -- 2023-08-05
 
 * Fix a bug in dependency cycle detection. Previously it didn't only failed

configure.ac

@@ -2,7 +2,7 @@
 # Process this file with autoconf to produce a configure script.
 
 AC_PREREQ([2.71])
-AC_INIT([pkgchkxx], [0.1.4], [pkgsrc-users@NetBSD.org])
+AC_INIT([pkgchkxx], [0.1.5], [pkgsrc-users@NetBSD.org])
 AC_CONFIG_MACRO_DIR([m4])
 AC_CONFIG_SRCDIR([lib/pkgxx/pkgname.hxx])
 AC_CONFIG_HEADERS([lib/pkgxx/config.h])

lib/pkgxx/graph.hxx

@@ -122,11 +122,15 @@ namespace pkgxx {
         std::enable_if_t<!std::is_same_v<EdgeT_, void>, void>
         add_edge(VertexT const& src, VertexT const& dest, EdgeT_ const& edge);
 
-        /** Remove an edge from the graph if it exists. Only available for
+        /** Remove an edge from the graph if it exists. */
+        void
+        remove_edge(VertexT const& src, VertexT const& dest);
+
+        /** Remove in-edges to a vertex if it exists. Only available for
          * bidirectional graphs. */
         template <bool IsBidi = IsBidirectional>
         std::enable_if_t<IsBidi, void>
-        remove_edge(VertexT const& src, VertexT const& dest);
+        remove_in_edges(VertexT const& src);
 
         /** Remove out-edges from a vertex if it exists. */
         void
@@ -380,27 +384,57 @@ namespace pkgxx {
     }
 
     template <typename VertexT, typename EdgeT, bool IsBidirectional>
-    template <bool IsBidi>
-    std::enable_if_t<IsBidi, void>
+    void
     graph<VertexT, EdgeT, IsBidirectional>::remove_edge(VertexT const& src, VertexT const& dest) {
-        static_assert(IsBidi == IsBidirectional, "can't explicitly specialise");
-
         if (auto src_id = _vertex_id_of.find(src); src_id != _vertex_id_of.end()) {
             auto src_v = _vertices.find(src_id->second);
             assert(src_v != _vertices.end());
 
             if (auto dest_id = _vertex_id_of.find(dest); dest_id != _vertex_id_of.end()) {
-                auto dest_v = _vertices.find(dest_id->second);
-                assert(dest_v != _vertices.end());
-
                 if (src_v->second.outs.erase(dest_id->second)) {
-                    dest_v->second.ins.erase(src_id->second);
+                    if constexpr (IsBidirectional) {
+                        auto dest_v = _vertices.find(dest_id->second);
+                        assert(dest_v != _vertices.end());
+
+                        dest_v->second.ins.erase(src_id->second);
+                    }
                     _tsort_cache.reset();
                 }
             }
         }
     }
 
+    template <typename VertexT, typename EdgeT, bool IsBidirectional>
+    template <bool IsBidi>
+    std::enable_if_t<IsBidi, void>
+    graph<VertexT, EdgeT, IsBidirectional>::remove_in_edges(VertexT const& value) {
+        static_assert(IsBidi == IsBidirectional, "can't explicitly specialise");
+
+        if (auto dest_id = _vertex_id_of.find(value); dest_id != _vertex_id_of.end()) {
+            auto dest_v = _vertices.find(dest_id->second);
+            assert(dest_v != _vertices.end());
+
+            if (!dest_v->second.ins.empty()) {
+                for (auto src: dest_v->second.ins) {
+                    vertex_id src_id;
+                    if constexpr (std::is_same_v<EdgeT, void>) {
+                        src_id = src;
+                    }
+                    else {
+                        src_id = src.first;
+                    }
+
+                    auto src_v = _vertices.find(src_id);
+                    assert(src_v != _vertices.end());
+
+                    src_v->second.outs.erase(dest_id->second);
+                }
+                dest_v->second.ins.clear();
+                _tsort_cache.reset();
+            }
+        }
+    }
+
     template <typename VertexT, typename EdgeT, bool IsBidirectional>
     void
     graph<VertexT, EdgeT, IsBidirectional>::remove_out_edges(VertexT const& value) {

lib/pkgxx/pkgdb.hxx

@@ -206,8 +206,8 @@ namespace pkgxx {
 
     /// Obtain the set of \c \@blddep entries of an installed package. \c
     /// Name must either be a \ref pkgxx::pkgbase or \ref
-    /// pkgxx::pkgname. This includes \c BUILD_DEPENDS and \c DEPENDS but
-    /// not \c TOOL_DEPENDS.
+    /// pkgxx::pkgname. This includes \c BOOTSTRAP_DEPENDS, \c
+    /// BUILD_DEPENDS, and \c DEPENDS but not \c TOOL_DEPENDS.
     template <typename Name>
     inline std::set<pkgxx::pkgname>
     build_depends(std::string const& PKG_INFO, Name const& name) {

src/pkg_chk/environment.hxx

@@ -2,7 +2,6 @@
 
 #include <filesystem>
 #include <future>
-#include <optional>
 #include <set>
 #include <string>
 

src/pkg_rr/environment.cxx

@@ -8,6 +8,7 @@ namespace fs = std::filesystem;
 
 namespace {
     struct makefile_env {
+        std::optional<pkgxx::pkgbase> FETCH_USING;
         std::string PKG_ADMIN;
         std::string PKG_INFO;
         std::string SU_CMD;
@@ -39,6 +40,8 @@ namespace pkg_rr {
                           });
                 }
                 std::vector<std::string> vars = {
+                    "FETCH_USING",
+                    "PKG_ADMIN",
                     "PKG_INFO",
                     "SU_CMD"
                 };
@@ -53,13 +56,15 @@ namespace pkg_rr {
                 for (auto const& [var, value]: value_of) {
                     verbose_var(opts, var, value);
                 }
-                _menv.PKG_ADMIN = value_of["PKG_ADMIN"].empty() ? CFG_PKG_ADMIN : value_of["PKG_ADMIN"];
-                _menv.PKG_INFO  = value_of["PKG_INFO" ].empty() ? CFG_PKG_INFO  : value_of["PKG_INFO" ];
-                _menv.SU_CMD    = value_of["SU_CMD"   ];
+                _menv.FETCH_USING = value_of["FETCH_USING"].empty() ? std::nullopt  : std::make_optional(value_of["FETCH_USING"]);
+                _menv.PKG_ADMIN   = value_of["PKG_ADMIN"  ].empty() ? CFG_PKG_ADMIN : value_of["PKG_ADMIN"];
+                _menv.PKG_INFO    = value_of["PKG_INFO"   ].empty() ? CFG_PKG_INFO  : value_of["PKG_INFO" ];
+                _menv.SU_CMD      = value_of["SU_CMD"     ];
                 return _menv;
             }).share();
-        PKG_ADMIN = std::async(std::launch::deferred, [menv]() { return menv.get().PKG_ADMIN; }).share();
-        PKG_INFO  = std::async(std::launch::deferred, [menv]() { return menv.get().PKG_INFO;  }).share();
-        SU_CMD    = std::async(std::launch::deferred, [menv]() { return menv.get().SU_CMD;    }).share();
+        FETCH_USING = std::async(std::launch::deferred, [menv]() { return menv.get().FETCH_USING; }).share();
+        PKG_ADMIN   = std::async(std::launch::deferred, [menv]() { return menv.get().PKG_ADMIN;   }).share();
+        PKG_INFO    = std::async(std::launch::deferred, [menv]() { return menv.get().PKG_INFO;    }).share();
+        SU_CMD      = std::async(std::launch::deferred, [menv]() { return menv.get().SU_CMD;      }).share();
     }
 }

src/pkg_rr/environment.hxx

@@ -1,8 +1,10 @@
 #pragma once
 
 #include <future>
+#include <optional>
 
 #include <pkgxx/environment.hxx>
+#include <pkgxx/pkgname.hxx>
 
 #include "options.hxx"
 
@@ -17,6 +19,7 @@ namespace pkg_rr {
     struct environment: public pkgxx::environment {
         environment(pkg_rr::options const& opts);
 
+        std::shared_future<std::optional<pkgxx::pkgbase>> FETCH_USING;
         std::shared_future<std::string> PKG_ADMIN;
         std::shared_future<std::string> PKG_INFO;
         std::shared_future<std::string> SU_CMD;

src/pkg_rr/replacer.cxx

@@ -444,6 +444,16 @@ namespace pkg_rr {
             to_scan = std::move(*(scheduled.lock()));
         }
 
+        // Now we have a graph of @blddep entries, which includes not only
+        // BUILD_DEPENDS and DEPENDS but also BOOTSTRAP_DEPENDS. The
+        // problem is that FETCH_USING also shows up in BOOTSTRAP_DEPENDS
+        // and creates cycles, so we must remove every edge that goes into
+        // it. Don't worry, if anything BUILD_DEPENDS or DEPENDS on it,
+        // such edges will be discovered later in the "new depends" phase.
+        if (auto const FETCH_USING = env.FETCH_USING.get(); FETCH_USING) {
+            depgraph.lock()->remove_in_edges(FETCH_USING.value());
+        }
+
         return std::move(*(depgraph.lock()));
     }