Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add support for response files #2320

Merged
merged 3 commits into from
Jun 30, 2023
Merged

Add support for response files #2320

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d00d191
Add support for response files
kant2002 Jun 5, 2023
313f61b
Merge branch 'master' into kant/rsp-support
kant2002 Jun 30, 2023
f04b01e
Address PR feedback
kant2002 Jun 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
96 changes: 96 additions & 0 deletions src/BenchmarkDotNet/ConsoleArguments/ConfigParser.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
using System.Diagnostics.CodeAnalysis;
using System.IO;
using System.Linq;
using System.Text;
using BenchmarkDotNet.Columns;
using BenchmarkDotNet.Configs;
using BenchmarkDotNet.Diagnosers;
Expand Down Expand Up @@ -75,6 +76,7 @@ public static (bool isSuccess, IConfig config, CommandLineOptions options) Parse
{
(bool isSuccess, IConfig config, CommandLineOptions options) result = default;

args = ExpandResponseFile(args).ToArray();
using (var parser = CreateParser(logger))
{
parser
Expand All @@ -86,6 +88,100 @@ public static (bool isSuccess, IConfig config, CommandLineOptions options) Parse
return result;
}

private static IEnumerable<string> ExpandResponseFile(string[] args)
{
foreach (var arg in args)
{
if (arg.StartsWith("@"))
{
var fileName = arg.Substring(1);
if (File.Exists(fileName))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if the given file doesn't exist? I believe we should warn the user about that rather than just silently ignore such an argument.

{
var lines = File.ReadAllLines(fileName);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please add try..catch with proper exception handling? If there are some issues with the given file (e.g., we don't have permission to read the file content), it would be nice to print an error message rather than just throwing an exception.

foreach (var line in lines)
{
foreach (var token in ConsumeTokens(line))
yield return token;
}
}
}
else
{
if (arg.Contains(' '))
{
// Workaround for CommandLine library issue with parsing these kind of args.
yield return " " + arg;
}
else
{
yield return arg;
}
}
}
}

private static IEnumerable<string> ConsumeTokens(string line)
{
bool insideQuotes = false;
var token = new StringBuilder();
for (int i = 0; i < line.Length; i++)
{
char currentChar = line[i];
if (currentChar == ' ' && !insideQuotes)
{
if (token.Length > 0)
{
yield return GetToken();
token = new StringBuilder();
}

continue;
}

if (currentChar == '"')
{
insideQuotes = !insideQuotes;
continue;
}

if (currentChar == '\\' && insideQuotes)
{
if (line[i + 1] == '"')
{
insideQuotes = false;
i++;
continue;
}

if (line[i + 1] == '\\')
{
token.Append('\\');
i++;
continue;
}
}

token.Append(currentChar);
}

if (token.Length > 0)
{
yield return GetToken();
}

string GetToken()
{
var result = token.ToString();
if (result.Contains(' '))
{
// Workaround for CommandLine library issue with parsing these kind of args.
return " " + result;
}

return result;
}
}

internal static bool TryUpdateArgs(string[] args, out string[]? updatedArgs, Action<CommandLineOptions> updater)
{
(bool isSuccess, CommandLineOptions options) result = default;
Expand Down
32 changes: 32 additions & 0 deletions tests/BenchmarkDotNet.Tests/ConfigParserTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -587,6 +587,38 @@ public void UsersCanSpecifyWithoutOverheadEvalution()
}
}

[Fact]
public void UserCanSpecifyWasmArgs()
{
var parsedConfiguration = ConfigParser.Parse(new[] { "--runtimes", "wasm", "--wasmArgs", "--expose_wasm --module" }, new OutputLogger(Output));
Assert.True(parsedConfiguration.isSuccess);
var jobs = parsedConfiguration.config.GetJobs();
foreach (var job in parsedConfiguration.config.GetJobs())
{
var wasmRuntime = Assert.IsType<WasmRuntime>(job.Environment.Runtime);
Assert.Equal(" --expose_wasm --module", wasmRuntime.JavaScriptEngineArguments);
}
}

[Fact]
public void UserCanSpecifyWasmArgsViaResponseFile()
{
var tempResponseFile = Path.GetRandomFileName();
File.WriteAllLines(tempResponseFile, new[]
{
"--runtimes wasm",
"--wasmArgs \"--expose_wasm --module\""
});
var parsedConfiguration = ConfigParser.Parse(new[] { $"@{tempResponseFile}" }, new OutputLogger(Output));
Assert.True(parsedConfiguration.isSuccess);
var jobs = parsedConfiguration.config.GetJobs();
foreach (var job in parsedConfiguration.config.GetJobs())
{
var wasmRuntime = Assert.IsType<WasmRuntime>(job.Environment.Runtime);
Assert.Equal(" --expose_wasm --module", wasmRuntime.JavaScriptEngineArguments);
}
}

[Theory]
[InlineData("--filter abc", "--filter *")]
[InlineData("-f abc", "--filter *")]
Expand Down