-
-
Notifications
You must be signed in to change notification settings - Fork 1k
feat: Add logics to escape MSBuildArgument text representation when contains MSBuild special chars #2730
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.
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
Conversation
I'm confused by this PR. The implementation does not match the MSBuild documentation for special characters. Wrapping values with quotes instead just works? Is it documented anywhere? Why does comma need to be escaped also? And why is it only escaping if the value contains *=*
and no other case?
Wrapping values with quotes instead just works?
It just works when using quoted argument. (both with/without %
escape)
Is it documented anywhere?
No official documentation found that relating to this behaviors.
MSBuild command-line reference page has following statement.
If you run MSBuild from a shell other than the Windows command prompt, lists of arguments to a switch (separated by semicolons or commas) might need single or double quotes to ensure that lists are passed to MSBuild instead of interpreted by the shell.
And MSBuild team suggesting to use this workaround(dotnet/sdk#8792 (comment))
Why does comma need to be escaped also?
It's not listed on MSBuild special characters.
But comma
can be used as separator char on some arguments. and requires quites/escape.
(List of argument that support comma can be found by search comma
on https://learn.microsoft.com/en-us/visualstudio/msbuild/msbuild-command-line-reference?view=vs-2022
)
And why is it only escaping if the value contains = and no other case?
It's because other arguments seems to accepts raw ;
separator char without quotes/escape.
(e.g. -getProperty:Prop1;Prop2
-noWarn:123;456)
Wrapping values with quotes instead just works?
It just works when using quoted argument. (both with/without % escape)
I've confirmed it's not works when escaped ;
separator chars(%3B
) are passed.
It works when running from command line.
But when running via .bat
file. It need to escape %3B
to %%%%3B
for .bat
file. (double escape is required because BenchmarkDotNet use call
to invoke dotnet command)
I thought this issue also applied to PR #2729
.
It might be better to keep default MSBuildArgument
behavior.
And enable escape/quotes as opt-in options.
I'll update this PR later.
In that case, would it be better to simply wrap the entire argument with quotes? We don't actually use bat files, we just run the command directly (I think those files are used for debug purposes only).
In that case, would it be better to simply wrap the entire argument with quotes?
As far as I've confirmed. It's not works without %
escapes when wrapping entire argument with quotes.
We don't actually use bat files, we just run the command directly (I think those files are used for debug purposes only).
Thanks for letting me know.
I've confirmed these behaviors with generated .bat
/.sh
with keepFiles options.
I also try to confirm behaviors with DotNetCliBuilder
/DotNetCliCommand
.
I've updated MSBuildArgument escape logics.
Escape Logic changes
- Argument quoting is unified to
\"
on both Windows/non-Windows environment. - Escape following 3 characters. (These char needs escaping when used inside
\"{value}\"
syntax)
2.1. Space (%20
)
2.2. Double Quote (%22
)
2.3. BackSlash (%5C
) - If escaped value contains [MSBuild Special characters]. return value surrounded with
\"
.
(Otherwise returns original argument)
Other changes
1. Change MsBuildArgument
escaping feature to opt-in
Escaping feature is enabled when passing escape: true
to constructor.
2. Add integration tests to BenchmarkDotNet.IntegrationTests.ManualRunning
projects.
Add tests to verify MSBuildArgument
escaped value is properly passed as MSBuild property.
4f7971b
to
395c27a
Compare
395c27a
to
5266994
Compare
Uh oh!
There was an error while loading. Please reload this page.
This PR intended to fix issue #2719.
There is existing PR(#2729) that trying to resolve issue by escaping MSBuild special chars
Instead, this PR try to resolve issue by surrounding MSBuild parameter value by quotes.
(See: dotnet/sdk#8792 (comment) for details)
What's changed in this PR
1. Argument.cs
Add internal method
GetEscapedTextRepresentation
toMsBuildArgument
.This method returns platform-dependent text representation that is used for script file.
DotNetCliCommand.cs
Modify code to pass
GetEscapedTextRepresentation
value instead ofTextRepresentation
MsBuildArgumentTests.cs
Add unit tests for various argument patterns.
Additional integration tests
I've confirmed benchmarks works as expected both Windows/Ubuntu(on WSL) environments.