Skip to content

Commit dca6833

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 4c04a5e commit dca6833

File tree

7 files changed

+346
-38
lines changed

7 files changed

+346
-38
lines changed

src/libstore-tests/find-cycles.cc

Lines changed: 115 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,115 @@
1+
#include "nix/store/build/find-cycles.hh"
2+
#include "nix/store/dependency-graph.hh"
3+
4+
#include <gtest/gtest.h>
5+
#include <ranges>
6+
7+
namespace nix {
8+
9+
/**
10+
* Parameters for DependencyGraph cycle detection tests
11+
*/
12+
struct FindCyclesParams
13+
{
14+
std::string description;
15+
std::vector<std::pair<std::string, std::string>> inputEdges; // (from, to) pairs
16+
std::vector<std::vector<std::string>> expectedCycles;
17+
18+
friend std::ostream & operator<<(std::ostream & os, const FindCyclesParams & params)
19+
{
20+
os << "Test: " << params.description << "\n";
21+
os << "Input edges (" << params.inputEdges.size() << "):\n";
22+
for (const auto & [from, to] : params.inputEdges) {
23+
os << " " << from << " -> " << to << "\n";
24+
}
25+
os << "Expected cycles (" << params.expectedCycles.size() << "):\n";
26+
for (const auto & cycle : params.expectedCycles) {
27+
os << " ";
28+
for (size_t i = 0; i < cycle.size(); ++i) {
29+
if (i > 0)
30+
os << " -> ";
31+
os << cycle[i];
32+
}
33+
os << "\n";
34+
}
35+
return os;
36+
}
37+
};
38+
39+
class FindCyclesTest : public ::testing::TestWithParam<FindCyclesParams>
40+
{};
41+
42+
namespace {
43+
// Comparator for sorting cycles deterministically
44+
bool compareCycles(const std::vector<std::string> & a, const std::vector<std::string> & b)
45+
{
46+
if (a.size() != b.size())
47+
return a.size() < b.size();
48+
return std::lexicographical_compare(a.begin(), a.end(), b.begin(), b.end());
49+
}
50+
} // namespace
51+
52+
TEST_P(FindCyclesTest, FindCycles)
53+
{
54+
const auto & params = GetParam();
55+
56+
// Build graph from input edges
57+
FilePathGraph depGraph;
58+
for (const auto & [from, to] : params.inputEdges) {
59+
depGraph.addEdge(from, to);
60+
}
61+
62+
// Find cycles - returns vector<vector<string>> directly!
63+
auto actualCycles = depGraph.findCycles();
64+
65+
EXPECT_EQ(actualCycles.size(), params.expectedCycles.size()) << "Number of cycles doesn't match expected";
66+
67+
// Sort both for comparison using ranges (order may vary, but content should match)
68+
std::ranges::sort(actualCycles, compareCycles);
69+
auto expectedCycles = params.expectedCycles;
70+
std::ranges::sort(expectedCycles, compareCycles);
71+
72+
// Compare each cycle
73+
EXPECT_EQ(actualCycles, expectedCycles);
74+
}
75+
76+
INSTANTIATE_TEST_CASE_P(
77+
FindCycles,
78+
FindCyclesTest,
79+
::testing::Values(
80+
// Empty input - no cycles
81+
FindCyclesParams{"empty input", {}, {}},
82+
83+
// Single edge - no cycle
84+
FindCyclesParams{"single edge no cycle", {{"a", "b"}}, {}},
85+
86+
// Simple cycle: A->B, B->A
87+
FindCyclesParams{"simple cycle", {{"a", "b"}, {"b", "a"}}, {{"a", "b", "a"}}},
88+
89+
// Complete cycle: A->B->C->A
90+
FindCyclesParams{"three node cycle", {{"a", "b"}, {"b", "c"}, {"c", "a"}}, {{"a", "b", "c", "a"}}},
91+
92+
// Four node cycle: A->B->C->D->A
93+
FindCyclesParams{
94+
"four node cycle", {{"a", "b"}, {"b", "c"}, {"c", "d"}, {"d", "a"}}, {{"a", "b", "c", "d", "a"}}},
95+
96+
// Multiple disjoint cycles
97+
FindCyclesParams{
98+
"multiple disjoint cycles",
99+
{{"a", "b"}, {"b", "a"}, {"c", "d"}, {"d", "c"}},
100+
{{"a", "b", "a"}, {"c", "d", "c"}}},
101+
102+
// Cycle with non-cycle edges (A->B->A is cycle, C->D is not)
103+
FindCyclesParams{"cycle with extra edges", {{"a", "b"}, {"b", "a"}, {"c", "d"}}, {{"a", "b", "a"}}},
104+
105+
// Self-loop
106+
FindCyclesParams{"self-loop", {{"a", "a"}}, {{"a", "a"}}},
107+
108+
// Chain without cycle
109+
FindCyclesParams{"chain no cycle", {{"a", "b"}, {"b", "c"}, {"c", "d"}}, {}},
110+
111+
// Complex: cycle with incoming/outgoing edges
112+
// X->A->B->C->A (only A->B->C->A is the cycle)
113+
FindCyclesParams{"cycle with tail", {{"x", "a"}, {"a", "b"}, {"b", "c"}, {"c", "a"}}, {{"a", "b", "c", "a"}}}));
114+
115+
} // namespace nix

src/libstore-tests/meson.build

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,7 @@ sources = files(
6262
'derived-path.cc',
6363
'downstream-placeholder.cc',
6464
'dummy-store.cc',
65+
'find-cycles.cc',
6566
'http-binary-cache-store.cc',
6667
'legacy-ssh-store.cc',
6768
'local-binary-cache-store.cc',

src/libstore/build/find-cycles.cc

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,57 @@
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+
for (size_t i = 0; i < cycle.size() - 1; ++i) {
41+
cycleWithFiles.emplace_back(cycle[i].to_string());
42+
43+
// Add file annotations for this edge
44+
for (auto & fileAnnotation : annotateEdgeWithFiles(cycle[i], cycle[i + 1])) {
45+
cycleWithFiles.push_back(std::move(fileAnnotation));
46+
}
47+
}
48+
// Add final node to close the cycle
49+
if (!cycle.empty()) {
50+
cycleWithFiles.emplace_back(cycle.back().to_string());
51+
}
52+
return cycleWithFiles;
53+
})
54+
| std::ranges::to<std::vector>();
55+
}
56+
57+
} // 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',

0 commit comments

Comments
 (0)