diff --git a/lix/libstore/build/local-derivation-goal.cc b/lix/libstore/build/local-derivation-goal.cc index 0fc2deb3a..543cafc66 100644 --- a/lix/libstore/build/local-derivation-goal.cc +++ b/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 & cycle) -> std::vector { // 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()); diff --git a/lix/libstore/local-store.cc b/lix/libstore/local-store.cc index 5373f175b..8978d3ae8 100644 --- a/lix/libstore/local-store.cc +++ b/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 &) {}, + [&](const Cycle & 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(); diff --git a/lix/libutil/topo-sort.hh b/lix/libutil/topo-sort.hh index f4b7fd5ee..2cc31c9ba 100644 --- a/lix/libutil/topo-sort.hh +++ b/lix/libutil/topo-sort.hh @@ -8,36 +8,56 @@ namespace nix { template -std::vector topoSort(std::set items, - std::function(const T &)> getChildren, - std::function makeCycleError) +struct Cycle +{ + T path; + T parent; +}; + +template +using TopoSortResult = std::variant, Cycle>; + +template +TopoSortResult topoSort(std::set items, std::function(const T &)> getChildren) { std::vector sorted; std::set visited, parents; - std::function dfsVisit; + std::function>(const T & path, const T * parent)> dfsVisit; - dfsVisit = [&](const T & path, const T * parent) { + dfsVisit = [&](const T & path, const T * parent) -> std::optional> { 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 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()); diff --git a/tests/unit/libutil/topo-sort.cc b/tests/unit/libutil/topo-sort.cc new file mode 100644 index 000000000..6798043bd --- /dev/null +++ b/tests/unit/libutil/topo-sort.cc @@ -0,0 +1,45 @@ +#include "lix/libutil/topo-sort.hh" +#include + +namespace nix { +static auto testToposort(const std::map> & data) +{ + std::set 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>(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>(result); + + ASSERT_EQ(cycle.path, "bar"); + ASSERT_EQ(cycle.parent, "foo"); +} +} diff --git a/tests/unit/meson.build b/tests/unit/meson.build index 7f1197afc..04fa7b0a4 100644 --- a/tests/unit/meson.build +++ b/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',