From 64abbd0ea6e15a7af2001bed3e0c7e097df2bb68 Mon Sep 17 00:00:00 2001 From: Gregory Anders Date: Tue, 17 Sep 2024 19:42:42 -0500 Subject: [PATCH] config: move optional path parsing into RepeatablePath This commit refactors RepeatablePath to contain a list of tagged unions containing "optional" and "required" variants. Both variants have a null terminated file path as their payload, but the tag dictates whether the path must exist or not. This implemenation is used to force consumers to handle the optional vs. required distinction. This also moves the parsing of optional file paths into RepeatablePath's parseCLI function. This allows the code to be better unit tested. Since RepeatablePath no longer contains a simple list of RepeatableStrings, many other of its methods needed to be reimplemented as well. Because all of this functionality is built into the RepeatablePath type, other config options which also use RepeatablePath gain the ability to specify optional paths as well. Right now this is only the "custom-shaders" option. The code paths in the renderer to load shader files has been updated accordingly. In the original optional config file parsing, the leading ? character was removed when paths were expanded. Thus, when config files were actually loaded recursively, they appeared to be regular (required) config files and an error occurred if the file did not exist. **This issue was not found during testing because the presence of the "theme" option masks the error**. I am not sure why the presence of "theme" does this, I did not dig into that. Now because the "optional" or "required" state of each path is tracked in the enum tag the "optional" status of the path is preserved after being expanded to an absolute path. Finally, this commit fixes a bug where missing "config-file" files were not included in the +show-config command (i.e. if a user had `config-file = foo.conf` and `foo.conf` did not exist, then `ghostty +show-config` would only display `config-file =`). This bug applied to `custom-shaders` too, where it has also been fixed. --- src/config.zig | 1 + src/config/Config.zig | 199 +++++++++++++++++++++++++++++-------- src/renderer/Metal.zig | 8 +- src/renderer/OpenGL.zig | 6 +- src/renderer/shadertoy.zig | 18 +++- 5 files changed, 183 insertions(+), 49 deletions(-) diff --git a/src/config.zig b/src/config.zig index 082c842c9..b9f214fc9 100644 --- a/src/config.zig +++ b/src/config.zig @@ -23,6 +23,7 @@ pub const OptionAsAlt = Config.OptionAsAlt; pub const RepeatableCodepointMap = Config.RepeatableCodepointMap; pub const RepeatableFontVariation = Config.RepeatableFontVariation; pub const RepeatableString = Config.RepeatableString; +pub const RepeatablePath = Config.RepeatablePath; pub const ShellIntegrationFeatures = Config.ShellIntegrationFeatures; pub const WindowPaddingColor = Config.WindowPaddingColor; diff --git a/src/config/Config.zig b/src/config/Config.zig index 0aa29addd..7cc84c52a 100644 --- a/src/config/Config.zig +++ b/src/config/Config.zig @@ -2246,7 +2246,7 @@ pub fn loadCliArgs(self: *Config, alloc_gpa: Allocator) !void { } // Config files loaded from the CLI args are relative to pwd - if (self.@"config-file".value.list.items.len > 0) { + if (self.@"config-file".value.items.len > 0) { var buf: [std.fs.max_path_bytes]u8 = undefined; try self.expandPaths(try std.fs.cwd().realpath(".", &buf)); } @@ -2254,7 +2254,7 @@ pub fn loadCliArgs(self: *Config, alloc_gpa: Allocator) !void { /// Load and parse the config files that were added in the "config-file" key. pub fn loadRecursiveFiles(self: *Config, alloc_gpa: Allocator) !void { - if (self.@"config-file".value.list.items.len == 0) return; + if (self.@"config-file".value.items.len == 0) return; const arena_alloc = self._arena.?.allocator(); // Keeps track of loaded files to prevent cycles. @@ -2267,21 +2267,15 @@ pub fn loadRecursiveFiles(self: *Config, alloc_gpa: Allocator) !void { // may add items to the list while iterating for recursive // config-file entries. var i: usize = 0; - while (i < self.@"config-file".value.list.items.len) : (i += 1) { - const optional, const path = blk: { - const path = self.@"config-file".value.list.items[i]; - if (path.len == 0) { - continue; - } - - break :blk if (path[0] == '?') - .{ true, path[1..] } - else if (path[0] == '"' and path[path.len - 1] == '"') - .{ false, path[1 .. path.len - 1] } - else - .{ false, path }; + while (i < self.@"config-file".value.items.len) : (i += 1) { + const path, const optional = switch (self.@"config-file".value.items[i]) { + .optional => |path| .{ path, true }, + .required => |path| .{ path, false }, }; + // Error paths + if (path.len == 0) continue; + // All paths should already be absolute at this point because // they're fixed up after each load. assert(std.fs.path.isAbsolute(path)); @@ -3246,27 +3240,86 @@ pub const RepeatableString = struct { pub const RepeatablePath = struct { const Self = @This(); - value: RepeatableString = .{}, + const Path = union(enum) { + /// No error if the file does not exist. + optional: [:0]const u8, + + /// The file is required to exist. + required: [:0]const u8, + }; + + value: std.ArrayListUnmanaged(Path) = .{}, pub fn parseCLI(self: *Self, alloc: Allocator, input: ?[]const u8) !void { - return self.value.parseCLI(alloc, input); + const value, const optional = if (input) |value| blk: { + if (value.len == 0) { + self.value.clearRetainingCapacity(); + return; + } + + break :blk if (value[0] == '?') + .{ value[1..], true } + else if (value.len >= 2 and value[0] == '"' and value[value.len - 1] == '"') + .{ value[1 .. value.len - 1], false } + else + .{ value, false }; + } else return error.ValueRequired; + + if (value.len == 0) { + // This handles the case of zero length paths after removing any ? + // prefixes or surrounding quotes. In this case, we don't reset the + // list. + return; + } + + const item: Path = if (optional) + .{ .optional = try alloc.dupeZ(u8, value) } + else + .{ .required = try alloc.dupeZ(u8, value) }; + + try self.value.append(alloc, item); } /// Deep copy of the struct. Required by Config. pub fn clone(self: *const Self, alloc: Allocator) !Self { + const value = try self.value.clone(alloc); + for (value.items) |*item| { + switch (item.*) { + .optional, .required => |*path| path.* = try alloc.dupeZ(u8, path.*), + } + } + return .{ - .value = try self.value.clone(alloc), + .value = value, }; } /// Compare if two of our value are requal. Required by Config. pub fn equal(self: Self, other: Self) bool { - return self.value.equal(other.value); + if (self.value.items.len != other.value.items.len) return false; + for (self.value.items, other.value.items) |a, b| { + if (!std.meta.eql(a, b)) return false; + } + + return true; } /// Used by Formatter pub fn formatEntry(self: Self, formatter: anytype) !void { - try self.value.formatEntry(formatter); + if (self.value.items.len == 0) { + try formatter.formatEntry(void, {}); + return; + } + + var buf: [std.fs.max_path_bytes + 1]u8 = undefined; + for (self.value.items) |item| { + const value = switch (item) { + .optional => |path| try std.fmt.bufPrint(&buf, "?{s}", .{path}), + .required => |path| path, + }; + + try formatter.formatEntry([]const u8, value); + } } /// Expand all the paths relative to the base directory. @@ -3280,29 +3333,19 @@ pub const RepeatablePath = struct { var dir = try std.fs.cwd().openDir(base, .{}); defer dir.close(); - for (0..self.value.list.items.len) |i| { - const optional, const path = blk: { - const path = self.value.list.items[i]; - if (path.len == 0) { - continue; - } - - break :blk if (path[0] == '?') - .{ true, path[1..] } - else if (path[0] == '"' and path[path.len - 1] == '"') - .{ false, path[1 .. path.len - 1] } - else - .{ false, path }; + for (0..self.value.items.len) |i| { + const path = switch (self.value.items[i]) { + .optional, .required => |path| path, }; // If it is already absolute we can ignore it. - if (std.fs.path.isAbsolute(path)) continue; + if (path.len == 0 or std.fs.path.isAbsolute(path)) continue; // If it isn't absolute, we need to make it absolute relative // to the base. var buf: [std.fs.max_path_bytes]u8 = undefined; const abs = dir.realpath(path, &buf) catch |err| abs: { - if (err == error.FileNotFound and optional) { + if (err == error.FileNotFound) { // The file doesn't exist. Try to resolve the relative path // another way. const resolved = try std.fs.path.resolve(alloc, &.{ base, path }); @@ -3314,21 +3357,99 @@ pub const RepeatablePath = struct { try errors.add(alloc, .{ .message = try std.fmt.allocPrintZ( alloc, - "error resolving config-file {s}: {}", + "error resolving file path {s}: {}", .{ path, err }, ), }); - self.value.list.items[i] = ""; + + // Blank this path so that we don't attempt to resolve it again + self.value.items[i] = .{ .required = "" }; + continue; }; log.debug( - "expanding config-file path relative={s} abs={s}", + "expanding file path relative={s} abs={s}", .{ path, abs }, ); - self.value.list.items[i] = try alloc.dupeZ(u8, abs); + + switch (self.value.items[i]) { + .optional, .required => |*p| p.* = try alloc.dupeZ(u8, abs), + } } } + + test "parseCLI" { + const testing = std.testing; + var arena = ArenaAllocator.init(testing.allocator); + defer arena.deinit(); + const alloc = arena.allocator(); + + var list: Self = .{}; + try list.parseCLI(alloc, "config.1"); + try list.parseCLI(alloc, "?config.2"); + try list.parseCLI(alloc, "\"?config.3\""); + + // Zero-length values, ignored + try list.parseCLI(alloc, "?"); + try list.parseCLI(alloc, "\"\""); + + try testing.expectEqual(@as(usize, 3), list.value.items.len); + + const Tag = std.meta.Tag(Path); + try testing.expectEqual(Tag.required, @as(Tag, list.value.items[0])); + try testing.expectEqualStrings("config.1", list.value.items[0].required); + + try testing.expectEqual(Tag.optional, @as(Tag, list.value.items[1])); + try testing.expectEqualStrings("config.2", list.value.items[1].optional); + + try testing.expectEqual(Tag.required, @as(Tag, list.value.items[2])); + try testing.expectEqualStrings("?config.3", list.value.items[2].required); + + try list.parseCLI(alloc, ""); + try testing.expectEqual(@as(usize, 0), list.value.items.len); + } + + test "formatConfig empty" { + const testing = std.testing; + var buf = std.ArrayList(u8).init(testing.allocator); + defer buf.deinit(); + + var list: Self = .{}; + try list.formatEntry(formatterpkg.entryFormatter("a", buf.writer())); + try std.testing.expectEqualSlices(u8, "a = \n", buf.items); + } + + test "formatConfig single item" { + const testing = std.testing; + var buf = std.ArrayList(u8).init(testing.allocator); + defer buf.deinit(); + + var arena = ArenaAllocator.init(testing.allocator); + defer arena.deinit(); + const alloc = arena.allocator(); + + var list: Self = .{}; + try list.parseCLI(alloc, "A"); + try list.formatEntry(formatterpkg.entryFormatter("a", buf.writer())); + try std.testing.expectEqualSlices(u8, "a = A\n", buf.items); + } + + test "formatConfig multiple items" { + const testing = std.testing; + var buf = std.ArrayList(u8).init(testing.allocator); + defer buf.deinit(); + + var arena = ArenaAllocator.init(testing.allocator); + defer arena.deinit(); + const alloc = arena.allocator(); + + var list: Self = .{}; + try list.parseCLI(alloc, "A"); + try list.parseCLI(alloc, "?B"); + try list.formatEntry(formatterpkg.entryFormatter("a", buf.writer())); + try std.testing.expectEqualSlices(u8, "a = A\na = ?B\n", buf.items); + } }; /// FontVariation is a repeatable configuration value that sets a single diff --git a/src/renderer/Metal.zig b/src/renderer/Metal.zig index 1629678f6..94ca977a8 100644 --- a/src/renderer/Metal.zig +++ b/src/renderer/Metal.zig @@ -347,7 +347,7 @@ pub const DerivedConfig = struct { bold_is_bright: bool, min_contrast: f32, padding_color: configpkg.WindowPaddingColor, - custom_shaders: std.ArrayListUnmanaged([:0]const u8), + custom_shaders: configpkg.RepeatablePath, links: link.Set, vsync: bool, @@ -360,7 +360,7 @@ pub const DerivedConfig = struct { const alloc = arena.allocator(); // Copy our shaders - const custom_shaders = try config.@"custom-shader".value.list.clone(alloc); + const custom_shaders = try config.@"custom-shader".value.clone(alloc); // Copy our font features const font_features = try config.@"font-feature".list.clone(alloc); @@ -540,7 +540,7 @@ pub fn init(alloc: Allocator, options: renderer.Options) !Metal { // Load our custom shaders const custom_shaders: []const [:0]const u8 = shadertoy.loadFromFiles( arena_alloc, - options.config.custom_shaders.items, + options.config.custom_shaders, .msl, ) catch |err| err: { log.warn("error loading custom shaders err={}", .{err}); @@ -549,7 +549,7 @@ pub fn init(alloc: Allocator, options: renderer.Options) !Metal { // If we have custom shaders then setup our state var custom_shader_state: ?CustomShaderState = state: { - if (custom_shaders.len == 0) break :state null; + if (custom_shaders.value.items.len == 0) break :state null; // Build our sampler for our texture var sampler = try mtl_sampler.Sampler.init(gpu_state.device); diff --git a/src/renderer/OpenGL.zig b/src/renderer/OpenGL.zig index d220cdadc..6d39eb445 100644 --- a/src/renderer/OpenGL.zig +++ b/src/renderer/OpenGL.zig @@ -301,7 +301,7 @@ pub const DerivedConfig = struct { bold_is_bright: bool, min_contrast: f32, padding_color: configpkg.WindowPaddingColor, - custom_shaders: std.ArrayListUnmanaged([:0]const u8), + custom_shaders: configpkg.RepeatablePath, links: link.Set, pub fn init( @@ -313,7 +313,7 @@ pub const DerivedConfig = struct { const alloc = arena.allocator(); // Copy our shaders - const custom_shaders = try config.@"custom-shader".value.list.clone(alloc); + const custom_shaders = try config.@"custom-shader".clone(alloc); // Copy our font features const font_features = try config.@"font-feature".list.clone(alloc); @@ -2327,7 +2327,7 @@ const GLState = struct { const custom_state: ?custom.State = custom: { const shaders: []const [:0]const u8 = shadertoy.loadFromFiles( arena_alloc, - config.custom_shaders.items, + config.custom_shaders, .glsl, ) catch |err| err: { log.warn("error loading custom shaders err={}", .{err}); diff --git a/src/renderer/shadertoy.zig b/src/renderer/shadertoy.zig index 75c058e20..8c9b68447 100644 --- a/src/renderer/shadertoy.zig +++ b/src/renderer/shadertoy.zig @@ -5,6 +5,7 @@ const Allocator = std.mem.Allocator; const ArenaAllocator = std.heap.ArenaAllocator; const glslang = @import("glslang"); const spvcross = @import("spirv_cross"); +const configpkg = @import("../config.zig"); const log = std.log.scoped(.shadertoy); @@ -15,15 +16,26 @@ pub const Target = enum { glsl, msl }; /// format. The shader order is preserved. pub fn loadFromFiles( alloc_gpa: Allocator, - paths: []const []const u8, + paths: configpkg.RepeatablePath, target: Target, ) ![]const [:0]const u8 { var list = std.ArrayList([:0]const u8).init(alloc_gpa); defer list.deinit(); errdefer for (list.items) |shader| alloc_gpa.free(shader); - for (paths) |path| { - const shader = try loadFromFile(alloc_gpa, path, target); + for (paths.value.items) |item| { + const path, const optional = switch (item) { + .optional => |path| .{ path, true }, + .required => |path| .{ path, false }, + }; + + const shader = loadFromFile(alloc_gpa, path, target) catch |err| { + if (err == error.FileNotFound and optional) { + continue; + } + + return err; + }; log.info("loaded custom shader path={s}", .{path}); try list.append(shader); }