libutil/topo-sort: return std::variant<std::vector<T>, Cycle>

The variant has on the left-hand side the topologically sorted vector
and the right-hand side is a pair showing the path and its parent that
represent a cycle in the graph making the sort impossible.

The goal is to implement #551 which needs to throw an error if the
topo-sort fails. However, the error-message is supposed to contain a
graph of store-paths and the API to generate this is inherently async.

Now, catching the exception and re-throwing another one is impossible
since `co_await` is forbidden in `catch`-blocks and adding another
topoSort variant that allows an async `makeError` also seems odd. Hence,
I decided to alter the data-structure in use a bit for this use-case.
One out of two uses of the function are affected after all.

Change-Id: I70a987f470437df8beb3b1cc203ff88701d0aa1b

lix/libstore/build/local-derivation-goal.cc

@@ -1786,35 +1786,52 @@ try {
         outputStats.insert_or_assign(outputName, std::move(st));
     }
 
-    auto sortedOutputNames = topoSort(outputsToSort,
-        {[&](const std::string & name) {
-            auto orifu = get(outputReferencesIfUnregistered, name);
-            if (!orifu)
-                throw BuildError(
-                    "no output reference for '%s' in build of '%s'",
-                    name, worker.store.printStorePath(drvPath));
-            return std::visit(overloaded {
-                /* Since we'll use the already installed versions of these, we
-                   can treat them as leaves and ignore any references they
-                   have. */
-                [&](const AlreadyRegistered &) { return StringSet {}; },
-                [&](const PerhapsNeedToRegister & refs) {
-                    StringSet referencedOutputs;
-                    /* FIXME build inverted map up front so no quadratic waste here */
-                    for (auto & r : refs.refs)
-                        for (auto & [o, p] : scratchOutputs)
-                            if (r == p)
-                                referencedOutputs.insert(o);
-                    return referencedOutputs;
-                },
-            }, *orifu);
-        }},
-        {[&](const std::string & path, const std::string & parent) {
+    auto topoSortedOutputs =
+        topoSort(outputsToSort, {[&](const std::string & name) {
+                     auto orifu = get(outputReferencesIfUnregistered, name);
+                     if (!orifu) {
+                         throw BuildError(
+                             "no output reference for '%s' in build of '%s'",
+                             name,
+                             worker.store.printStorePath(drvPath)
+                         );
+                     }
+                     return std::visit(
+                         overloaded{
+                             /* Since we'll use the already installed versions of these, we
+                                can treat them as leaves and ignore any references they
+                                have. */
+                             [&](const AlreadyRegistered &) { return StringSet{}; },
+                             [&](const PerhapsNeedToRegister & refs) {
+                                 StringSet referencedOutputs;
+                                 /* FIXME build inverted map up front so no quadratic waste here */
+                                 for (auto & r : refs.refs) {
+                                     for (auto & [o, p] : scratchOutputs) {
+                                         if (r == p) {
+                                             referencedOutputs.insert(o);
+                                         }
+                                     }
+                                 }
+                                 return referencedOutputs;
+                             },
+                         },
+                         *orifu
+                     );
+                 }});
+
+    auto sortedOutputNames = std::visit(overloaded {
+        [&](Cycle<std::string> & cycle) -> std::vector<std::string> {
             // TODO with more -vvvv also show the temporary paths for manual inspection.
-            return BuildError(
-                "cycle detected in build of '%s' in the references of output '%s' from output '%s'",
-                worker.store.printStorePath(drvPath), path, parent);
-        }});
+            throw BuildError(
+                "cycle detected in build of '%s' in the references of output '%s' from output "
+                "'%s'",
+                worker.store.printStorePath(drvPath),
+                cycle.path,
+                cycle.parent
+            );
+        },
+        [&](auto & r) { return r; }
+    }, topoSortedOutputs);
 
     std::reverse(sortedOutputNames.begin(), sortedOutputNames.end());
 

lix/libstore/local-store.cc

