-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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.
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
More response file support #9654
Conversation
@andrewrk You mentioned wanting zig-based Response file support. I believe this PR should be what you are looking for, but let me know if there is more support you want added. |
3127c70
to
92c8d7d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@PhaseMage thanks for your patience on getting a review for this. I'm definitely interested in merging this, but I do have a couple of requests first.
Also I hope you don't mind I took the liberty to rebase this for you, made a couple of trivial edits, and force pushed to your branch.
92c8d7d
to
69a7406
Compare
I can spend more time on this over the weekend. |
2b763ac
to
bf236a7
Compare
CI appears to be passing now (ignoring the single failure in ziglang.zig / BuildWindows, which appears to be an infrastructure failure, it was passing earlier). Let me know if there's anything else you'd like me to change. |
I hit the "quotes in an RSP file" issue when trying to compile gRPC using "zig cc". As a fun exercise, I decided to see if I could fix it myself. I'm fully open to this code being flat-out rejected. Or I can take feedback to fix it up. This modifies (and renames) _ArgIteratorWindows_ in process.zig such that it works with arbitrary strings (or the contents of an RSP file). In main.zig, this new _ArgIteratorGeneral_ is used to address the "TODO" listed in _ClangArgIterator_. This change closes ziglang#4833. **Pros:** - It has the nice attribute of handling "RSP file" arguments in the same way it handles "cmd_line" arguments. - High Performance, minimal allocations - Fixed bug in previous _ArgIteratorWindows_, where final trailing backslashes in a command line were entirely dropped - Added a test case for the above bug - Harmonized the _ArgIteratorXxxx._initWithAllocator()_ and _next()_ interface across Windows/Posix/Wasi (Moved Windows errors to _initWithAllocator()_ rather than _next()_) - Likely perf benefit on Windows by doing _utf16leToUtf8AllocZ()_ only once for the entire cmd_line **Cons:** - Breaking Change in std library on Windows: Call _ArgIterator.initWithAllocator()_ instead of _ArgIterator.init()_ - PhaseMage is new with contributions to Zig, might need a lot of hand-holding - PhaseMage is a Windows person, non-Windows stuff will need to be double-checked **Testing Done:** - Wrote a few new test cases in process.zig - zig.exe build test -Dskip-release (no new failures seen) - zig cc now builds gRPC without error
bf236a7
to
cf59ff4
Compare
I rebased this to master and made |
Thanks @PhaseMage and @Vexu for landing this improvement! |
I hit the "quotes in an RSP file" issue when trying to compile gRPC using
zig cc
.This modifies (and renames)
ArgIteratorWindows
in process.zig such that it works with arbitrary strings (or the contents of an RSP file).In main.zig, this new
ArgIteratorGeneral
is used to address the "TODO" listed inClangArgIterator
.This change is meant to close out issue #4833
Benefits:
cmd_line
arguments.ArgIteratorWindows
, where final trailing backslashes in a command line were entirely droppedArgIteratorXxxx.initWithAllocator()
andnext()
interface across Windows/Posix/Wasi (Moved Windows errors toinitWithAllocator()
rather thannext()
)utf16leToUtf8AllocZ()
only once for the entire cmd_lineBreaking Change in std library on Windows: Call
ArgIterator.initWithAllocator()
instead ofArgIterator.init()
Testing Done:
zig.exe build test -Dskip-release
(no new failures seen)zig cc
now builds gRPC without error