Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Add support for response files
I make workaround here for now, but essentially issues in CommadLine library.
Also place workardoud for stupid bug with quoted argumetns with space in them.

I notice issue with parsing and request for RSP on Discord where @kg and @radical discuss how to feed proper parameters in `benchmarks_ci.py`
  • Loading branch information
kant2002 committed Jun 5, 2023
commit d00d191a0829f77a2c8d8c307843cc2fdb80cd00
96 changes: 96 additions & 0 deletions src/BenchmarkDotNet/ConsoleArguments/ConfigParser.cs
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;
}
}

private static Parser CreateParser(ILogger logger)
=> new Parser(settings =>
{
Expand Down
32 changes: 32 additions & 0 deletions tests/BenchmarkDotNet.Tests/ConfigParserTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -586,5 +586,37 @@ public void UsersCanSpecifyWithoutOverheadEvalution()
Assert.False(job.Accuracy.EvaluateOverhead);
}
}

[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);
}
}
}
}