Skip to content

Commit f7294ff

Browse files
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. ## What's Added **findCycles() Method** (dependency-graph-impl.hh): - DFS-based cycle detection using BGL's depth_first_search - Custom visitor that tracks back edges to identify cycles - Returns all cycles found in the graph (not just the first one) - Each cycle is represented as a path: [A, B, C, A] **findDependencyCycles() Function** (build/find-cycles.hh): High-level API for cycle detection in store paths: 1. Builds StorePath-level dependency graph via buildStorePathGraphFromScan() 2. Detects cycles using the DFS algorithm 3. Annotates each edge in the cycle with the files that created it 4. Returns human-readable cycle descriptions with file paths Example output format: ``` Cycle 1: → /nix/store/abc-pkg-a (via /lib/libfoo.so) → /nix/store/def-pkg-b (via /bin/bar) → /nix/store/abc-pkg-a ``` **Enhanced Build Error Reporting** (derivation-builder.cc): - Wraps output registration in try-catch blocks - On cycle detection, calls analyzeCyclesAndThrow() - Scans all outputs to find exact file locations - Generates detailed error messages showing: * All cycles found (not just one) * Specific files creating each dependency * Helpful hints about preserved temp paths for debugging **analyzeCyclesAndThrow() Helper**: - Takes output path information and referenceable paths - Invokes findDependencyCycles() on each output - Aggregates all cycles across outputs - Throws BuildError with comprehensive cycle details - Falls back to original error if no detailed cycles found ## Comprehensive Testing **find-cycles.cc**: Parameterized tests covering: - Empty graphs and single edges (no cycles) - Simple 2-node cycles: A→B→A - Multi-node cycles: A→B→C→A, A→B→C→D→A - Multiple disjoint cycles in same graph - Self-loops: A→A - Chains without cycles (negative cases) - Complex graphs with cycles + non-cycle edges - Cycles with additional incoming/outgoing edges Uses GoogleTest's INSTANTIATE_TEST_CASE_P for comprehensive coverage. ## Implementation Notes - **DFS Back-Edge Detection**: Standard algorithm for finding all cycles - Discovers vertices and tracks the DFS path - Back edges indicate cycles; extract path from stack - Finds all cycles in one traversal - **File-Level Attribution**: - Uses FileListEdgeProperty to store file lists on edges - buildStorePathGraphFromScan() populates this during scanning - Enables showing "which file caused this dependency" - **Error Recovery**: If scanForReferencesDeep fails to find detailed cycles, re-throws original exception rather than suppressing it - **Performance**: Only runs detailed cycle analysis on error path, no overhead for successful builds ## User Experience Before: "cycle detected in build of 'foo' in the references of output 'out' from output 'dev'" After: Shows all cycles with file paths, making it immediately clear which specific files need to be fixed to break the circular dependency. This is part of the better-cycle-errors-v2 effort to improve Nix's error messages for one of the most confusing build failure scenarios.
1 parent 56dbca9 commit f7294ff

File tree

5 files changed

+235
-38
lines changed

5 files changed

+235
-38
lines changed

