-
-
Notifications
You must be signed in to change notification settings - Fork 108
Escape absolute paths with Z: also in response files #96
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
base: master
Are you sure you want to change the base?
Escape absolute paths with Z: also in response files #96
Conversation
|
Can @huangqinjin have a look at this? |
|
@marcoesposito1988 could you please provide some rsp files here for reference? You can pass |
Here is an example: |
|
@marcoesposito1988 thanks. Could you please also provide an example of |
Here: |
|
@marcoesposito1988 Thanks again.
|
Hmm, why do you want the
That's probably a good thing to do. FWIW, in the existing rewrite for actual arguments, we have a bit of a heuristic for what to convert; if I'd like to see it written out, either here in the PR or perhaps rather as a code comment, what cases the regex is designed around and how it tries to handle them. |
To keep it simple and correct, I think we should restrict the rewritten to only
|
That sounds reasonable - there's much more seldom any need for response files for compilation. It'd be good to clarify these design aspects in a code comment. |
|
Then we need a way to detect which tool is running in |
Ah, right. Or what if we just have patterns that primarily match linker commands, is there any risk of it breaking anything if it would be applied on cl.exe? Or is it just the case that we wouldn't end up handling all the tricky cases in cl.exe command lines? |
We are rewriting |
Right, that's true - I agree. |
a4b37a1 to
c6bcee4
Compare
|
Hi @mstorsjo and @huangqinjin, I updated the PR with the suggestions I got from our discussion (using portable sed flags and the new regex, only apply the change when the tool is Can you please review the changes again? |
Solves the issue discussed in #93