build: protect from adding the same dependency multiple times

In experimenting with the build system I discovered there were some places
where the same step was being added as a dependency multiple times. The
implementation currently uses an ArrayList and doesn't prevent the same
entry from being added. For the same reason that Zig makes unused variables
an error, I've made a change that makes duplicate dependencies an error
by default. I did this by using an array hash map for dependencies instead
of an array list. `Step.dependOn` will now panic if the same dependency is
added multiple times. In cases where its unclear if a dependency may have
already been added, like `LazyPath.addStepDependencies`, build code can
call `Step.dependOnIfNotAlready` to avoid this panic.

.github/workflows/ci.yaml

@@ -5,6 +5,7 @@ on:
     branches:
       - master
       - llvm19
+  workflow_dispatch:
 concurrency:
   # Cancels pending runs when a PR gets updated.
   group: ${{ github.head_ref || github.run_id }}-${{ github.actor }}

lib/compiler/build_runner.zig

@@ -943,34 +943,34 @@ fn printTreeStep(
     if (first) {
         try printStepStatus(s, stderr, ttyconf, run);
 
-        const last_index = if (summary == .all) s.dependencies.items.len -| 1 else blk: {
-            var i: usize = s.dependencies.items.len;
+        const last_index = if (summary == .all) s.dependencies.count() -| 1 else blk: {
+            var i: usize = s.dependencies.count();
             while (i > 0) {
                 i -= 1;
 
-                const step = s.dependencies.items[i];
+                const step = s.dependencies.keys()[i];
                 const found = switch (summary) {
                     .all, .none => unreachable,
                     .failures => step.state != .success,
                     .new => !step.result_cached,
                 };
                 if (found) break :blk i;
             }
-            break :blk s.dependencies.items.len -| 1;
+            break :blk s.dependencies.count() -| 1;
         };
-        for (s.dependencies.items, 0..) |dep, i| {
+        for (s.dependencies.keys(), 0..) |dep, i| {
             var print_node: PrintNode = .{
                 .parent = parent_node,
                 .last = i == last_index,
             };
             try printTreeStep(b, dep, run, stderr, ttyconf, &print_node, step_stack);
         }
     } else {
-        if (s.dependencies.items.len == 0) {
+        if (s.dependencies.count() == 0) {
             try stderr.writeAll(" (reused)\n");
         } else {
             try stderr.writer().print(" (+{d} more reused dependencies)\n", .{
-                s.dependencies.items.len,
+                s.dependencies.count(),
             });
         }
         try ttyconf.setColor(stderr, .reset);
@@ -1002,11 +1002,11 @@ fn constructGraphAndCheckForDependencyLoop(
         .precheck_unstarted => {
             s.state = .precheck_started;
 
-            try step_stack.ensureUnusedCapacity(b.allocator, s.dependencies.items.len);
+            try step_stack.ensureUnusedCapacity(b.allocator, s.dependencies.count());
 
             // We dupe to avoid shuffling the steps in the summary, it depends
             // on s.dependencies' order.
-            const deps = b.allocator.dupe(*Step, s.dependencies.items) catch @panic("OOM");
+            const deps = b.allocator.dupe(*Step, s.dependencies.keys()) catch @panic("OOM");
             rand.shuffle(*Step, deps);
 
             for (deps) |dep| {
@@ -1046,7 +1046,7 @@ fn workerMakeOneStep(
     // First, check the conditions for running this step. If they are not met,
     // then we return without doing the step, relying on another worker to
     // queue this step up again when dependencies are met.
-    for (s.dependencies.items) |dep| {
+    for (s.dependencies.keys()) |dep| {
         switch (@atomicLoad(Step.State, &dep.state, .seq_cst)) {
             .success, .skipped => continue,
             .failure, .dependency_failure, .skipped_oom => {

lib/std/Build.zig

@@ -2353,7 +2353,7 @@ pub const LazyPath = union(enum) {
     pub fn addStepDependencies(lazy_path: LazyPath, other_step: *Step) void {
         switch (lazy_path) {
             .src_path, .cwd_relative, .dependency => {},
-            .generated => |gen| other_step.dependOn(gen.file.step),
+            .generated => |gen| _ = other_step.dependOnIfNotAlready(gen.file.step),
         }
     }
 

lib/std/Build/Step.zig

@@ -3,7 +3,7 @@ name: []const u8,
 owner: *Build,
 makeFn: MakeFn,
 
-dependencies: std.ArrayList(*Step),
+dependencies: std.AutoArrayHashMapUnmanaged(*Step, void) = .empty,
 /// This field is empty during execution of the user's build script, and
 /// then populated during dependency loop checking in the build runner.
 dependants: std.ArrayListUnmanaged(*Step),
@@ -196,7 +196,6 @@ pub fn init(options: StepOptions) Step {
         .name = arena.dupe(u8, options.name) catch @panic("OOM"),
         .owner = options.owner,
         .makeFn = options.makeFn,
-        .dependencies = std.ArrayList(*Step).init(arena),
         .dependants = .{},
         .inputs = Inputs.init,
         .state = .precheck_unstarted,
@@ -250,8 +249,19 @@ pub fn make(s: *Step, options: MakeOptions) error{ MakeFailed, MakeSkipped }!voi
     }
 }
 
+/// Adds `other` as a dependency of `step`. Panics if it has already been
+/// added. Use `dependOnIfNotAlready` to avoid this panic.
 pub fn dependOn(step: *Step, other: *Step) void {
-    step.dependencies.append(other) catch @panic("OOM");
+    if (!step.dependOnIfNotAlready(other)) std.debug.panic(
+        "step '{s}' already depends on '{s}'",
+        .{ step.name, other.name },
+    );
+}
+
+/// Adds `other` as a dependency of `step`. Returns true if it was newly added.
+pub fn dependOnIfNotAlready(step: *Step, other: *Step) bool {
+    const entry = step.dependencies.getOrPut(step.owner.allocator, other) catch @panic("OOM");
+    return !entry.found_existing;
 }
 
 pub fn getStackTrace(s: *Step) ?std.builtin.StackTrace {
@@ -271,7 +281,7 @@ fn makeNoOp(step: *Step, options: MakeOptions) anyerror!void {
 
     var all_cached = true;
 
-    for (step.dependencies.items) |dep| {
+    for (step.dependencies.keys()) |dep| {
         all_cached = all_cached and dep.result_cached;
     }
 

test/standalone/cmakedefine/build.zig

@@ -85,7 +85,7 @@ fn compare_headers(step: *std.Build.Step, options: std.Build.Step.MakeOptions) !
     const allocator = step.owner.allocator;
     const expected_fmt = "expected_{s}";
 
-    for (step.dependencies.items) |config_header_step| {
+    for (step.dependencies.keys()) |config_header_step| {
         const config_header: *ConfigHeader = @fieldParentPtr("step", config_header_step);
 
         const zig_header_path = config_header.output_file.path orelse @panic("Could not locate header file");

test/standalone/emit_asm_no_bin/build.zig

@@ -13,7 +13,6 @@ pub fn build(b: *std.Build) void {
         .target = b.graph.host,
     });
     _ = obj.getEmittedAsm();
-    b.default_step.dependOn(&obj.step);
 
     test_step.dependOn(&obj.step);
 }

test/standalone/emit_llvm_no_bin/build.zig

@@ -14,7 +14,6 @@ pub fn build(b: *std.Build) void {
     });
     _ = obj.getEmittedLlvmIr();
     _ = obj.getEmittedLlvmBc();
-    b.default_step.dependOn(&obj.step);
 
     test_step.dependOn(&obj.step);
 }

test/standalone/run_output_caching/build.zig

@@ -90,7 +90,7 @@ const CheckOutputCaching = struct {
     fn make(step: *std.Build.Step, _: std.Build.Step.MakeOptions) !void {
         const check: *CheckOutputCaching = @fieldParentPtr("step", step);
 
-        for (step.dependencies.items) |dependency| {
+        for (step.dependencies.keys()) |dependency| {
             if (check.expect_caching) {
                 if (dependency.result_cached) continue;
                 return step.fail("expected '{s}' step to be cached, but it was not", .{dependency.name});