feat: reorder the target parameter in zig wrapper#223
Conversation
|
|
|
This is a good solution to the issue, thanks. However looks like you'll need to fix the tests and sign the CLA. |
|
CI looks like a flaky failure when downloading protobuf. Could you please rerun it? |
|
bump on this pr @cameron-martin |
I don't have any powers in this repo, I'm just somebody who wants the issue fixed too. |
|
@linzhp I see you have active commit in this repo recently. Could you please help review the PR? |
|
@TroyKomodo If you need a quick fix, it is possible to patch the zig wrapper using git override. I provided my patch in the original issue in |
|
I can take a look later this week |
I am, thanks! |
| // 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) { |
There was a problem hiding this comment.
| 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.
There was a problem hiding this comment.
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.
5 participants
Closes #222