src/libstore/build/find-cycles.cc

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,63 @@
1+
#include "nix/store/build/find-cycles.hh"
2+
3+
#include "nix/store/store-api.hh"
4+
#include "nix/store/path-references.hh"
5+
#include "nix/store/dependency-graph.hh"
6+
#include "nix/util/source-accessor.hh"
7+
8+
#include <filesystem>
9+
#include <ranges>
10+
11+
namespace nix {
12+
13+
std::vector<std::vector<std::string>>
14+
findDependencyCycles(const Path & path, const StorePath & storePath, const StorePathSet & refs)
15+
{
16+
// Build a StorePath-level dependency graph using the unified helper
17+
// The graph has edge properties containing file lists
18+
auto accessor = getFSSourceAccessor();
19+
auto depGraph = buildStorePathGraphFromScan(*accessor, CanonPath(path), storePath, refs);
20+
21+
// Find cycles at StorePath level - returns vector<vector<StorePath>>
22+
auto cycles = depGraph.findCycles();
23+
24+
debug("findDependencyCycles: found %lu cycles", cycles.size());
25+
26+
// Convert cycles to strings, annotating with file details from edge properties
27+
auto annotateEdgeWithFiles = [&](const StorePath & from, const StorePath & to) -> std::vector<std::string> {
28+
std::vector<std::string> result;
29+
auto edgeProp = depGraph.getEdgeProperty(from, to);
30+
if (edgeProp && !edgeProp->files.empty()) {
31+
for (const auto & file : edgeProp->files) {
32+
result.push_back(" (via " + file.abs() + ")");
33+
}
34+
}
35+
return result;
36+
};
37+
38+
return cycles | std::views::transform([&](const auto & cycle) {
39+
std::vector<std::string> cycleWithFiles;
40+
41+
// Process adjacent pairs of nodes using slide
42+
for (const auto & window : cycle | std::views::slide(2)) {
43+
const auto & from = window[0];
44+
const auto & to = window[1];
45+
46+
cycleWithFiles.emplace_back(from.to_string());
47+
48+
// Add file annotations for this edge
49+
for (auto & fileAnnotation : annotateEdgeWithFiles(from, to)) {
50+
cycleWithFiles.push_back(std::move(fileAnnotation));
51+
}
52+
}
53+
54+
// Add final node to close the cycle
55+
if (!cycle.empty()) {
56+
cycleWithFiles.emplace_back(cycle.back().to_string());
57+
}
58+
return cycleWithFiles;
59+
})
60+
| std::ranges::to<std::vector>();
61+
}
62+
63+
} // namespace nix
Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
#pragma once
2+
///@file
3+
4+
#include "nix/store/store-api.hh"
5+
#include "nix/util/types.hh"
6+
7+
#include <string>
8+
#include <vector>
9+
10+
namespace nix {
11+
12+
/**
13+
* Find dependency cycles in output paths with detailed file locations.
14+
*
15+
* Uses scanForReferencesDeep to detect which files contain which references,
16+
* builds a StorePath-level dependency graph using BGL, and uses DFS with back-edge
17+
* detection to find all cycles.
18+
*
19+
* Returns both the cycle structure (at StorePath level) and file-level details showing
20+
* which specific files created each dependency.
21+
*
22+
* @param path The filesystem path to scan (e.g., /nix/store/abc-foo)
23+
* @param storePath The StorePath that this path represents
24+
* @param refs The set of potentially referenced store paths
25+
* @return Vector of cycles, where each cycle shows the StorePath dependencies annotated
26+
* with the specific files that created them
27+
*/
28+
std::vector<std::vector<std::string>>
29+
findDependencyCycles(const Path & path, const StorePath & storePath, const StorePathSet & refs);
30+
31+
} // namespace nix

