dotnet / command-line-api

Command line parsing, invocation, and rendering of terminal output.
https://github.com/dotnet/command-line-api/wiki
MIT License
3.43k stars 384 forks source link

Response files not working correctly #1633

Open jakubch1 opened 2 years ago

jakubch1 commented 2 years ago

Look at below output from console. I think rsp file is treated just as 1 argument not as command, options and arguments. Output is from latest beta version.

D:\devdiv\vscodecoverage\artifacts>more cov.rsp
merge -r *.coverage

D:\devdiv\vscodecoverage\artifacts>dotnet .\bin\Microsoft.CodeCoverage.Console\Release\netcoreapp3.1\Microsoft.CodeCoverage.Console.dll [parse] merge -r *.coverage
[ codecoverage [ merge [ -r  ] <*.coverage> *[ --output <output.coverage> ] *[ --output-format <coverage> ] ] ]

D:\devdiv\vscodecoverage\artifacts>dotnet .\bin\Microsoft.CodeCoverage.Console\Release\netcoreapp3.1\Microsoft.CodeCoverage.Console.dll [parse] @cov.rsp
![ codecoverage ]   ???--> merge -r *.coverage
jonsequitur commented 2 years ago

~Could you post the response file that produced this?~

Sorry, I see it.

By default, response files are parsed as line-separated. Each line is consumed as a single token. Part of the reasoning is that this lets you avoid having to deal with shell escaping. Using the response file in your example should be the equivalent of passing the following command line:

D:\devdiv\vscodecoverage\artifacts>dotnet .\bin\Microsoft.CodeCoverage.Console\Release\netcoreapp3.1\Microsoft.CodeCoverage.Console.dll "merge -r *.coverage"

To use the response file to get the equivalent of your second example, each token should be separated by a newline:

merge 
-r 
*.coverage
baronfel commented 2 years ago

If you prefer the space-delimited, I think this setting applied to this builder method should do the same thing.

jonsequitur commented 2 years ago

I've wondered whether the end user should be in control of this rather than the tool author. I brought this idea up a couple of years back and gathered it was a little unorthodox. But I do see that the tool author's intent isn't discoverable here.

jakubch1 commented 2 years ago

I think it's good idea or even you could parse line by line and if failing try also with space separators. It would be also good to add example into your documentation. For me more natural was to use spaces. I didn't even think about putting in lines and same story for end user of our tool who reported this to us.

vaboca commented 2 years ago

This is very counterintuitive and not how other tools work (see cl.exe, link.exe, etc) It doesn't matter if you use spaces or new lines, the parsing should normalize new lines to spaces and pass this to the command line parser engine as it would have come naturally. We need this feature to support explicit file names (in the order of thousands) and to avoid command line length limitations.

vaboca commented 2 years ago

Even more, it seems some tools don't support at all new lines: https://docs.microsoft.com/en-us/windows/win32/midl/response-files#:~:text=A%20response%20file%20is%20a,convenience%20for%20your%20build%20process. https://docs.microsoft.com/en-us/cpp/build/reference/at-specify-a-compiler-response-file?view=msvc-170

jonsequitur commented 2 years ago

I think it's good idea or even you could parse line by line and if failing try also with space separators.

I believe this would be ambiguous as many response files could produce valid parse results either way.