Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Open
filzrev wants to merge 2 commits into dotnet:master
base: master
Choose a base branch
Loading
from filzrev:feat-escape-msbuild-argument

Conversation

Copy link
Contributor

@filzrev filzrev commented May 15, 2025
edited
Loading

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 to MsBuildArgument.
This method returns platform-dependent text representation that is used for script file.

  1. DotNetCliCommand.cs
    Modify code to pass GetEscapedTextRepresentation value instead of TextRepresentation

  2. 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.

public class CustomConfig : ManualConfig
{
 public CustomConfig()
 {
 WithOptions(ConfigOptions.DisableOptimizationsValidator);
 WithOptions(ConfigOptions.StopOnFirstError);
 WithOptions(ConfigOptions.KeepBenchmarkFiles);
 WithOptions(ConfigOptions.GenerateMSBuildBinLog);
 WithUnionRule(DefaultConfig.Instance.UnionRule);
 var baseJob = Job.ShortRun
 .WithStrategy(RunStrategy.Monitoring)
 .DontEnforcePowerPlan()
 .Freeze();
 AddJob(baseJob.WithArguments([new MsBuildArgument("/p:DefineConstants2=V1;V111")]).WithId("V1"));
 AddJob(baseJob.WithArguments([new MsBuildArgument("/p:DefineConstants2=V2;V222")]).WithId("V2"));
 AddLogger(ConsoleLogger.Default);
 AddExporter(DefaultConfig.Instance.GetExporters().ToArray());
 AddAnalyser(DefaultConfig.Instance.GetAnalysers().ToArray());
 AddColumnProvider(DefaultColumnProviders.Instance);
 }

Copy link
Collaborator

timcassell commented May 15, 2025
edited
Loading

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?

Copy link
Contributor Author

filzrev commented May 16, 2025

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)

Copy link
Contributor Author

filzrev commented May 16, 2025

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.

Copy link
Collaborator

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).

Copy link
Contributor Author

filzrev commented May 16, 2025

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.

Copy link
Contributor Author

filzrev commented May 20, 2025
edited
Loading

I've updated MSBuildArgument escape logics.

Escape Logic changes

  1. Argument quoting is unified to \" on both Windows/non-Windows environment.
  2. 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)
  3. 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.

@filzrev filzrev force-pushed the feat-escape-msbuild-argument branch from 4f7971b to 395c27a Compare May 20, 2025 15:13
Copy link
Collaborator

I think this would be better as a combo of #2729 (escape all special characters opt-in) and #2732 (new type specifically for MsBuild properties). I do like how you handle the separation of display and command line here, though.

filzrev reacted with thumbs up emoji

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /