Skip to content

feat: reorder the target parameter in zig wrapper #223

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
linzhp merged 3 commits into from
Jun 30, 2025
+17 −11
Merged

feat: reorder the target parameter in zig wrapper #223

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
94f75db
feat: reorder the target parameter in zig wrapper
liningpan Jun 8, 2025
b7a24fa
fix: fixed zig wrapper unit tests
liningpan Jun 13, 2025
d9b8b8c
fmt: fix format difference
liningpan Jun 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 17 additions & 11 deletions toolchain/zig-wrapper.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
//
// In simple cases it is usually enough to:
//
// zig c++ -target <triple> <...>
// zig c++ <...> -target <triple>
//
// However, there are some caveats:
//
Expand Down Expand Up @@ -227,17 +227,23 @@ fn parseArgs(

switch (run_mode) {
.wrapper => {},
.arg1 => try args.appendSlice(arena, &[_][]const u8{arg0_noexe}),
.cc => |target| try args.appendSlice(arena, &[_][]const u8{
arg0_noexe,
"-target",
target,
}),
.arg1, .cc => try args.appendSlice(arena, &[_][]const u8{arg0_noexe}),
}

while (argv_it.next()) |arg|
try args.append(arena, arg);

// Add -target as the last parameter. The wrapper should overwrite
// the target specified by other tools calling the wrapper.
// Some tools might pass LLVM target triple, which are rejected by zig.
// https://github.com/uber/hermetic_cc_toolchain/issues/222
if (run_mode == RunMode.cc) {
Copy link
Copy Markdown
Contributor

@linzhp linzhp Jun 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (run_mode == RunMode.cc) {
if (run_mode == .cc) {

shouldn't it be like this? When I test this internally at uber, cursor bot commented:

The patch incorrectly handles the RunMode.cc variant of a Zig tagged union. It uses an invalid comparison if (run_mode == RunMode.cc) and an incorrect direct access run_mode.cc to retrieve its payload, which is not how tagged union variants with data are accessed in Zig.

Copy link
Copy Markdown
Contributor Author

@liningpan liningpan Jun 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know much about zig. Based on my understanding of zig, .cc is equivalent to RunMode.cc, where the enum type is inferred (https://ziglang.org/documentation/master/#toc-Enum-Literals). It does seem like not specifying the type when it can be inferred is idiomatic in zig.

try args.appendSlice(arena, &[_][]const u8{
"-target",
run_mode.cc,
});
}

return ParseResults{ .exec = .{ .args = args, .env = env } };
}

Expand Down Expand Up @@ -363,11 +369,11 @@ test "zig-wrapper:parseArgs" {
"tools" ++ sep ++ "x86_64-linux-musl" ++ sep ++
".." ++ sep ++ ".." ++ sep ++ "zig" ++ EXE,
"c++",
"-target",
"x86_64-linux-musl",
"main.c",
"-o",
"/dev/null",
"-target",
"x86_64-linux-musl",
},
.env_zig_lib_dir = "tools" ++ sep ++ "x86_64-linux-musl" ++
sep ++ ".." ++ sep ++ ".." ++ sep ++ "lib",
Expand Down Expand Up @@ -407,11 +413,11 @@ test "zig-wrapper:parseArgs" {
.args = &[_][:0]const u8{
"external" ++ sep ++ "zig_sdk" ++ sep ++ "zig" ++ EXE,
"c++",
"-target",
"x86_64-linux-gnu.2.28",
"main.c",
"-o",
"/dev/null",
"-target",
"x86_64-linux-gnu.2.28",
},
.env_zig_lib_dir = "external" ++ sep ++ "zig_sdk" ++
sep ++ "lib",
Expand Down
Loading