From f87213c2f68867c7cca2befdd0a75d94f289e2b6 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Wed, 20 Aug 2025 11:09:05 -0700 Subject: [PATCH] build: add run-valgrind and test-valgrind steps This adds two explicit `zig build` steps: `run-valgrind` and `test-valgrind` to run the Ghostty exe or tests under Valgrind, respectively. This simplifies the manual Valgrind calls in a few ways: 1. It automatically sets the CPU to baseline, which is a frequent and requirement for Valgrind on newer CPUs, and generally safe. 2. It sets up the rather complicated set of flags to call Valgrind with, importantly setting up our suppressions. 3. It enables pairing it with the typical and comfortable workflow of specifying extra args (with `--`) or flags like `-Dtest-filter` for tests. --- CONTRIBUTING.md | 35 ++++++-------------------- build.zig | 58 ++++++++++++++++++++++++++++++++++++++++---- flake.nix | 13 ---------- src/build/Config.zig | 23 ++++++++++++++++++ 4 files changed, 84 insertions(+), 45 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index a525b5d54..0e988704b 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -142,37 +142,18 @@ Zig cannot detect by itself. ### On Linux -On Linux the recommended tool to check for memory leaks is Valgrind. We supply -a file containing suppressions for false positives and known leaks in 3rd party -libraries. The recommended way to run Valgrind is: +On Linux the recommended tool to check for memory leaks is Valgrind. The +recommended way to run Valgrind is via `zig build`: -``` -zig build -Dcpu=baseline -Doptimize=Debug -valgrind \ - --leak-check=full \ - --num-callers=50 \ - --suppressions=valgrind.supp \ - ./zig-out/bin/ghostty +```sh +zig build run-valgrind ``` -> [!NOTE] -> -> `-Dcpu=baseline` may not be needed depending on your CPU, but Valgrind cannot -> deal with some instructions on certain newer CPUs so using `-Dcpu=baseline` -> doesn't hurt. +This builds a Ghostty executable with Valgrind support and runs Valgrind +with the proper flags to ensure we're suppressing known false positives. -Any leaks found by Valgrind should be investigated. - -If you use Nix, you can use the following commands to run Valgrind so that you -don't need to look up the Valgrind invocation every time: - -``` -nix develop -nix run .#valgrind -- --config-default-files=true -``` - -You can add any Ghostty CLI arguments after the `--` and they will be passed to -the invocation of Ghostty. +You can combine the same build args with `run-valgrind` that you can with +`run`, such as specifying additional configurations after a trailing `--`. ## Input Stack Testing diff --git a/build.zig b/build.zig index 4acca53cc..38cfd0e56 100644 --- a/build.zig +++ b/build.zig @@ -19,7 +19,15 @@ pub fn build(b: *std.Build) !void { // All our steps which we'll hook up later. The steps are shown // up here just so that they are more self-documenting. const run_step = b.step("run", "Run the app"); - const test_step = b.step("test", "Run all tests"); + const run_valgrind_step = b.step( + "run-valgrind", + "Run the app under valgrind", + ); + const test_step = b.step("test", "Run tests"); + const test_valgrind_step = b.step( + "test-valgrind", + "Run tests under valgrind", + ); const translations_step = b.step( "update-translations", "Update translation files", @@ -77,9 +85,11 @@ pub fn build(b: *std.Build) !void { // Runtime "none" is libghostty, anything else is an executable. if (config.app_runtime != .none) { - exe.install(); - resources.install(); - if (i18n) |v| v.install(); + if (config.emit_exe) { + exe.install(); + resources.install(); + if (i18n) |v| v.install(); + } } else { // Libghostty // @@ -181,6 +191,31 @@ pub fn build(b: *std.Build) !void { } } + // Valgrind + if (config.app_runtime != .none) { + // We need to rebuild Ghostty with a baseline CPU target. + const valgrind_exe = exe: { + var valgrind_config = config; + valgrind_config.target = valgrind_config.baselineTarget(); + break :exe try buildpkg.GhosttyExe.init( + b, + &valgrind_config, + &deps, + ); + }; + + const run_cmd = b.addSystemCommand(&.{ + "valgrind", + "--leak-check=full", + "--num-callers=50", + b.fmt("--suppressions={s}", .{b.pathFromRoot("valgrind.supp")}), + "--gen-suppressions=all", + }); + run_cmd.addArtifactArg(valgrind_exe.exe); + if (b.args) |args| run_cmd.addArgs(args); + run_valgrind_step.dependOn(&run_cmd.step); + } + // Tests { const test_exe = b.addTest(.{ @@ -188,7 +223,7 @@ pub fn build(b: *std.Build) !void { .filters = if (test_filter) |v| &.{v} else &.{}, .root_module = b.createModule(.{ .root_source_file = b.path("src/main.zig"), - .target = config.target, + .target = config.baselineTarget(), .optimize = .Debug, .strip = false, .omit_frame_pointer = false, @@ -198,8 +233,21 @@ pub fn build(b: *std.Build) !void { if (config.emit_test_exe) b.installArtifact(test_exe); _ = try deps.add(test_exe); + + // Normal test running const test_run = b.addRunArtifact(test_exe); test_step.dependOn(&test_run.step); + + // Valgrind test running + const valgrind_run = b.addSystemCommand(&.{ + "valgrind", + "--leak-check=full", + "--num-callers=50", + b.fmt("--suppressions={s}", .{b.pathFromRoot("valgrind.supp")}), + "--gen-suppressions=all", + }); + valgrind_run.addArtifactArg(test_exe); + test_valgrind_step.dependOn(&valgrind_run.step); } // update-translations does what it sounds like and updates the "pot" diff --git a/flake.nix b/flake.nix index 1c36b64ac..7cf58b27c 100644 --- a/flake.nix +++ b/flake.nix @@ -94,19 +94,6 @@ x11-gnome = runVM ./nix/vm/x11-gnome.nix; x11-plasma6 = runVM ./nix/vm/x11-plasma6.nix; x11-xfce = runVM ./nix/vm/x11-xfce.nix; - valgrind = let - script = pkgs.writeShellScript "valgrind" '' - zig build -Dcpu=baseline -Doptimize=Debug - valgrind \ - --leak-check=full \ - --num-callers=50 \ - --suppressions=valgrind.supp \ - ./zig-out/bin/ghostty "$@" - ''; - in { - type = "app"; - program = "${script}"; - }; }; } # Our supported systems are the same supported systems as the Zig binaries. diff --git a/src/build/Config.zig b/src/build/Config.zig index 175745dc6..fd892f16c 100644 --- a/src/build/Config.zig +++ b/src/build/Config.zig @@ -53,6 +53,7 @@ patch_rpath: ?[]const u8 = null, flatpak: bool = false, emit_bench: bool = false, emit_docs: bool = false, +emit_exe: bool = false, emit_helpgen: bool = false, emit_macos_app: bool = false, emit_terminfo: bool = false, @@ -286,6 +287,12 @@ pub fn init(b: *std.Build) !Config { //--------------------------------------------------------------- // Artifacts to Emit + config.emit_exe = b.option( + bool, + "emit-exe", + "Build and install main executables with 'build'", + ) orelse true; + config.emit_test_exe = b.option( bool, "emit-test-exe", @@ -460,6 +467,22 @@ pub fn addOptions(self: *const Config, step: *std.Build.Step.Options) !void { ); } +/// Returns a baseline CPU target retaining all the other CPU configs. +pub fn baselineTarget(self: *const Config) std.Build.ResolvedTarget { + // Set our cpu model as baseline. There may need to be other modifications + // we need to make such as resetting CPU features but for now this works. + var q = self.target.query; + q.cpu_model = .baseline; + + // Same logic as build.resolveTargetQuery but we don't need to + // handle the native case. + return .{ + .query = q, + .result = std.zig.system.resolveTargetQuery(q) catch + @panic("unable to resolve baseline query"), + }; +} + /// Rehydrate our Config from the comptime options. Note that not all /// options are available at comptime, so look closely at this implementation /// to see what is and isn't available.