Add Response File Capabilities to XUnit Console Runner #1843
Conversation
* consume a path prefixed by the @ symbol * file should be a space/tab delimited list of command line arguments (may be on multiple lines) * does not check file extension but typically it is .rsp
| @@ -109,13 +109,29 @@ static bool IsConfigFile(string fileName) | |||
| public static CommandLine Parse(params string[] args) | |||
| => new CommandLine(args); | |||
|
|
|||
| protected void ParseRspFile(string path) | |||
| { | |||
| if (!File.Exists(path)) | |||
There was a problem hiding this comment.
You don't need a File.Exists check here as File.ReadLines already throws a corresponding FileNotFoundException if the file doesn't exist.
|
|
||
| foreach (string line in File.ReadLines(path)) | ||
| { | ||
| foreach (var arg in line.Split(new char[] { ' ', '\t' }, StringSplitOptions.RemoveEmptyEntries).Reverse()) |
There was a problem hiding this comment.
This does not work with paths that contain spaces. CommandLineUtils developed by @natemcmaster does this really well. Please take a look at his parser for the correct implementation: https://github.com/natemcmaster/CommandLineUtils/blob/f498cc7383b27730fd24486510573ab61ccab9d6/src/CommandLineUtils/Internal/ResponseFileParser.cs
There was a problem hiding this comment.
+1 - I share @ViktorHofer's concern. Splitting on spaces alone is insufficient for response file parsing. How would a caller escape parameters with spaces in them?
There was a problem hiding this comment.
Good point! Will modify appropriately.
|
/AzurePipelines run arcade-ci |
|
Success! 1 created and queued. |
There was a problem hiding this comment.
FYI @josalem -- MC is showing some test failures for your build. The build is passing because there's a known issue where we can't download test results to builds triggered from forks; in the meantime, make sure you examine those and get them fixed before merging
|
@josalem -- looks like the failures were not caused by your PR but by something that was already merged, so ignore what I said for now |
… characters and spaces * Add attribution for response file reader code, and modified response file parser
|
I know that there are multiple modes in @natemcmaster's CommandLineUtils rsp implementation. Which mode did you port? The line one? Do we also need the other one? |
I ported the "space delimited" version, since it is more flexible. It reads the file line-by-line and parses each line so any combination of new-line delimited and space delimited args should work, e.g., and so I didn't see the point in including both impls. |
|
LGTM if you have tested it locally and it works fine. Thanks for doing this! |
@symbol(may be on multiple lines)
.rsp#1613
CC - @sergiy-k