refactor(libutil/topo-sort): return variant instead of throwing

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.

This change prepares for enhanced cycle error messages that can provide
more context about the cycle. The variant approach allows callers to
handle cycles more flexibly, enabling better error reporting that shows
the full cycle path and which files are involved.

Adapted from Lix commit f7871fcb5.

Change-Id: I70a987f470437df8beb3b1cc203ff88701d0aa1b
Co-Authored-By: Maximilian Bosch <[email protected]>

Author: 2 people

src/libstore/local-store.cc

@@ -989,19 +989,22 @@ void LocalStore::registerValidPaths(const ValidPathInfos & infos)
            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(
-                    BuildResult::Failure::OutputRejected,
-                    "cycle detected in the references of '%s' from '%s'",
-                    printStorePath(path),
-                    printStorePath(parent));
-            }});
+        auto topoSortResult = topoSort(paths, {[&](const StorePath & path) {
+                                           auto i = infos.find(path);
+                                           return i == infos.end() ? StorePathSet() : i->second.references;
+                                       }});
+
+        std::visit(
+            overloaded{
+                [&](const Cycle<StorePath> & cycle) {
+                    throw BuildError(
+                        BuildResult::Failure::OutputRejected,
+                        "cycle detected in the references of '%s' from '%s'",
+                        printStorePath(cycle.path),
+                        printStorePath(cycle.parent));
+                },
+                [](auto &) { /* Success, continue */ }},
+            topoSortResult);
 
         txn.commit();
     });

src/libstore/misc.cc

@@ -311,22 +311,25 @@ MissingPaths Store::queryMissing(const std::vector<DerivedPath> & targets)
 
 StorePaths Store::topoSortPaths(const StorePathSet & paths)
 {
-    return topoSort(
-        paths,
-        {[&](const StorePath & path) {
-            try {
-                return queryPathInfo(path)->references;
-            } catch (InvalidPath &) {
-                return StorePathSet();
-            }
-        }},
-        {[&](const StorePath & path, const StorePath & parent) {
-            return BuildError(
-                BuildResult::Failure::OutputRejected,
-                "cycle detected in the references of '%s' from '%s'",
-                printStorePath(path),
-                printStorePath(parent));
-        }});
+    auto result = topoSort(paths, {[&](const StorePath & path) {
+                               try {
+                                   return queryPathInfo(path)->references;
+                               } catch (InvalidPath &) {
+                                   return StorePathSet();
+                               }
+                           }});
+
+    return std::visit(
+        overloaded{
+            [&](const Cycle<StorePath> & cycle) -> StorePaths {
+                throw BuildError(
+                    BuildResult::Failure::OutputRejected,
+                    "cycle detected in the references of '%s' from '%s'",
+                    printStorePath(cycle.path),
+                    printStorePath(cycle.parent));
+            },
+            [](const auto & sorted) { return sorted; }},
+        result);
 }
 
 std::map<DrvOutput, StorePath>

src/libstore/unix/build/derivation-builder.cc

@@ -1473,43 +1473,46 @@ SingleDrvOutputs DerivationBuilderImpl::registerOutputs()
         outputStats.insert_or_assign(outputName, std::move(st));
     }
 
-    auto sortedOutputNames = topoSort(
-        outputsToSort,
-        {[&](const std::string & name) {
-            auto orifu = get(outputReferencesIfUnregistered, name);
-            if (!orifu)
+    auto topoSortResult = topoSort(outputsToSort, {[&](const std::string & name) {
+                                       auto orifu = get(outputReferencesIfUnregistered, name);
+                                       if (!orifu)
+                                           throw BuildError(
+                                               BuildResult::Failure::OutputRejected,
+                                               "no output reference for '%s' in build of '%s'",
+                                               name,
+                                               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.
                 throw BuildError(
                     BuildResult::Failure::OutputRejected,
-                    "no output reference for '%s' in build of '%s'",
-                    name,
-                    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) {
-            // TODO with more -vvvv also show the temporary paths for manual inspection.
-            return BuildError(
-                BuildResult::Failure::OutputRejected,
-                "cycle detected in build of '%s' in the references of output '%s' from output '%s'",
-                store.printStorePath(drvPath),
-                path,
-                parent);
-        }});
+                    "cycle detected in build of '%s' in the references of output '%s' from output '%s'",
+                    store.printStorePath(drvPath),
+                    cycle.path,
+                    cycle.parent);
+            },
+            [](auto & sorted) { return sorted; }},
+        topoSortResult);
 
     std::reverse(sortedOutputNames.begin(), sortedOutputNames.end());
 

src/libutil-tests/meson.build

@@ -74,6 +74,7 @@ sources = files(
   'strings.cc',
   'suggestions.cc',
   'terminal.cc',
+  'topo-sort.cc',
   'url.cc',
   'util.cc',
   'xml-writer.cc',