Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 10, 2025
+427 −77
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 16 additions & 13 deletions src/libstore/local-store.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
});
Expand Down
35 changes: 19 additions & 16 deletions src/libstore/misc.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
73 changes: 38 additions & 35 deletions src/libstore/unix/build/derivation-builder.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Member

@roberth roberth Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking: too complicated name. Refactor might improve that by moving to a smaller scope, but no concrete suggestions.

Copy link
Member

@Ericson2314 Ericson2314 Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(existing name it looks like)

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 */
Copy link
Member

@roberth roberth Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FIXME
Pre-existing 🫠

Copy link
Member Author

@lovesegfault lovesegfault Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The next PR has a fix for that :)

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());

Expand Down
1 change: 1 addition & 0 deletions src/libutil-tests/meson.build
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ sources = files(
'strings.cc',
'suggestions.cc',
'terminal.cc',
'topo-sort.cc',
'url.cc',
'util.cc',
'xml-writer.cc',
Expand Down
Loading
Loading