Skip to content
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

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

Closed
georgehemmings opened this issue Oct 2, 2017 · 8 comments
Closed
Labels
Milestone

Comments

@georgehemmings
Copy link

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


@devlead
Copy link
Member

devlead commented Oct 2, 2017

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

@dchristensen
Copy link

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.

@marcus-n3rd
Copy link

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
Copy link

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

@wpspires
Copy link

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?

@Joe-Dunleavy
Copy link
Contributor

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

@Joe-Dunleavy
Copy link
Contributor

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.

@cake-build-bot
Copy link

🎉 This issue has been resolved in version v2.3.0 🎉

The release is available on:

Your GitReleaseManager bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

8 participants