From 7577c2fa16289fc887052005ae282ef46e716333 Mon Sep 17 00:00:00 2001 From: Techatrix Date: Thu, 26 Dec 2024 05:37:23 +0100 Subject: [PATCH] fix and test build runner package collection issues (#2119) fixes #2117 fixes #2118 --- .gitattributes | 1 + .github/workflows/build_runner.yml | 3 +- build.zig | 5 + src/build_runner/master.zig | 185 +++++++++++------- tests/add_build_runner_cases.zig | 62 ++++++ tests/build_runner_cases/add_module.json | 15 ++ tests/build_runner_cases/add_module.zig | 7 + tests/build_runner_cases/empty.json | 10 + tests/build_runner_cases/empty.zig | 5 + .../module_self_import.json | 19 ++ .../build_runner_cases/module_self_import.zig | 9 + .../multiple_module_import_names.json | 39 ++++ .../multiple_module_import_names.zig | 18 ++ tests/build_runner_check.zig | 48 +++++ 14 files changed, 348 insertions(+), 78 deletions(-) create mode 100644 tests/add_build_runner_cases.zig create mode 100644 tests/build_runner_cases/add_module.json create mode 100644 tests/build_runner_cases/add_module.zig create mode 100644 tests/build_runner_cases/empty.json create mode 100644 tests/build_runner_cases/empty.zig create mode 100644 tests/build_runner_cases/module_self_import.json create mode 100644 tests/build_runner_cases/module_self_import.zig create mode 100644 tests/build_runner_cases/multiple_module_import_names.json create mode 100644 tests/build_runner_cases/multiple_module_import_names.zig create mode 100644 tests/build_runner_check.zig diff --git a/.gitattributes b/.gitattributes index 2a740732f..d607042ec 100644 --- a/.gitattributes +++ b/.gitattributes @@ -1,3 +1,4 @@ * text=auto *.zig text=auto eol=lf *.zon text=auto eol=lf +*.json text=auto eol=lf diff --git a/.github/workflows/build_runner.yml b/.github/workflows/build_runner.yml index 8c1f384df..3814d4e70 100644 --- a/.github/workflows/build_runner.yml +++ b/.github/workflows/build_runner.yml @@ -21,8 +21,7 @@ jobs: include: - zig-version: mach-latest build-runner-file: master.zig - - zig-version: master - build-runner-file: master.zig + # Zig master is handled by 'zig build test' in the main CI runs-on: ubuntu-latest steps: diff --git a/build.zig b/build.zig index faf8724ae..50df0b281 100644 --- a/build.zig +++ b/build.zig @@ -243,6 +243,11 @@ pub fn build(b: *Build) !void { { // zig build test const test_step = b.step("test", "Run all the tests"); + + const test_build_runner_step = b.step("test-build-runner", "Run all the build runner tests"); + @import("tests/add_build_runner_cases.zig").addCases(b, test_build_runner_step, test_filters); + + test_step.dependOn(test_build_runner_step); test_step.dependOn(&b.addRunArtifact(tests).step); test_step.dependOn(&b.addRunArtifact(src_tests).step); } diff --git a/src/build_runner/master.zig b/src/build_runner/master.zig index 914096c30..21efc98d5 100644 --- a/src/build_runner/master.zig +++ b/src/build_runner/master.zig @@ -926,11 +926,7 @@ const Packages = struct { /// Returns true if the package was already present. pub fn addPackage(self: *Packages, name: []const u8, path: []const u8) !bool { - const name_gop_result = try self.packages.getOrPut(self.allocator, name); - if (!name_gop_result.found_existing) { - name_gop_result.value_ptr.* = .{}; - } - + const name_gop_result = try self.packages.getOrPutValue(self.allocator, name, .{}); const path_gop_result = try name_gop_result.value_ptr.getOrPut(self.allocator, path); return path_gop_result.found_existing; } @@ -939,11 +935,19 @@ const Packages = struct { var result: std.ArrayListUnmanaged(BuildConfig.Package) = .{}; errdefer result.deinit(self.allocator); - var name_iter = self.packages.iterator(); - while (name_iter.next()) |path_hashmap| { - var path_iter = path_hashmap.value_ptr.iterator(); - while (path_iter.next()) |path| { - try result.append(self.allocator, .{ .name = path_hashmap.key_ptr.*, .path = path.key_ptr.* }); + const Context = struct { + keys: [][]const u8, + + pub fn lessThan(ctx: @This(), a_index: usize, b_index: usize) bool { + return std.mem.lessThan(u8, ctx.keys[a_index], ctx.keys[b_index]); + } + }; + + self.packages.sort(Context{ .keys = self.packages.keys() }); + + for (self.packages.keys(), self.packages.values()) |name, path_hashmap| { + for (path_hashmap.keys()) |path| { + try result.append(self.allocator, .{ .name = name, .path = path }); } } @@ -951,9 +955,8 @@ const Packages = struct { } pub fn deinit(self: *Packages) void { - var outer_iter = self.packages.iterator(); - while (outer_iter.next()) |inner| { - inner.value_ptr.deinit(self.allocator); + for (self.packages.values()) |*path_hashmap| { + path_hashmap.deinit(self.allocator); } self.packages.deinit(self.allocator); } @@ -990,18 +993,9 @@ fn extractBuildInformation( const helper = struct { fn addStepDependencies(allocator: Allocator, set: *std.AutoArrayHashMapUnmanaged(*Step, void), lazy_path: std.Build.LazyPath) !void { - const lazy_path_updated_version = comptime std.SemanticVersion.parse("0.13.0-dev.79+6bc0cef60") catch unreachable; - if (comptime builtin.zig_version.order(lazy_path_updated_version) == .lt) { - switch (lazy_path) { - .src_path, .path, .cwd_relative, .dependency => {}, - .generated => |gen| try set.put(allocator, gen.step, {}), - .generated_dirname => |gen| try set.put(allocator, gen.generated.step, {}), - } - } else { - switch (lazy_path) { - .src_path, .cwd_relative, .dependency => {}, - .generated => |gen| try set.put(allocator, gen.file.step, {}), - } + switch (lazy_path) { + .src_path, .cwd_relative, .dependency => {}, + .generated => |gen| try set.put(allocator, gen.file.step, {}), } } fn addModuleDependencies(allocator: Allocator, set: *std.AutoArrayHashMapUnmanaged(*Step, void), module: *std.Build.Module) !void { @@ -1035,6 +1029,60 @@ fn extractBuildInformation( } } } + + fn processItem( + allocator: Allocator, + module: *std.Build.Module, + compile: ?*std.Build.Step.Compile, + name: []const u8, + packages: *Packages, + include_dirs: *std.StringArrayHashMapUnmanaged(void), + ) !void { + if (module.root_source_file) |root_source_file| { + _ = try packages.addPackage(name, root_source_file.getPath(module.owner)); + } + + if (compile) |exe| { + try processPkgConfig(allocator, include_dirs, exe); + } + + for (module.include_dirs.items) |include_dir| { + switch (include_dir) { + .path, + .path_system, + .path_after, + .framework_path, + .framework_path_system, + => |include_path| try include_dirs.put(allocator, include_path.getPath(module.owner), {}), + + .other_step => |other| { + if (other.generated_h) |header| { + try include_dirs.put( + allocator, + std.fs.path.dirname(header.getPath()).?, + {}, + ); + } + if (other.installed_headers_include_tree) |include_tree| { + try include_dirs.put( + allocator, + include_tree.generated_directory.getPath(), + {}, + ); + } + }, + .config_header_step => |config_header| { + const full_file_path = config_header.output_file.getPath(); + const header_dir_path = full_file_path[0 .. full_file_path.len - config_header.include_path.len]; + try include_dirs.put( + allocator, + header_dir_path, + {}, + ); + }, + } + } + } }; var step_dependencies: std.AutoArrayHashMapUnmanaged(*Step, void) = .{}; @@ -1049,21 +1097,27 @@ fn extractBuildInformation( defer dependency_set.deinit(gpa); if (comptime builtin.zig_version.order(accept_root_module_version) != .lt) { + var modules: std.AutoArrayHashMapUnmanaged(*std.Build.Module, void) = .{}; + defer modules.deinit(gpa); + // collect root modules of `Step.Compile` for (steps.keys()) |step| { const compile = step.cast(Step.Compile) orelse continue; const graph = compile.root_module.getGraph(); + try modules.ensureUnusedCapacity(gpa, graph.modules.len); + for (graph.modules) |module| modules.putAssumeCapacity(module, {}); + } - try dependency_set.ensureUnusedCapacity(arena, graph.modules.len); - _ = dependency_set.fetchPutAssumeCapacity(.{ .module = compile.root_module, .compile = compile }, "root"); - for (graph.modules[1..], graph.names[1..]) |module, name| { - _ = dependency_set.fetchPutAssumeCapacity(.{ .module = module, .compile = null }, name); - } + // collect public modules + for (b.modules.values()) |root_module| { + const graph = root_module.getGraph(); + try modules.ensureUnusedCapacity(gpa, graph.modules.len); + for (graph.modules) |module| modules.putAssumeCapacity(module, {}); } - // collect all dependencies - for (dependency_set.keys()) |item| { - try helper.addModuleDependencies(gpa, &step_dependencies, item.module); + // collect all dependencies of all found modules + for (modules.keys()) |module| { + try helper.addModuleDependencies(gpa, &step_dependencies, module); } } else { var dependency_iterator: std.Build.Module.DependencyIterator = .{ @@ -1121,61 +1175,40 @@ fn extractBuildInformation( ); var include_dirs: std.StringArrayHashMapUnmanaged(void) = .{}; + defer include_dirs.deinit(gpa); + var packages: Packages = .{ .allocator = gpa }; defer packages.deinit(); // extract packages and include paths - for (dependency_set.keys(), dependency_set.values()) |item, name| { - if (item.module.root_source_file) |root_source_file| { - _ = try packages.addPackage(name, root_source_file.getPath(item.module.owner)); - } - - if (comptime builtin.zig_version.order(accept_root_module_version) == .lt) { + if (comptime builtin.zig_version.order(accept_root_module_version) == .lt) { + for (dependency_set.keys(), dependency_set.values()) |item, name| { + try helper.processItem(gpa, item.module, item.compile, name, &packages, &include_dirs); for (item.module.import_table.keys(), item.module.import_table.values()) |import_name, import| { if (import.root_source_file) |root_source_file| { _ = try packages.addPackage(import_name, root_source_file.getPath(item.module.owner)); } } } - - if (item.compile) |exe| { - try processPkgConfig(gpa, &include_dirs, exe); + } else { + for (steps.keys()) |step| { + const compile = step.cast(Step.Compile) orelse continue; + const graph = compile.root_module.getGraph(); + try helper.processItem(gpa, compile.root_module, compile, "root", &packages, &include_dirs); + for (graph.modules) |module| { + for (module.import_table.keys(), module.import_table.values()) |name, import| { + try helper.processItem(gpa, import, null, name, &packages, &include_dirs); + } + } } - for (item.module.include_dirs.items) |include_dir| { - switch (include_dir) { - .path, - .path_system, - .path_after, - .framework_path, - .framework_path_system, - => |include_path| try include_dirs.put(arena, include_path.getPath(item.module.owner), {}), - - .other_step => |other| { - if (other.generated_h) |header| { - try include_dirs.put( - arena, - std.fs.path.dirname(header.getPath()).?, - {}, - ); - } - if (other.installed_headers_include_tree) |include_tree| { - try include_dirs.put( - arena, - include_tree.generated_directory.getPath(), - {}, - ); - } - }, - .config_header_step => |config_header| { - const full_file_path = config_header.output_file.getPath(); - const header_dir_path = full_file_path[0 .. full_file_path.len - config_header.include_path.len]; - try include_dirs.put( - arena, - header_dir_path, - {}, - ); - }, + for (b.modules.values()) |root_module| { + const graph = root_module.getGraph(); + try helper.processItem(gpa, root_module, null, "root", &packages, &include_dirs); + for (graph.modules) |module| { + for (module.import_table.keys(), module.import_table.values()) |name, import| { + try helper.processItem(gpa, import, null, name, &packages, &include_dirs); + } } } } diff --git a/tests/add_build_runner_cases.zig b/tests/add_build_runner_cases.zig new file mode 100644 index 000000000..2c952e131 --- /dev/null +++ b/tests/add_build_runner_cases.zig @@ -0,0 +1,62 @@ +const std = @import("std"); + +pub fn addCases( + b: *std.Build, + test_step: *std.Build.Step, + test_filters: []const []const u8, +) void { + const build_runner = b.path("src/build_runner/master.zig"); + const cases_dir = b.path("tests/build_runner_cases"); + const cases_path_from_root = b.pathFromRoot("tests/build_runner_cases"); + + const check_exe = b.addExecutable(.{ + .name = "build_runner_check", + .root_source_file = b.path("tests/build_runner_check.zig"), + .target = b.graph.host, + }); + + // https://github.com/ziglang/zig/issues/20605 + var dir = std.fs.openDirAbsolute(b.pathFromRoot(cases_path_from_root), .{ .iterate = true }) catch |err| + std.debug.panic("failed to open '{s}': {}", .{ cases_path_from_root, err }); + defer dir.close(); + + var it = dir.iterate(); + + while (true) { + const entry = it.next() catch |err| + std.debug.panic("failed to walk directory '{s}': {}", .{ cases_path_from_root, err }) orelse break; + + if (entry.kind != .file) continue; + if (!std.mem.eql(u8, std.fs.path.extension(entry.name), ".zig")) continue; + + for (test_filters) |test_filter| { + if (std.mem.indexOf(u8, entry.name, test_filter) != null) break; + } else if (test_filters.len > 0) continue; + + const build_file = cases_dir.path(b, entry.name); + const build_config_json_path = b.fmt("{s}/{s}.json", .{ cases_path_from_root, std.fs.path.stem(entry.name) }); + const expected_build_config_json = cases_dir.path(b, build_config_json_path); + + const build_cmd = std.Build.Step.Run.create(b, b.fmt("run build runner ({s})", .{entry.name})); + build_cmd.addFileArg(.{ .cwd_relative = b.graph.zig_exe }); + build_cmd.addArg("build"); + build_cmd.addArg("--build-file"); + build_cmd.addFileArg(build_file); + build_cmd.addArg("--build-runner"); + build_cmd.addFileArg(build_runner); + build_cmd.addArg("--cache-dir"); + build_cmd.addDirectoryArg(.{ .cwd_relative = b.fmt("{}", .{b.cache_root}) }); + build_cmd.addArg("--global-cache-dir"); + build_cmd.addDirectoryArg(.{ .cwd_relative = b.fmt("{}", .{b.graph.global_cache_root}) }); + + const actual_build_config_json = build_cmd.captureStdOut(); + + const run_diff = b.addRunArtifact(check_exe); + run_diff.setName(b.fmt("run {s} ({s})", .{ check_exe.name, entry.name })); + run_diff.addFileArg(expected_build_config_json); + run_diff.addFileArg(actual_build_config_json); + run_diff.addDirectoryArg(cases_dir); + + test_step.dependOn(&run_diff.step); + } +} diff --git a/tests/build_runner_cases/add_module.json b/tests/build_runner_cases/add_module.json new file mode 100644 index 000000000..fa62c8df9 --- /dev/null +++ b/tests/build_runner_cases/add_module.json @@ -0,0 +1,15 @@ +{ + "deps_build_roots": [], + "packages": [ + { + "name": "root", + "path": "root.zig" + } + ], + "include_dirs": [], + "top_level_steps": [ + "install", + "uninstall" + ], + "available_options": {} +} \ No newline at end of file diff --git a/tests/build_runner_cases/add_module.zig b/tests/build_runner_cases/add_module.zig new file mode 100644 index 000000000..30bef6678 --- /dev/null +++ b/tests/build_runner_cases/add_module.zig @@ -0,0 +1,7 @@ +const std = @import("std"); + +pub fn build(b: *std.Build) void { + _ = b.addModule("foo", .{ + .root_source_file = b.path("root.zig"), + }); +} diff --git a/tests/build_runner_cases/empty.json b/tests/build_runner_cases/empty.json new file mode 100644 index 000000000..8b55ee76a --- /dev/null +++ b/tests/build_runner_cases/empty.json @@ -0,0 +1,10 @@ +{ + "deps_build_roots": [], + "packages": [], + "include_dirs": [], + "top_level_steps": [ + "install", + "uninstall" + ], + "available_options": {} +} \ No newline at end of file diff --git a/tests/build_runner_cases/empty.zig b/tests/build_runner_cases/empty.zig new file mode 100644 index 000000000..5b3ba7fda --- /dev/null +++ b/tests/build_runner_cases/empty.zig @@ -0,0 +1,5 @@ +const std = @import("std"); + +pub fn build(b: *std.Build) void { + _ = b; +} diff --git a/tests/build_runner_cases/module_self_import.json b/tests/build_runner_cases/module_self_import.json new file mode 100644 index 000000000..b6149690a --- /dev/null +++ b/tests/build_runner_cases/module_self_import.json @@ -0,0 +1,19 @@ +{ + "deps_build_roots": [], + "packages": [ + { + "name": "bar", + "path": "root.zig" + }, + { + "name": "root", + "path": "root.zig" + } + ], + "include_dirs": [], + "top_level_steps": [ + "install", + "uninstall" + ], + "available_options": {} +} \ No newline at end of file diff --git a/tests/build_runner_cases/module_self_import.zig b/tests/build_runner_cases/module_self_import.zig new file mode 100644 index 000000000..dc29a955f --- /dev/null +++ b/tests/build_runner_cases/module_self_import.zig @@ -0,0 +1,9 @@ +const std = @import("std"); + +// https://github.com/zigtools/zls/issues/2117 +pub fn build(b: *std.Build) void { + const module = b.addModule("foo", .{ + .root_source_file = b.path("root.zig"), + }); + module.addImport("bar", module); +} diff --git a/tests/build_runner_cases/multiple_module_import_names.json b/tests/build_runner_cases/multiple_module_import_names.json new file mode 100644 index 000000000..0efa251a3 --- /dev/null +++ b/tests/build_runner_cases/multiple_module_import_names.json @@ -0,0 +1,39 @@ +{ + "deps_build_roots": [], + "packages": [ + { + "name": "bar_in_foo", + "path": "bar.zig" + }, + { + "name": "bar_in_main", + "path": "bar.zig" + }, + { + "name": "foo_in_bar", + "path": "foo.zig" + }, + { + "name": "foo_in_main", + "path": "foo.zig" + }, + { + "name": "root", + "path": "foo.zig" + }, + { + "name": "root", + "path": "bar.zig" + }, + { + "name": "root", + "path": "main.zig" + } + ], + "include_dirs": [], + "top_level_steps": [ + "install", + "uninstall" + ], + "available_options": {} +} \ No newline at end of file diff --git a/tests/build_runner_cases/multiple_module_import_names.zig b/tests/build_runner_cases/multiple_module_import_names.zig new file mode 100644 index 000000000..352813d9b --- /dev/null +++ b/tests/build_runner_cases/multiple_module_import_names.zig @@ -0,0 +1,18 @@ +const std = @import("std"); + +// https://github.com/zigtools/zls/issues/2118 +pub fn build(b: *std.Build) void { + const foo = b.addModule("foo", .{ .root_source_file = b.path("foo.zig") }); + const bar = b.addModule("bar", .{ .root_source_file = b.path("bar.zig") }); + + foo.addImport("bar_in_foo", bar); + bar.addImport("foo_in_bar", foo); + + _ = b.addModule("main", .{ + .root_source_file = b.path("main.zig"), + .imports = &.{ + .{ .name = "foo_in_main", .module = foo }, + .{ .name = "bar_in_main", .module = bar }, + }, + }); +} diff --git a/tests/build_runner_check.zig b/tests/build_runner_check.zig new file mode 100644 index 000000000..37c518b7c --- /dev/null +++ b/tests/build_runner_check.zig @@ -0,0 +1,48 @@ +const std = @import("std"); + +pub fn main() !u8 { + var general_purpose_allocator: std.heap.GeneralPurposeAllocator(.{}) = .init; + defer _ = general_purpose_allocator.deinit(); + const gpa = general_purpose_allocator.allocator(); + + const args = try std.process.argsAlloc(gpa); + defer std.process.argsFree(gpa, args); + + if (args.len != 4) @panic("invalid arguments"); + + const expected = std.fs.cwd().readFileAlloc(gpa, args[1], std.math.maxInt(u32)) catch |err| + std.debug.panic("could no open/read file '{s}': {}", .{ args[1], err }); + defer gpa.free(expected); + + const actual_unsanitized = std.fs.cwd().readFileAlloc(gpa, args[2], std.math.maxInt(u32)) catch |err| + std.debug.panic("could no open/read file '{s}': {}", .{ args[2], err }); + defer gpa.free(actual_unsanitized); + + const actual = blk: { + var base_dir_buffer: std.ArrayListUnmanaged(u8) = .{}; + defer base_dir_buffer.deinit(gpa); + + try std.json.encodeJsonStringChars(args[3], .{}, base_dir_buffer.writer(gpa)); + try std.json.encodeJsonStringChars(&.{std.fs.path.sep}, .{}, base_dir_buffer.writer(gpa)); + + // The build runner will produce absolute paths in the output so we remove them here. + const actual = try std.mem.replaceOwned(u8, gpa, actual_unsanitized, base_dir_buffer.items, ""); + + // We also convert windows style '\\' path separators to posix style '/'. + switch (std.fs.path.sep) { + '/' => break :blk actual, + '\\' => { + defer gpa.free(actual); + break :blk try std.mem.replaceOwned(u8, gpa, actual, "\\\\", "/"); + }, + else => unreachable, + } + }; + defer gpa.free(actual); + + if (std.mem.eql(u8, expected, actual)) return 0; + + std.testing.expectEqualStrings(expected, actual) catch {}; + + return 1; +}