@@ -1025,17 +1025,22 @@ try {
                error if a cycle is detected and roll back the
                transaction.  Cycles can only occur when a derivation
                has multiple outputs. */
-            topoSort(paths,
-                {[&](const StorePath & path) {
-                    auto i = infos.find(path);
-                    return i == infos.end() ? StorePathSet() : i->second.references;
-                }},
-                {[&](const StorePath & path, const StorePath & parent) {
-                    return BuildError(
-                        "cycle detected in the references of '%s' from '%s'",
-                        printStorePath(path),
-                        printStorePath(parent));
-                }});
+            std::visit(
+                overloaded{
+                    [&](const std::vector<StorePath> &) {},
+                    [&](const Cycle<StorePath> & cycle) {
+                        throw BuildError(
+                            "cycle detected in the references of '%s' from '%s'",
+                            printStorePath(cycle.path),
+                            printStorePath(cycle.parent)
+                        );
+                    }
+                },
+                topoSort(paths, {[&](const StorePath & path) {
+                             auto i = infos.find(path);
+                             return i == infos.end() ? StorePathSet() : i->second.references;
+                         }})
+            );
 
             txn.commit();
             co_return result::success();

lix/libutil/topo-sort.hh

@@ -8,36 +8,56 @@
 namespace nix {
 
 template<typename T>
-std::vector<T> topoSort(std::set<T> items,
-        std::function<std::set<T>(const T &)> getChildren,
-        std::function<Error(const T &, const T &)> makeCycleError)
+struct Cycle
+{
+    T path;
+    T parent;
+};
+
+template<typename T>
+using TopoSortResult = std::variant<std::vector<T>, Cycle<T>>;
+
+template<typename T>
+TopoSortResult<T> topoSort(std::set<T> items, std::function<std::set<T>(const T &)> getChildren)
 {
     std::vector<T> sorted;
     std::set<T> visited, parents;
 
-    std::function<void(const T & path, const T * parent)> dfsVisit;
+    std::function<std::optional<Cycle<T>>(const T & path, const T * parent)> dfsVisit;
 
-    dfsVisit = [&](const T & path, const T * parent) {
+    dfsVisit = [&](const T & path, const T * parent) -> std::optional<Cycle<T>> {
         if (parents.count(path)) {
-            throw makeCycleError(path, *parent); // NOLINT(lix-foreign-exceptions): type dependent
+            return Cycle{path, *parent};
         }
 
-        if (!visited.insert(path).second) return;
+        if (!visited.insert(path).second) {
+            return std::nullopt;
+        }
         parents.insert(path);
 
         std::set<T> references = getChildren(path);
 
         for (auto & i : references)
             /* Don't traverse into items that don't exist in our starting set. */
-            if (i != path && items.count(i))
-                dfsVisit(i, &path);
+            if (i != path && items.count(i)) {
+                auto result = dfsVisit(i, &path);
+                if (result.has_value()) {
+                    return result;
+                }
+            }
 
         sorted.push_back(path);
         parents.erase(path);
+
+        return std::nullopt;
     };
 
-    for (auto & i : items)
-        dfsVisit(i, nullptr);
+    for (auto & i : items) {
+        auto cycle = dfsVisit(i, nullptr);
+        if (cycle.has_value()) {
+            return *cycle;
+        }
+    }
 
     std::reverse(sorted.begin(), sorted.end());
 

tests/unit/libutil/topo-sort.cc

@@ -0,0 +1,45 @@
+#include "lix/libutil/topo-sort.hh"
+#include <gtest/gtest.h>
+
+namespace nix {
+static auto testToposort(const std::map<std::string, std::set<std::string>> & data)
+{
+    std::set<std::string> keys;
+    for (auto & [k, _] : data) {
+        keys.insert(k);
+    }
+
+    return topoSort(keys, {[&](const std::string & lib) { return data.at(lib); }});
+}
+
+TEST(toposort, trivial)
+{
+    // The dependencies are incomplete on purpose here, this is just a test-case.
+    auto result = testToposort(
+        {{"openssh", {"glibc", "zlib", "polkit"}},
+         {"zlib", {"glibc"}},
+         {"polkit", {"glibc", "pam"}},
+         {"pam", {"glibc"}},
+         {"glibc", {}}}
+    );
+
+    auto ordered = std::get<std::vector<std::string>>(result);
+    ASSERT_EQ(5, ordered.size());
+
+    ASSERT_EQ("openssh", ordered[0]);
+    ASSERT_EQ("zlib", ordered[1]);
+    ASSERT_EQ("polkit", ordered[2]);
+    ASSERT_EQ("pam", ordered[3]);
+    ASSERT_EQ("glibc", ordered[4]);
+}
+
+TEST(toposort, cycle)
+{
+    auto result = testToposort({{"foo", {"bar"}}, {"bar", {"baz"}}, {"baz", {"foo"}}});
+
+    auto cycle = std::get<Cycle<std::string>>(result);
+
+    ASSERT_EQ(cycle.path, "bar");
+    ASSERT_EQ(cycle.parent, "foo");
+}
+}

tests/unit/meson.build

@@ -74,6 +74,7 @@ libutil_tests_sources = files(
   'libutil/terminal.cc',
   'libutil/tests.cc',
   'libutil/thread-pool.cc',
+  'libutil/topo-sort.cc',
   'libutil/url-name.cc',
   'libutil/url.cc',
   'libutil/xml-writer.cc',