Skip to content

Commit eeec0c2

Browse files
lovesegfaultlovesegfault
committed
feat(libstore): add detailed cycle detection for build errors
Implements comprehensive cycle detection that shows users exactly which files create circular dependencies when a build fails. This dramatically improves the developer experience when debugging dependency cycles.
1 parent 56dbca9 commit eeec0c2

File tree

1 file changed

+168
-38
lines changed

1 file changed

+168
-38
lines changed

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

Lines changed: 168 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,10 @@
44
#include "nix/util/processes.hh"
55
#include "nix/store/builtins.hh"
66
#include "nix/store/path-references.hh"
7+
#include "nix/store/dependency-graph.hh"
78
#include "nix/util/finally.hh"
9+
10+
#include <ranges>
811
#include "nix/util/util.hh"
912
#include "nix/util/archive.hh"
1013
#include "nix/util/git.hh"
@@ -62,6 +65,102 @@ struct NotDeterministic : BuildError
6265
}
6366
};
6467

68+
/**
69+
* Information about an output path for cycle analysis.
70+
*/
71+
struct OutputPathInfo
72+
{
73+
std::string outputName; ///< Name of the output (e.g., "out", "dev")
74+
StorePath storePath; ///< The StorePath of this output
75+
Path actualPath; ///< Actual filesystem path where the output is located
76+
};
77+
78+
/**
79+
* Helper to analyze cycles and throw a detailed error.
80+
*
81+
* This function always throws - either the detailed cycle error or re-throws
82+
* the original exception if no cycles are found.
83+
*
84+
* @param drvName The formatted name of the derivation being built
85+
* @param referenceablePaths Set of paths to search for in the outputs
86+
* @param getOutputPaths Callback returning output information to scan
87+
*/
88+
[[noreturn]] static void analyzeCyclesAndThrow(
89+
std::string_view drvName,
90+
const StorePathSet & referenceablePaths,
91+
std::function<std::vector<OutputPathInfo>()> getOutputPaths)
92+
{
93+
debug("cycle detected, analyzing for detailed error report");
94+
95+
// Scan all outputs and build dependency graph
96+
auto accessor = getFSSourceAccessor();
97+
DependencyGraph<StorePath, FileListEdgeProperty> depGraph;
98+
99+
for (auto & output : getOutputPaths()) {
100+
debug("scanning output '%s' at path '%s' for cycles", output.outputName, output.actualPath);
101+
auto outputGraph = buildStorePathGraphFromScan(*accessor, CanonPath(output.actualPath), output.storePath, referenceablePaths);
102+
103+
// Merge into combined graph
104+
for (auto & node : outputGraph.getAllNodes()) {
105+
for (auto & successor : outputGraph.getSuccessors(node)) {
106+
auto edgeProp = outputGraph.getEdgeProperty(node, successor);
107+
if (edgeProp) {
108+
depGraph.addEdge(node, successor, *edgeProp);
109+
}
110+
}
111+
}
112+
}
113+
114+
// Find cycles in the combined graph
115+
auto cycles = depGraph.findCycles();
116+
117+
if (cycles.empty()) {
118+
debug("no detailed cycles found, re-throwing original error");
119+
throw;
120+
}
121+
122+
debug("found %lu cycles", cycles.size());
123+
124+
// Build detailed error message with file annotations
125+
std::string cycleDetails = fmt("Detailed cycle analysis found %d cycle path(s):", cycles.size());
126+
127+
for (const auto & [idx, cycle] : std::views::enumerate(cycles)) {
128+
cycleDetails += fmt("\n\nCycle %d:", idx + 1);
129+
130+
for (const auto & window : cycle | std::views::slide(2)) {
131+
const StorePath & from = window[0];
132+
const StorePath & to = window[1];
133+
134+
cycleDetails += fmt("\n → %s", from.to_string());
135+
136+
// Add file annotations
137+
auto edgeProp = depGraph.getEdgeProperty(from, to);
138+
if (edgeProp && !edgeProp->files.empty()) {
139+
for (const auto & file : edgeProp->files) {
140+
cycleDetails += fmt("\n (via %s)", file.abs());
141+
}
142+
}
143+
}
144+
145+
// Close the cycle
146+
if (!cycle.empty()) {
147+
cycleDetails += fmt("\n → %s", cycle.back().to_string());
148+
}
149+
}
150+
151+
cycleDetails +=
152+
fmt("\n\nThis means there are circular references between output files.\n"
153+
"The build cannot proceed because the outputs reference each other.");
154+
155+
// Add hint with temp paths for debugging
156+
if (settings.keepFailed || verbosity >= lvlDebug) {
157+
cycleDetails += "\n\nNote: Temporary build outputs are preserved for inspection.";
158+
}
159+
160+
throw BuildError(
161+
BuildResult::Failure::OutputRejected, "cycle detected in build of '%s': %s", drvName, cycleDetails);
162+
}
163+
65164
/**
66165
* This class represents the state for building locally.
67166
*
@@ -1473,43 +1572,62 @@ SingleDrvOutputs DerivationBuilderImpl::registerOutputs()
14731572
outputStats.insert_or_assign(outputName, std::move(st));
14741573
}
14751574

1476-
auto sortedOutputNames = topoSort(
1477-
outputsToSort,
1478-
{[&](const std::string & name) {
1479-
auto orifu = get(outputReferencesIfUnregistered, name);
1480-
if (!orifu)
1481-
throw BuildError(
1482-
BuildResult::Failure::OutputRejected,
1483-
"no output reference for '%s' in build of '%s'",
1484-
name,
1485-
store.printStorePath(drvPath));
1486-
return std::visit(
1487-
overloaded{
1488-
/* Since we'll use the already installed versions of these, we
1489-
can treat them as leaves and ignore any references they
1490-
have. */
1491-
[&](const AlreadyRegistered &) { return StringSet{}; },
1492-
[&](const PerhapsNeedToRegister & refs) {
1493-
StringSet referencedOutputs;
1494-
/* FIXME build inverted map up front so no quadratic waste here */
1495-
for (auto & r : refs.refs)
1496-
for (auto & [o, p] : scratchOutputs)
1497-
if (r == p)
1498-
referencedOutputs.insert(o);
1499-
return referencedOutputs;
1500-
},
1501-
},
1502-
*orifu);
1503-
}},
1504-
{[&](const std::string & path, const std::string & parent) {
1505-
// TODO with more -vvvv also show the temporary paths for manual inspection.
1506-
return BuildError(
1507-
BuildResult::Failure::OutputRejected,
1508-
"cycle detected in build of '%s' in the references of output '%s' from output '%s'",
1509-
store.printStorePath(drvPath),
1510-
path,
1511-
parent);
1512-
}});
1575+
auto sortedOutputNames = [&]() {
1576+
try {
1577+
return topoSort(
1578+
outputsToSort,
1579+
{[&](const std::string & name) {
1580+
auto orifu = get(outputReferencesIfUnregistered, name);
1581+
if (!orifu)
1582+
throw BuildError(
1583+
BuildResult::Failure::OutputRejected,
1584+
"no output reference for '%s' in build of '%s'",
1585+
name,
1586+
store.printStorePath(drvPath));
1587+
return std::visit(
1588+
overloaded{
1589+
/* Since we'll use the already installed versions of these, we
1590+
can treat them as leaves and ignore any references they
1591+
have. */
1592+
[&](const AlreadyRegistered &) { return StringSet{}; },
1593+
[&](const PerhapsNeedToRegister & refs) {
1594+
StringSet referencedOutputs;
1595+
/* FIXME build inverted map up front so no quadratic waste here */
1596+
for (auto & r : refs.refs)
1597+
for (auto & [o, p] : scratchOutputs)
1598+
if (r == p)
1599+
referencedOutputs.insert(o);
1600+
return referencedOutputs;
1601+
},
1602+
},
1603+
*orifu);
1604+
}},
1605+
{[&](const std::string & path, const std::string & parent) {
1606+
// TODO with more -vvvv also show the temporary paths for manual inspection.
1607+
return BuildError(
1608+
BuildResult::Failure::OutputRejected,
1609+
"cycle detected in build of '%s' in the references of output '%s' from output '%s'",
1610+
store.printStorePath(drvPath),
1611+
path,
1612+
parent);
1613+
}});
1614+
} catch (std::exception & e) {
1615+
analyzeCyclesAndThrow(store.printStorePath(drvPath), referenceablePaths, [&]() {
1616+
std::vector<OutputPathInfo> outputPaths;
1617+
for (auto & [outputName, _] : drv.outputs) {
1618+
auto scratchOutput = get(scratchOutputs, outputName);
1619+
if (scratchOutput) {
1620+
outputPaths.push_back(
1621+
OutputPathInfo{
1622+
.outputName = outputName,
1623+
.storePath = *scratchOutput,
1624+
.actualPath = realPathInSandbox(store.printStorePath(*scratchOutput))});
1625+
}
1626+
}
1627+
return outputPaths;
1628+
});
1629+
}
1630+
}();
15131631

