Add necessary space before "-r" extra argument for CoreRT

[Beau-Gosse-dev]

Without this, the commands come out like dotnet restore /p:DebugType=portable -bl:benchmarkdotnet.binlog-r win-x64 /p:UseSharedCompilation=false /p:BuildInParallel=false /m:1 /p:Deterministic=true /p:Optimize=true and dotnet fails to restore/build/publish

[adamsitnik]

Hi @Beau-Gosse-dev

Big thanks for you contribution! I am very excited to see you working on getting CoreRT/NativeAOT working with BenchmarkDotNet!

You fix solves the problem for CoreRT, but it would be great if we could make it more generic so it could be applied to other toolchains as well.

Since the extra arguments are appended here:

BenchmarkDotNet/src/BenchmarkDotNet/Toolchains/DotNetCli/DotNetCliCommand.cs

Lines 142 to 144 in 9b990df

	.Append(GetCustomMsBuildArguments(buildPartition.RepresentativeBenchmarkCase, buildPartition.Resolver))
	.Append(extraArguments)
	.Append(GetMandatoryMsBuildSettings(buildPartition.BuildConfiguration))

after the custom MsBuild arguments, I think it would be better to append space to the custom msbuild arg:

BenchmarkDotNet/src/BenchmarkDotNet/Toolchains/DotNetCli/DotNetCliCommand.cs

Line 172 in 9b990df

return string.Join(" ", msBuildArguments.Select(arg => arg.TextRepresentation));

- return string.Join(" ", msBuildArguments.Select(arg => arg.TextRepresentation));
+ return string.Join(" ", msBuildArguments.Select(arg => arg.TextRepresentation)) + ' ';

What do you think?

[Beau-Gosse-dev]

@adamsitnik Thanks for the quick review! I see what you mean about making it more generic, and not just limiting this to CoreRT. I actually first put the extra space inside the DotNetCliCommand.GetRestoreCommand method, but then realized it would be needed inside Build and Publish also. I don't love the idea of putting the extra space at the end of GetCustomMsBuildArguments because then the extraArguments tightly depends on GetCustomMsBuildArguments being directly before extraArguments in the list of arguments. If someone was to change or move GetCustomMsBuildArguments later, that could break this again.

What do you think about instead creating an extension method called AppendCliArgument or just AppendArgument which would check that any argument being added has a leading space. Then we can replace the calls to Append with AppendArgument so it would look like this:

new StringBuilder()
                .AppendArgument("restore")
                .AppendArgument(string.IsNullOrEmpty(artifactsPaths.PackagesDirectoryName) ? string.Empty : $"--packages \"{artifactsPaths.PackagesDirectoryName}\"")
                .AppendArgument(GetCustomMsBuildArguments(buildPartition.RepresentativeBenchmarkCase, buildPartition.Resolver))
                .AppendArgument(extraArguments)
                .AppendArgument(GetMandatoryMsBuildSettings(buildPartition.BuildConfiguration))
                .ToString()

It seems like there are a lot of places where either leading or trailing spaces are hard-coded as magic strings, which seems dangerous and inconsistent. Such as these places:
Leading space
Trailing space
Leading space
Trailing space

To me, it seems better to just deal with the spaces between arguments in one place, like this proposed AppendArgument method.

Thoughts?

[adamsitnik]

What do you think about instead creating an extension method called AppendCliArgument or just AppendArgument which would check that any argument being added has a leading space.

It's a very good idea! 👍 AppendArgument sounds best for me!

[Beau-Gosse-dev]

@adamsitnik I've opened a new PR with the changes we talked about plus some other cleanup and the other output path fix. It's here: #1955

I thought it would be easier to have it all in one place with a better-named branch.