Merge pull request #14479 from lovesegfault/topo-sort-handle-cycles

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

Author: Ericson2314

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

@@ -1470,43 +1470,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',