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

Revert to token-per-line response file handling #26204

Merged
merged 12 commits into from
Jul 6, 2022

Conversation

baronfel
Copy link
Member

@baronfel baronfel commented Jun 23, 2022

An update to System.CommandLine changed the response file handling support to a different, more cross-platform-consistent logic. However, we cannot take this change because users expect a token-per-line style of processing. As a result, we change the dotnet CLI to use token-per-line handling for response files.

Description

There are two issues resolved here:

  • A change to the defaults in System.CommandLine resulted in response-file parsing breaking across versions. This was resolved by using the S.CL extension points to implement the old, one-token-per-line behavior.
  • A user scenario used a version of the MSBuild --property option (/p) that the .NET SDK didn't have handling for. This was resolved by adding in handling for all unhandled forms of this property.

Fixes #26026.

Customer Impact

Customers that relied on the old response-file behavior typically ran into it in one of two scenarios - the first is related to an internal Brotli compression tool, the workaround here is to publish your application in a directory that contains no spaces. The second involved response files with the /p flag - the workaround here is to modify the response file to convert /p into one of the recognized forms (-p, --p, -property).

Regression?

  • Yes
  • No

Risk

  • High
  • Medium
  • Low

The change touches response file handling, which can be very broadly used. It does however have testing and reimplements prior behavior.

Verification

  • Manual (required)
  • Automated

Manually tested the provided user scenarios, and also added automated testing around the parsing changes.

Packaging changes reviewed?

  • Yes
  • No
  • N/A

@baronfel baronfel added Area-CLI cli-ux Issues and PRs that deal with the UX of the CLI (exit codes, log output, verbs/options, and so on) labels Jun 23, 2022
@baronfel baronfel requested a review from dsplaisted June 23, 2022 19:36
src/Tests/dotnet.Tests/ParserTests/ResponseFileTests.cs Outdated Show resolved Hide resolved
src/Cli/dotnet/Parser.cs Outdated Show resolved Hide resolved
Copy link
Member Author

@baronfel baronfel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few notes on the updates from today/consulting with @jonsequitur.

@baronfel baronfel force-pushed the token-replacement branch from 3c14272 to 5da0f6f Compare June 24, 2022 13:53
src/Cli/dotnet/CommonOptions.cs Outdated Show resolved Hide resolved
src/Cli/dotnet/OptionForwardingExtensions.cs Outdated Show resolved Hide resolved
@baronfel
Copy link
Member Author

I could add /property as well, I thought about that last night - ideally we'd accept all forms of the property option that MSbuild accepts, as well as the more posix-standard --property long form and -p short form.

@baronfel
Copy link
Member Author

@dsplaisted if you wanna take another look that'd be cool :) I added full support for the different permutations of property that MSBuild supports (--, -, /), and fleshed out testing for the token replacement for response files as well as the MSBuild property forwarding since the last time you looked.

@BradBarnich
Copy link

will this change fix to allow handling a comma in a quoted string in an RSP file? I didn't see this case covered in the tests

such as:

/p:Copyright="Copyright (C) CareEvolution, Inc. 2022"

another example in this comment: #26026 (comment)

@baronfel
Copy link
Member Author

@BradBarnich thanks for checking! the response file part is parsed correctly, but the value gets mutilated during the forwarding to MSBuild. I will add additional test cases and fixes for this now.

@rbhanda rbhanda modified the milestone: 6.0.304 Jun 28, 2022
@rbhanda rbhanda added this to the 6.0.303 milestone Jun 28, 2022
@baronfel
Copy link
Member Author

Actually this did fix parameters with spaces and quotes, and I was just comparing to the wrong collection in the tests. Updated and we should be good to go!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-CLI Area-Infrastructure cli-ux Issues and PRs that deal with the UX of the CLI (exit codes, log output, verbs/options, and so on) Servicing-approved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants