cake icon indicating copy to clipboard operation
cake copied to clipboard

Incorrect escaping of semi-colon in property values for MS Build

Open georgehemmings opened this issue 8 years ago • 7 comments

What You Are Seeing?

When using .WithProperty("DefineConstants", "A=a;B=b".Quote()); the semi-colon is escaped which is invalid. The escaping behaviour was added in #1625

What is Expected?

The semi-colon should not be escaped in this scenario as the resulting command is invalid.

What version of Cake are you using?

0.22.2

Are you running on a 32 or 64 bit system?

64

What environment are you running on? Windows? Linux? Mac?

Windows

Are you running on a CI Server? If so, which one?

TeamCity

How Did You Get This To Happen? (Steps to Reproduce)

MS Build with property.

.WithProperty("DefineConstants", "A=a;B=b".Quote());

Output Log

Executing: "C:/Program Files (x86)/MSBuild/14.0/Bin/MSBuild.exe" /v:minimal /p:Configuration="Release" /p:Platform=x86 /p:WarningLevel=0 /p:DefineConstants="A=a%3BB=b" /target:Rebuild "C:/code/x/src/x/xSetup.wixproj"
Microsoft (R) Build Engine version 14.0.25420.1
Copyright (C) Microsoft Corporation. All rights reserved.

C:\code\x\src\xSetup\x.wxs(15): error CNDL0150: Undefined preprocess
or variable '$(var.ProductName)'. [C:\code\x\src\xSetup\xSetup.wixpr
oj]
An error occurred when executing task 'Package'.
Error: Cake.Core.CakeException: MSBuild: Process returned an error (exit code 1).
   at Cake.Core.Tooling.Tool`1.ProcessExitCode(Int32 exitCode)
   at Cake.Core.Tooling.Tool`1.Run(TSettings settings, ProcessArgumentBuilder arguments, ProcessSettings processSettings, Action`1 postAction)
   at Cake.Common.Tools.MSBuild.MSBuildAliases.MSBuild(ICakeContext context, FilePath solution, MSBuildSettings settings)
   at Submission#0.<.ctor>b__2()
   at Cake.Core.ActionTask.Execute(ICakeContext context)
   at Cake.Core.DefaultExecutionStrategy.Execute(CakeTask task, ICakeContext context)
   at Cake.Core.CakeEngine.ExecuteTask(ICakeContext context, IExecutionStrategy strategy, Stopwatch stopWatch, CakeTask task, CakeReport report)
   at Cake.Core.CakeEngine.RunTarget(ICakeContext context, IExecutionStrategy strategy, String target)
   at Cake.Scripting.BuildScriptHost.RunTarget(String target)
   at Submission#0..ctor(Session session, Object& submissionResult)
   at Submission#0.<Factory>(Session session)
   at Roslyn.Scripting.CommonScriptEngine.Execute[T](String code, String path, DiagnosticBag diagnostics, Session session, Boolean isInteractive)
   at Roslyn.Scripting.Session.Execute(String code)
   at Cake.Core.Scripting.ScriptRunner.Run(IScriptHost host, FilePath scriptPath, IDictionary`2 arguments)
   at Cake.Commands.BuildCommand.Execute(CakeOptions options)
   at Cake.CakeApplication.Run(CakeOptions options)
   at Cake.Program.Main()


georgehemmings avatar Oct 02 '17 11:10 georgehemmings

@cake-build/cake-team I kinda feel this WithProperty("DefineConstants", "A=a", "B=b"); should generate /p:DefineConstants=A=a;B=b that way we support both escaping and multiple values in a typed way.

Potentially one could have an dictionary of properties that doesn't support multiple properties beginning with DefineConstants.

devlead avatar Oct 02 '17 11:10 devlead

I'm running into this issue as well, and I agree with @devlead that when passing multiple values into WithProperty they should be combined with a separating ; to support this scenario. Defining multiple values for the same parameter name on the command line doesn't make much sense anyway since only the last value will be available in the MSBuild script.

dchristensen avatar Nov 27 '18 18:11 dchristensen

I am having the same issue trying to pass the ExcludeFilesFromDeployment property with multiple files as the value and this functionality will help immensely.

marcus-n3rd avatar Nov 27 '18 20:11 marcus-n3rd

Though, I did find a way to workaround it using the ArgumentCustomization property:

string excludeFiles = "\"" + string.Join(";", configuration.ExcludeFilesFromDeployment) + "\"";
MSBuildSettings settings = new MSBuildSettings() {
  ArgumentCustomization = args=>args.Append("/p:ExcludeFilesFromDeployment=" + excludeFiles)
};

marcus-n3rd avatar Nov 27 '18 21:11 marcus-n3rd

Hi team, How do you want this handled? Should semicolon just not be escaped? I could just remove it from the dictionary of escaped chars. Or are there conditions in which it should be kept vs removed?

wpspires avatar Oct 11 '21 03:10 wpspires

Hi, I'm willing to give this a shot. I will keep you updated on progress.

Joe-Dunleavy avatar Jul 22 '22 12:07 Joe-Dunleavy

Please see #1852 for proposed changes.

If this is not quite what you had in mind -- or if there are any issues, please let me know and I will do my best to amend them.

Joe-Dunleavy avatar Jul 25 '22 16:07 Joe-Dunleavy

:tada: This issue has been resolved in version v2.3.0 :tada:

The release is available on:

Your GitReleaseManager bot :package::rocket:

cake-build-bot avatar Oct 14 '22 11:10 cake-build-bot