Skip to content

Commit f87213c

Browse files
committed
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.
1 parent a909aac commit f87213c

File tree

4 files changed

+84
-45
lines changed

4 files changed

+84
-45
lines changed

CONTRIBUTING.md

Lines changed: 8 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -142,37 +142,18 @@ Zig cannot detect by itself.
142142

143143
### On Linux
144144

145-
On Linux the recommended tool to check for memory leaks is Valgrind. We supply
146-
a file containing suppressions for false positives and known leaks in 3rd party
147-
libraries. The recommended way to run Valgrind is:
145+
On Linux the recommended tool to check for memory leaks is Valgrind. The
146+
recommended way to run Valgrind is via `zig build`:
148147

148+
```sh
149+
zig build run-valgrind
149150
```
150-
zig build -Dcpu=baseline -Doptimize=Debug
151-
valgrind \
152-
--leak-check=full \
153-
--num-callers=50 \
154-
--suppressions=valgrind.supp \
155-
./zig-out/bin/ghostty
156-
```
157-
158-
> [!NOTE]
159-
>
160-
> `-Dcpu=baseline` may not be needed depending on your CPU, but Valgrind cannot
161-
> deal with some instructions on certain newer CPUs so using `-Dcpu=baseline`
162-
> doesn't hurt.
163151

164-
Any leaks found by Valgrind should be investigated.
165-
166-
If you use Nix, you can use the following commands to run Valgrind so that you
167-
don't need to look up the Valgrind invocation every time:
168-
169-
```
170-
nix develop
171-
nix run .#valgrind -- --config-default-files=true
172-
```
152+
This builds a Ghostty executable with Valgrind support and runs Valgrind
153+
with the proper flags to ensure we're suppressing known false positives.
173154

174-
You can add any Ghostty CLI arguments after the `--` and they will be passed to
175-
the invocation of Ghostty.
155+
You can combine the same build args with `run-valgrind` that you can with
156+
`run`, such as specifying additional configurations after a trailing `--`.
176157

177158
## Input Stack Testing
178159

build.zig

Lines changed: 53 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,15 @@ pub fn build(b: *std.Build) !void {
1919
// All our steps which we'll hook up later. The steps are shown
2020
// up here just so that they are more self-documenting.
2121
const run_step = b.step("run", "Run the app");
22-
const test_step = b.step("test", "Run all tests");
22+
const run_valgrind_step = b.step(
23+
"run-valgrind",
24+
"Run the app under valgrind",
25+
);
26+
const test_step = b.step("test", "Run tests");
27+
const test_valgrind_step = b.step(
28+
"test-valgrind",
29+
"Run tests under valgrind",
30+
);
2331
const translations_step = b.step(
2432
"update-translations",
2533
"Update translation files",
@@ -77,9 +85,11 @@ pub fn build(b: *std.Build) !void {
7785

7886
// Runtime "none" is libghostty, anything else is an executable.
7987
if (config.app_runtime != .none) {
80-
exe.install();
81-
resources.install();
82-
if (i18n) |v| v.install();
88+
if (config.emit_exe) {
89+
exe.install();
90+
resources.install();
91+
if (i18n) |v| v.install();
92+
}
8393
} else {
8494
// Libghostty
8595
//
@@ -181,14 +191,39 @@ pub fn build(b: *std.Build) !void {
181191
}
182192
}
183193

194+
// Valgrind
195+
if (config.app_runtime != .none) {
196+
// We need to rebuild Ghostty with a baseline CPU target.
197+
const valgrind_exe = exe: {
198+
var valgrind_config = config;
199+
valgrind_config.target = valgrind_config.baselineTarget();
200+
break :exe try buildpkg.GhosttyExe.init(
201+
b,
202+
&valgrind_config,
203+
&deps,
204+
);
205+
};
206+
207+
const run_cmd = b.addSystemCommand(&.{
208+
"valgrind",
209+
"--leak-check=full",
210+
"--num-callers=50",
211+
b.fmt("--suppressions={s}", .{b.pathFromRoot("valgrind.supp")}),
212+
"--gen-suppressions=all",
213+
});
214+
run_cmd.addArtifactArg(valgrind_exe.exe);
215+
if (b.args) |args| run_cmd.addArgs(args);
216+
run_valgrind_step.dependOn(&run_cmd.step);
217+
}
218+
184219
// Tests
185220
{
186221
const test_exe = b.addTest(.{
187222
.name = "ghostty-test",
188223
.filters = if (test_filter) |v| &.{v} else &.{},
189224
.root_module = b.createModule(.{
190225
.root_source_file = b.path("src/main.zig"),
191-
.target = config.target,
226+
.target = config.baselineTarget(),
192227
.optimize = .Debug,
193228
.strip = false,
194229
.omit_frame_pointer = false,
@@ -198,8 +233,21 @@ pub fn build(b: *std.Build) !void {
198233

199234
if (config.emit_test_exe) b.installArtifact(test_exe);
200235
_ = try deps.add(test_exe);
236+
237+
// Normal test running
201238
const test_run = b.addRunArtifact(test_exe);
202239
test_step.dependOn(&test_run.step);
240+
241+
// Valgrind test running
242+
const valgrind_run = b.addSystemCommand(&.{
243+
"valgrind",
244+
"--leak-check=full",
245+
"--num-callers=50",
246+
b.fmt("--suppressions={s}", .{b.pathFromRoot("valgrind.supp")}),
247+
"--gen-suppressions=all",
248+
});
249+
valgrind_run.addArtifactArg(test_exe);
250+
test_valgrind_step.dependOn(&valgrind_run.step);
203251
}
204252

205253
// update-translations does what it sounds like and updates the "pot"

flake.nix

Lines changed: 0 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -94,19 +94,6 @@
9494
x11-gnome = runVM ./nix/vm/x11-gnome.nix;
9595
x11-plasma6 = runVM ./nix/vm/x11-plasma6.nix;
9696
x11-xfce = runVM ./nix/vm/x11-xfce.nix;
97-
valgrind = let
98-
script = pkgs.writeShellScript "valgrind" ''
99-
zig build -Dcpu=baseline -Doptimize=Debug
100-
valgrind \
101-
--leak-check=full \
102-
--num-callers=50 \
103-
--suppressions=valgrind.supp \
104-
./zig-out/bin/ghostty "$@"
105-
'';
106-
in {
107-
type = "app";
108-
program = "${script}";
109-
};
11097
};
11198
}
11299
# Our supported systems are the same supported systems as the Zig binaries.

src/build/Config.zig

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,7 @@ patch_rpath: ?[]const u8 = null,
5353
flatpak: bool = false,
5454
emit_bench: bool = false,
5555
emit_docs: bool = false,
56+
emit_exe: bool = false,
5657
emit_helpgen: bool = false,
5758
emit_macos_app: bool = false,
5859
emit_terminfo: bool = false,
@@ -286,6 +287,12 @@ pub fn init(b: *std.Build) !Config {
286287
//---------------------------------------------------------------
287288
// Artifacts to Emit
288289

290+
config.emit_exe = b.option(
291+
bool,
292+
"emit-exe",
293+
"Build and install main executables with 'build'",
294+
) orelse true;
295+
289296
config.emit_test_exe = b.option(
290297
bool,
291298
"emit-test-exe",
@@ -460,6 +467,22 @@ pub fn addOptions(self: *const Config, step: *std.Build.Step.Options) !void {
460467
);
461468
}
462469

470+
/// Returns a baseline CPU target retaining all the other CPU configs.
471+
pub fn baselineTarget(self: *const Config) std.Build.ResolvedTarget {
472+
// Set our cpu model as baseline. There may need to be other modifications
473+
// we need to make such as resetting CPU features but for now this works.
474+
var q = self.target.query;
475+
q.cpu_model = .baseline;
476+
477+
// Same logic as build.resolveTargetQuery but we don't need to
478+
// handle the native case.
479+
return .{
480+
.query = q,
481+
.result = std.zig.system.resolveTargetQuery(q) catch
482+
@panic("unable to resolve baseline query"),
483+
};
484+
}
485+
463486
/// Rehydrate our Config from the comptime options. Note that not all
464487
/// options are available at comptime, so look closely at this implementation
465488
/// to see what is and isn't available.

0 commit comments

Comments
 (0)