From ae119a9a8d5b8dec21bd314e91afcec122eb8631 Mon Sep 17 00:00:00 2001 From: Andrew Kelley Date: Tue, 16 May 2023 16:29:20 -0700 Subject: [PATCH] CLI: fix stdin dumping behavior * no need to move `tmpFilePath` around * no need for calculating max length of `FileExt` tag name * provide a canonical file extension name for `FileExt` so that, e.g. the file will be named `stdin.S` instead of `stdin.assembly_with_cpp`. * move temp file cleanup to a function to reduce defer bloat in a large function. * fix bug caused by mixing relative and absolute paths in the cleanup logic. * remove commented out test and dead code --- lib/std/Build/Cache.zig | 10 ----- src/Compilation.zig | 39 +++++++++++++---- src/main.zig | 54 ++++++++++++++---------- test/tests.zig | 93 ----------------------------------------- 4 files changed, 62 insertions(+), 134 deletions(-) diff --git a/lib/std/Build/Cache.zig b/lib/std/Build/Cache.zig index 4fc6784684..17429c0370 100644 --- a/lib/std/Build/Cache.zig +++ b/lib/std/Build/Cache.zig @@ -31,16 +31,6 @@ pub const Directory = struct { } } - pub fn tmpFilePath(self: Directory, ally: Allocator, suffix: []const u8) error{OutOfMemory}![]const u8 { - const s = std.fs.path.sep_str; - const rand_int = std.crypto.random.int(u64); - if (self.path) |p| { - return std.fmt.allocPrint(ally, "{s}" ++ s ++ "tmp" ++ s ++ "{x}-{s}", .{ p, rand_int, suffix }); - } else { - return std.fmt.allocPrint(ally, "tmp" ++ s ++ "{x}-{s}", .{ rand_int, suffix }); - } - } - /// Whether or not the handle should be closed, or the path should be freed /// is determined by usage, however this function is provided for convenience /// if it happens to be what the caller needs. diff --git a/src/Compilation.zig b/src/Compilation.zig index 3ae43773e3..0818eaafdd 100644 --- a/src/Compilation.zig +++ b/src/Compilation.zig @@ -3981,7 +3981,7 @@ fn updateCObject(comp: *Compilation, c_object: *CObject, c_obj_prog_node: *std.P // We can't know the digest until we do the C compiler invocation, // so we need a temporary filename. - const out_obj_path = try comp.local_cache_directory.tmpFilePath(arena, o_basename); + const out_obj_path = try comp.tmpFilePath(arena, o_basename); var zig_cache_tmp_dir = try comp.local_cache_directory.handle.makeOpenPath("tmp", .{}); defer zig_cache_tmp_dir.close(); @@ -4129,6 +4129,16 @@ fn updateCObject(comp: *Compilation, c_object: *CObject, c_obj_prog_node: *std.P }; } +pub fn tmpFilePath(comp: *Compilation, ally: Allocator, suffix: []const u8) error{OutOfMemory}![]const u8 { + const s = std.fs.path.sep_str; + const rand_int = std.crypto.random.int(u64); + if (comp.local_cache_directory.path) |p| { + return std.fmt.allocPrint(ally, "{s}" ++ s ++ "tmp" ++ s ++ "{x}-{s}", .{ p, rand_int, suffix }); + } else { + return std.fmt.allocPrint(ally, "tmp" ++ s ++ "{x}-{s}", .{ rand_int, suffix }); + } +} + pub fn addTranslateCCArgs( comp: *Compilation, arena: Allocator, @@ -4588,13 +4598,26 @@ pub const FileExt = enum { }; } - // maximum length of @tagName(ext: FileExt) - pub const max_len = blk: { - var max: u16 = 0; - inline for (std.meta.tags(FileExt)) |ext| - max = std.math.max(@tagName(ext).len, max); - break :blk max; - }; + pub fn canonicalName(ext: FileExt, target: Target) [:0]const u8 { + return switch (ext) { + .c => ".c", + .cpp => ".cpp", + .cu => ".cu", + .h => ".h", + .m => ".m", + .mm => ".mm", + .ll => ".ll", + .bc => ".bc", + .assembly => ".s", + .assembly_with_cpp => ".S", + .shared_library => target.dynamicLibSuffix(), + .object => target.ofmt.fileExt(target.cpu.arch), + .static_library => target.staticLibSuffix(), + .zig => ".zig", + .def => ".def", + .unknown => "", + }; + } }; pub fn hasObjectExt(filename: []const u8) bool { diff --git a/src/main.zig b/src/main.zig index a1f95aaf21..46dbf1de1e 100644 --- a/src/main.zig +++ b/src/main.zig @@ -705,6 +705,17 @@ const ArgsIterator = struct { } }; +fn cleanupTempStdinFile( + temp_stdin_file: ?[]const u8, + local_cache_directory: Compilation.Directory, +) void { + if (temp_stdin_file) |file| { + // Some garbage may stay in the file system if removal fails; this + // is harmless so no warning is needed. + local_cache_directory.handle.deleteFile(file) catch {}; + } +} + fn buildOutputType( gpa: Allocator, arena: Allocator, @@ -3021,15 +3032,11 @@ fn buildOutputType( }; var temp_stdin_file: ?[]const u8 = null; - defer { - if (temp_stdin_file) |file| { - // some garbage may stay in the file system if removal fails. - // Alternatively, we could tell the user that the removal failed, - // but it's not as much of a deal: it's a temporary cache directory - // at all. - local_cache_directory.handle.deleteFile(file) catch {}; - } - } + // Note that in one of the happy paths, execve() is used to switch to clang + // in which case this cleanup logic does not run and this temp file is + // leaked. Oh well. It's a minor punishment for using `-x c` which nobody + // should be doing. + defer cleanupTempStdinFile(temp_stdin_file, local_cache_directory); for (c_source_files.items) |*src| { if (!mem.eql(u8, src.src_path, "-")) continue; @@ -3038,21 +3045,22 @@ fn buildOutputType( fatal("-E or -x is required when reading from a non-regular file", .{}); // "-" is stdin. Dump it to a real file. - const new_file = blk: { - var buf: ["stdin.".len + Compilation.FileExt.max_len]u8 = undefined; - const fname = try std.fmt.bufPrint(&buf, "stdin.{s}", .{@tagName(ext)}); - const new_name = try local_cache_directory.tmpFilePath(arena, fname); - + const sub_path = blk: { + const sep = fs.path.sep_str; + const sub_path = try std.fmt.allocPrint(arena, "tmp" ++ sep ++ "{x}-stdin{s}", .{ + std.crypto.random.int(u64), ext.canonicalName(target_info.target), + }); try local_cache_directory.handle.makePath("tmp"); - var outfile = try local_cache_directory.handle.createFile(new_name, .{}); - defer outfile.close(); - errdefer local_cache_directory.handle.deleteFile(new_name) catch {}; - - try outfile.writeFileAll(io.getStdIn(), .{}); - break :blk new_name; + var f = try local_cache_directory.handle.createFile(sub_path, .{}); + defer f.close(); + errdefer local_cache_directory.handle.deleteFile(sub_path) catch {}; + try f.writeFileAll(io.getStdIn(), .{}); + break :blk sub_path; }; - temp_stdin_file = new_file; - src.src_path = new_file; + // Relative to `local_cache_directory`. + temp_stdin_file = sub_path; + // Relative to current working directory. + src.src_path = try local_cache_directory.join(arena, &.{sub_path}); } if (build_options.have_llvm and emit_asm != .no) { @@ -3908,7 +3916,7 @@ fn cmdTranslateC(comp: *Compilation, arena: Allocator, fancy_output: ?*Translate const c_src_basename = fs.path.basename(c_source_file.src_path); const dep_basename = try std.fmt.allocPrint(arena, "{s}.d", .{c_src_basename}); - const out_dep_path = try comp.local_cache_directory.tmpFilePath(arena, dep_basename); + const out_dep_path = try comp.tmpFilePath(arena, dep_basename); break :blk out_dep_path; }; diff --git a/test/tests.zig b/test/tests.zig index 26d81ead12..641914aabe 100644 --- a/test/tests.zig +++ b/test/tests.zig @@ -777,52 +777,6 @@ pub fn addCliTests(b: *std.Build) *Step { step.dependOn(&cleanup.step); } - // Test `zig cc -x c -` - // Test author was not able to figure out how to start a child process and - // give an fd to it's stdin. fork/exec works, but we are limiting ourselves - // to POSIX. - // - // TODO: the "zig cc <..." step should be a RunStep.create(...) - // However, how do I create a command (RunStep) that invokes a command - // with a specific file descriptor in the stdin? - //if (builtin.os.tag != .windows) { - // const tmp_path = b.makeTempPath(); - // var dir = std.fs.cwd().openDir(tmp_path, .{}) catch @panic("unhandled"); - // dir.writeFile("truth.c", "int main() { return 42; }") catch @panic("unhandled"); - // var infile = dir.openFile("truth.c", .{}) catch @panic("unhandled"); - - // const outfile = std.fs.path.joinZ( - // b.allocator, - // &[_][]const u8{ tmp_path, "truth" }, - // ) catch @panic("unhandled"); - - // const pid_result = std.os.fork() catch @panic("unhandled"); - // if (pid_result == 0) { // child - // std.os.dup2(infile.handle, std.os.STDIN_FILENO) catch @panic("unhandled"); - // const argv = &[_:null]?[*:0]const u8{ - // b.zig_exe, "cc", - // "-o", outfile, - // "-x", "c", - // "-", - // }; - // const envp = &[_:null]?[*:0]const u8{ - // std.fmt.allocPrintZ(b.allocator, "ZIG_GLOBAL_CACHE_DIR={s}", .{tmp_path}) catch @panic("unhandled"), - // }; - // const err = std.os.execveZ(b.zig_exe, argv, envp); - // std.debug.print("execve error: {any}\n", .{err}); - // std.os.exit(1); - // } - - // const res = std.os.waitpid(pid_result, 0); - // assert(0 == res.status); - - // // run the compiled executable and check if it's telling the truth. - // _ = exec(b.allocator, tmp_path, 42, &[_][]const u8{outfile}) catch @panic("unhandled"); - - // const cleanup = b.addRemoveDirTree(tmp_path); - // step.dependOn(&cleanup.step); - //} - { // Test `zig fmt`. // This test must use a temporary directory rather than a cache @@ -1200,50 +1154,3 @@ pub fn addCases( check_case_exe, ); } - -fn exec( - allocator: std.mem.Allocator, - cwd: []const u8, - expect_code: u8, - argv: []const []const u8, -) !std.ChildProcess.ExecResult { - const max_output_size = 100 * 1024; - const result = std.ChildProcess.exec(.{ - .allocator = allocator, - .argv = argv, - .cwd = cwd, - .max_output_bytes = max_output_size, - }) catch |err| { - std.debug.print("The following command failed:\n", .{}); - printCmd(cwd, argv); - return err; - }; - switch (result.term) { - .Exited => |code| { - if (code != expect_code) { - std.debug.print( - "The following command exited with error code {}, expected {}:\n", - .{ code, expect_code }, - ); - printCmd(cwd, argv); - std.debug.print("stderr:\n{s}\n", .{result.stderr}); - return error.CommandFailed; - } - }, - else => { - std.debug.print("The following command terminated unexpectedly:\n", .{}); - printCmd(cwd, argv); - std.debug.print("stderr:\n{s}\n", .{result.stderr}); - return error.CommandFailed; - }, - } - return result; -} - -fn printCmd(cwd: []const u8, argv: []const []const u8) void { - std.debug.print("cd {s} && ", .{cwd}); - for (argv) |arg| { - std.debug.print("{s} ", .{arg}); - } - std.debug.print("\n", .{}); -}