15141632
std::reverse(sortedOutputNames.begin(), sortedOutputNames.end());
15151633

@@ -1848,12 +1966,24 @@ SingleDrvOutputs DerivationBuilderImpl::registerOutputs()
18481966
/* Register each output path as valid, and register the sets of
18491967
paths referenced by each of them. If there are cycles in the
18501968
outputs, this will fail. */
1851-
{
1969+
try {
18521970
ValidPathInfos infos2;
18531971
for (auto & [outputName, newInfo] : infos) {
18541972
infos2.insert_or_assign(newInfo.path, newInfo);
18551973
}
18561974
store.registerValidPaths(infos2);
1975+
} catch (BuildError & e) {
1976+
analyzeCyclesAndThrow(store.printStorePath(drvPath), referenceablePaths, [&]() {
1977+
std::vector<OutputPathInfo> outputPaths;
1978+
for (auto & [outputName, newInfo] : infos) {
1979+
outputPaths.push_back(
1980+
OutputPathInfo{
1981+
.outputName = outputName,
1982+
.storePath = newInfo.path,
1983+
.actualPath = store.toRealPath(store.printStorePath(newInfo.path))});
1984+
}
1985+
return outputPaths;
1986+
});
18571987
}
18581988

18591989
/* If we made it this far, we are sure the output matches the

0 commit comments

Comments
 (0)