Skip to content

Update csc.json#97

Closed
laurenprinn wants to merge 6 commits intoactions:mainfrom
laurenprinn:laurenprinn-regularExpressionChanges
Closed

Update csc.json#97
laurenprinn wants to merge 6 commits intoactions:mainfrom
laurenprinn:laurenprinn-regularExpressionChanges

Conversation

@laurenprinn
Copy link
Copy Markdown

@laurenprinn laurenprinn commented Jun 9, 2020

I'm proposing a change in MSBuild's console output format (dotnet/msbuild#5414) that
will add additional information to errors/warnings from projects that have multiple
TargetFrameworks. This updates the problem matcher to handle the new format.
S:\msbuild\src\Build\Evaluation\ExpressionShredder.cs(33,7): error CS1003: Syntax error, ',' expected [S:\msbuild\src\Build\Microsoft.Build.csproj TargetFramework:netcore3.0]

Console logger output was changed, so updated the regular expression to match the new output.
Fixed escape characters
Comment thread .github/csc.json Outdated
"pattern": [
{
"regexp": "^([^\\s].*)\\((\\d+)(?:,\\d+|,\\d+,\\d+)?\\):\\s+(error|warning)\\s+([a-zA-Z]+(?<!MSB)\\d+):\\s*(.*?)\\s+\\[(.*?)\\]$",
"regexp": "^([^\\s].*)\\((\\d+)(?:,\\d+|,\\d+,\\d+)?\\):\\s+(error|warning)\\s+([a-zA-Z]+(?<!MSB)\\d+):\\s*(.*?)\\s+\\[([^\\s]+)[\\s\\W\\dA-z].*\\]$",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm not sure we can assume that the path to the project file doesn't contain whitepace--it's generally legal for it to do so. Should we separate based on a different character, or can this be rewritten to work backwards from the end to a space instead?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

A file path can't contain < so I could use that character before the properties are printed out to separate the file path and the properties

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I changed it so the output should follow below. I think this actually makes it a bit more clearer as well as to the separation between the properties and the file path S:\msbuild\src\Build\Evaluation\ExpressionShredder.cs(33,7): error CS1003: Syntax error, ',' expected [S:\msbuild\src\Build\Microsoft.Build.csproj > Properties:prop]

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The new pattern leaves the space between the file name and the greater-than-sign which might mess with the problem matching

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Right. I'll take that space out of the new pattern

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I hope no one ever does this, but < and almost any other character can be used in filenames on linux.

We changed the console logger's output to separate the outputted properties and the file path with a ">" symbol.
@chrispat
Copy link
Copy Markdown
Contributor

/cc @ericsciple

@ZEisinger
Copy link
Copy Markdown

ZEisinger commented Jun 12, 2020

I updated the script causing the test failure and added a smoke test for the pattern matching. Can you merge the latest from master into your branch to retest?

@ZEisinger ZEisinger changed the base branch from master to main July 15, 2020 17:44
Copy link
Copy Markdown

@ZEisinger ZEisinger left a comment

Choose a reason for hiding this comment

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

Updated again after the powershell script fell behind again. Looks good now. Please merge when ready.

@rainersigwald
Copy link
Copy Markdown
Contributor

Hey folks, MSBuild/SDK maintainer here. We didn't get this into the .NET 5.0 SDK and aren't quite sure when it will go in. I think it's best to close this PR for now and we'll pick it up again at some point in the future. Sorry for the delayed decision!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants