wip: address @andrewrk review comments

Author: motiejus

lib/std/Build/Cache.zig

@@ -31,7 +31,7 @@ pub const Directory = struct {
         }
     }
 
-    pub fn tmpFilePath(self: *const Directory, ally: Allocator, suffix: []const u8) error{OutOfMemory}![]const u8 {
+    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| {

src/Compilation.zig

@@ -4587,6 +4587,14 @@ pub const FileExt = enum {
             => false,
         };
     }
+
+    // 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 hasObjectExt(filename: []const u8) bool {

src/main.zig

@@ -661,34 +661,6 @@ fn optionalStringEnvVar(arena: Allocator, name: []const u8) !?[]const u8 {
     }
 }
 
-// Read an open file descriptor to a new file in `dst_dir`. Return a file name
-// of the new file.
-fn copyToFile(
-    arena: Allocator,
-    in_fd: std.os.fd_t,
-    dst_dir: Compilation.Directory,
-) ![]const u8 {
-    const new_name = try dst_dir.tmpFilePath(arena, "waspipe");
-    try dst_dir.handle.makePath("tmp");
-    var outfile = try dst_dir.handle.createFile(new_name, .{});
-    defer outfile.close();
-    errdefer dst_dir.handle.deleteFile(new_name) catch {};
-
-    // gnu tar uses that many bytes for its internal buffer, cargo culting
-    var buffer: [10240]u8 = undefined;
-    while (true) {
-        const amt = try std.os.read(in_fd, buffer[0..]);
-        if (amt == 0)
-            break;
-
-        var written: usize = 0;
-        while (written < amt)
-            written += try std.os.write(outfile.handle, buffer[written..amt]);
-    }
-
-    return new_name;
-}
-
 const ArgMode = union(enum) {
     build: std.builtin.OutputMode,
     cc,
@@ -3043,33 +3015,39 @@ fn buildOutputType(
         break :l global_cache_directory;
     };
 
-    var temp_files_for_comp = std.ArrayList([]const u8).init(gpa);
+    var temp_stdin_file: ?[]const u8 = null;
     defer {
-        for (temp_files_for_comp.items) |file| {
+        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 {};
         }
-        temp_files_for_comp.deinit();
     }
 
-    // if a c_source_file is "-", dump it to a tempdir and replace the src_name
-    // with the newly created file. translate-c works fine with streams; other
-    // actions don't.
-    if (arg_mode != .translate_c) {
-        for (c_source_files.items) |*src| {
-            if (!mem.eql(u8, src.src_path, "-")) continue;
+    for (c_source_files.items) |*src| {
+        if (!mem.eql(u8, src.src_path, "-")) continue;
 
-            if (src.ext == null)
-                fatal("-E or -x is required when reading from a non-regular file", .{});
+        const ext = src.ext orelse
+            fatal("-E or -x is required when reading from a non-regular file", .{});
 
-            // "-" is stdin. Dump it to a real file.
-            const new_file = try copyToFile(arena, io.getStdIn().handle, local_cache_directory);
-            try temp_files_for_comp.append(new_file);
-            src.src_path = new_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);
+
+            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;
+        };
+        temp_stdin_file = new_file;
+        src.src_path = new_file;
     }
 
     if (build_options.have_llvm and emit_asm != .no) {

test/tests.zig

@@ -781,42 +781,47 @@ pub fn addCliTests(b: *std.Build) *Step {
     // 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.
-    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);
-    }
+    //
+    // 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`.