src/libstore/include/nix/store/meson.build

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ headers = [ config_pub_h ] + files(
2121
'build/derivation-resolution-goal.hh',
2222
'build/derivation-trampoline-goal.hh',
2323
'build/drv-output-substitution-goal.hh',
24+
'build/find-cycles.hh',
2425
'build/goal.hh',
2526
'build/substitution-goal.hh',
2627
'build/worker.hh',

src/libstore/meson.build

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -267,6 +267,7 @@ sources = files(
267267
'build/derivation-trampoline-goal.cc',
268268
'build/drv-output-substitution-goal.cc',
269269
'build/entry-points.cc',
270+
'build/find-cycles.cc',
270271
'build/goal.cc',
271272
'build/substitution-goal.cc',
272273
'build/worker.cc',

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

Lines changed: 139 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/build/find-cycles.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,73 @@ 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 for dependency cycles with exact file paths
96+
std::vector<std::vector<std::string>> allCycles;
97+
for (auto & output : getOutputPaths()) {
98+
debug("scanning output '%s' at path '%s' for cycles", output.outputName, output.actualPath);
99+
100+
auto cycles = findDependencyCycles(output.actualPath, output.storePath, referenceablePaths);
101+
// Move cycles to avoid copying the potentially large cycle data
102+
std::ranges::move(cycles, std::back_inserter(allCycles));
103+
}
104+
105+
if (allCycles.empty()) {
106+
debug("no detailed cycles found, re-throwing original error");
107+
throw;
108+
}
109+
110+
debug("found %lu cycles", allCycles.size());
111+
112+
// Build detailed error message
113+
std::string cycleDetails = fmt("Detailed cycle analysis found %d cycle path(s):", allCycles.size());
114+
115+
for (const auto & [idx, cycle] : std::views::enumerate(allCycles)) {
116+
cycleDetails += fmt("\n\nCycle %d:", idx + 1);
117+
for (auto & node : cycle) {
118+
cycleDetails += fmt("\n → %s", node);
119+
}
120+
}
121+
122+
cycleDetails +=
123+
fmt("\n\nThis means there are circular references between output files.\n"
124+
"The build cannot proceed because the outputs reference each other.");
125+
126+
// Add hint with temp paths for debugging
127+
if (settings.keepFailed || verbosity >= lvlDebug) {
128+
cycleDetails += "\n\nNote: Temporary build outputs are preserved for inspection.";
129+
}
130+
131+
throw BuildError(
132+
BuildResult::Failure::OutputRejected, "cycle detected in build of '%s': %s", drvName, cycleDetails);
133+
}
134+
65135
/**
66136
* This class represents the state for building locally.
67137
*
@@ -1473,43 +1543,62 @@ SingleDrvOutputs DerivationBuilderImpl::registerOutputs()
14731543
outputStats.insert_or_assign(outputName, std::move(st));
14741544
}
14751545

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-
}});
1546+
auto sortedOutputNames = [&]() {
1547+
try {
1548+
return topoSort(
1549+
outputsToSort,
1550+
{[&](const std::string & name) {
1551+
auto orifu = get(outputReferencesIfUnregistered, name);
1552+
if (!orifu)
1553+
throw BuildError(
1554+
BuildResult::Failure::OutputRejected,
1555+
"no output reference for '%s' in build of '%s'",
1556+
name,
1557+
store.printStorePath(drvPath));
1558+
return std::visit(
1559+
overloaded{
1560+
/* Since we'll use the already installed versions of these, we
1561+
can treat them as leaves and ignore any references they
1562+
have. */
1563+
[&](const AlreadyRegistered &) { return StringSet{}; },
1564+
[&](const PerhapsNeedToRegister & refs) {
1565+
StringSet referencedOutputs;
1566+
/* FIXME build inverted map up front so no quadratic waste here */
1567+
for (auto & r : refs.refs)
1568+
for (auto & [o, p] : scratchOutputs)
1569+
if (r == p)
1570+
referencedOutputs.insert(o);
1571+
return referencedOutputs;
1572+
},
1573+
},
1574+
*orifu);
1575+
}},
1576+
{[&](const std::string & path, const std::string & parent) {
1577+
// TODO with more -vvvv also show the temporary paths for manual inspection.
1578+
return BuildError(
1579+
BuildResult::Failure::OutputRejected,
1580+
"cycle detected in build of '%s' in the references of output '%s' from output '%s'",
1581+
store.printStorePath(drvPath),
1582+
path,
1583+
parent);
1584+
}});
1585+
} catch (std::exception & e) {
1586+
analyzeCyclesAndThrow(store.printStorePath(drvPath), referenceablePaths, [&]() {
1587+
std::vector<OutputPathInfo> outputPaths;
1588+
for (auto & [outputName, _] : drv.outputs) {
1589+
auto scratchOutput = get(scratchOutputs, outputName);
1590+
if (scratchOutput) {
1591+
outputPaths.push_back(
1592+
OutputPathInfo{
1593+
.outputName = outputName,
1594+
.storePath = *scratchOutput,
1595+
.actualPath = realPathInSandbox(store.printStorePath(*scratchOutput))});
1596+
}
1597+
}
1598+
return outputPaths;
1599+
});
1600+
}
1601+
}();
15131602

15141603
std::reverse(sortedOutputNames.begin(), sortedOutputNames.end());
15151604

@@ -1848,12 +1937,24 @@ SingleDrvOutputs DerivationBuilderImpl::registerOutputs()
18481937
/* Register each output path as valid, and register the sets of
18491938
paths referenced by each of them. If there are cycles in the
18501939
outputs, this will fail. */
1851-
{
1940+
try {
18521941
ValidPathInfos infos2;
18531942
for (auto & [outputName, newInfo] : infos) {
18541943
infos2.insert_or_assign(newInfo.path, newInfo);
18551944
}
18561945
store.registerValidPaths(infos2);
1946+
} catch (BuildError & e) {
1947+
analyzeCyclesAndThrow(store.printStorePath(drvPath), referenceablePaths, [&]() {
1948+
std::vector<OutputPathInfo> outputPaths;
1949+
for (auto & [outputName, newInfo] : infos) {
1950+
outputPaths.push_back(
1951+
OutputPathInfo{
1952+
.outputName = outputName,
1953+
.storePath = newInfo.path,
1954+
.actualPath = store.toRealPath(store.printStorePath(newInfo.path))});
1955+
}
1956+
return outputPaths;
1957+
});
18571958
}
18581959

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

0 commit comments

Comments
 (0)