-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Parser for actual MSBuild Property option syntax #27086
Parser for actual MSBuild Property option syntax #27086
Conversation
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
7a222fa
to
44976af
Compare
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to break it from many different angles and could not break it. If Rainer has validated it from the MSBuild syntax side, LGTM. Not sure if you need approval from a servicing person as well.
src/Tests/dotnet.Tests/ParserTests/MSBuildArgumentCommandLineParserTests.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
Adds a parser that can handle all the msbuild property variations, so that arguments that contain a semicolon- delimited key/value list are correctly escaped.
Adds a parser that can handle all the msbuild property variations, so that arguments that contain a semicolon- delimited key/value list are correctly escaped.
Adds a parser that can handle all the msbuild property variations, so that arguments that contain a semicolon- delimited key/value list are correctly escaped.
Description
Fix issues forwarding a complex variation of allowable input for the MSBuild
-p
(Property) option, which is parsed, escaped, and forwarded to MSBuild as a feature of the SDK.Customer Impact
In AzDO (and likely other scenarios), users-defined properties are passed with a single
-p
option, in a semicolon-delimited list of key/value pairs. Enforcement/unification of the property option in the 6.0.3xx series of SDKs introduced a regression in this parsing that resulted in malformed key/value pairs being passed to user builds. The most common effect of this was to prevent the build from running due to invalid CLI argument syntax.Closes #27059
The SDK property option didn't know about the semicolon- and comma-delimited forms of MSBuild properties. This adds an actual parser to validate this shaping, so that we can intelligently do the semi-colon escaping that we aimed to do originally, without also inadvertently escaping the semicolon-delimited property list use case for
-p
.The core tension here is the SDK wants to do escaping of semicolons inside property values so that those properties aren't interpreted incorrectly by the MSBuild CLI parser. This is a historical pain point for users, which is why we started doing this back in the day. This has been done through the Property Option in the SDK, and over time our parsing of the value portions of this CLI flag has grown.
Regression
Yes
Risk
Medium. The fix was to create a new parser for the syntax of the MSBuild Property option, versus the naïve string splitting that was done in the past. This is a nontrivial amount of code.
Testing
The new tests for this forwarding now cover a few additional cases that I verified with Rainer and Ben - quoted property values that contain semicolons and spaces, and unquoted values signaling a new property name. Automated testing was done with all known variants of the
-p
argument syntax. The existing cases were:And the following cases were added: