sdk icon indicating copy to clipboard operation
sdk copied to clipboard

revert perma-escaping of semicolons and support semicolon-delimited unquoted property values

Open baronfel opened this issue 3 years ago • 1 comments

Description

In earlier SDKs we added explicit support for the MSBuild -property option to the CLI's parser, in order to include it in help output and smooth over a couple rough edges in the option syntax in MSBuild. In addition, adding this option allowed internal SDK code to start parsing and inspecting the keys and values of user-supplied Properties, something we started taking advantage of in several changes for .NET 7. However, the parser for these keys and values did not support unquoted values that contained semicolons, as is the case in situations like -p RuntimeIdentifiers=linux-x64;macos-arm64. We should support this, as MSBuild does support this.

In addition, we unified the escaping of semicolon-delimited values for all properties, resulting in a property like -p RuntimeIdentifiers="linux-x64;osx-arm64" being forwarded to MSBuild as -property:RuntimeIdentifiers="linux-x64%3Bosx-arm64". As a result, the users' intent (a list of RIDs) was not honored. This change was made after looking at the other uses of semicolon-escaping in the CLI codebase, but it was improper to generalize it to all user inputs. The rules that we should follow for semicolon escaping have been codified as a result.

This change removes the automatic semicolon escaping, as well as adding support for semicolon-delimited property values.

Closes https://github.com/dotnet/sdk/issues/28131.

If approved, we'll want to ensure quick flow + merge to 7.0.100 for sure, potentially 7.0.100-rc2 as well depending on time remaining.

Customer impact

Customers could not specify property lists via the CLI, and some forms of values could not be forwarded at all untouched by the CLI parser. Often this surfaces in runtime identifiers, or assembly search paths. Customers could workaround this in some cases by setting the value in an environment variable, since those are automatically used by MSBuild if the name of the variable matches the MSBuild property name, but this is an awkward workaround at best.

Regression

Yes, this mangling didn't occur in prior SDK version bands.

Risk

Low, we have even more automated parser test coverage over these scenarios now

Testing

Previous test cases showing the semicolon-escaping were updated to check for non-escaping, and new cases were added to cover the list-of-values scenarios for both quoted and unquoted property values.

baronfel avatar Sep 13 '22 20:09 baronfel

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.

If approved, do not merge yet as 6.0 branding is still blocked.

baronfel avatar Oct 06 '22 16:10 baronfel

Do you also want to take this for 7.0.1xx GA?

dsplaisted avatar Oct 06 '22 16:10 dsplaisted

Yes for 7.0 - I was going for approval for both today at tactics. Wasn't sure if I should rely on codeflow from 6.0.400 to 7.0.100 or if I'd need to cherry-pick.

baronfel avatar Oct 06 '22 17:10 baronfel