Add support for improved TaskParameterEventArgs #780
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ladipro
commented
May 15, 2024
| @@ -165,7 +165,7 @@ public static Build ReadBuild(Stream stream, Progress progress, byte[] projectIm | |||
|
|
|||
| if (errorByType.Any(i => i != 0)) | |||
| { | |||
| string summary = string.Join(", ", errorByType.Where((count, index) => count > 0).Select((count, index) => $"{((ReaderErrorType)index)}: {count}")); | |||
| string summary = string.Join(", ", errorByType.Where((count, index) => count > 0).Select((count, index) => $"{((ReaderErrorType)index)}: {count} cases")); | |||
There was a problem hiding this comment.
I'm finding the existing message confusing. For example, Skipped some data unknown to this version of Viewer. 7 cases encountered (UnknownEventType: 7). to me suggests that 7 is the event type. Making it clearer that the number is the count.
There was a problem hiding this comment.
Indeed very confusing. Thanks!
Comment on lines
-74
to
-85
| private static Action<BuildMessageEventArgs, int> lineNumberSetter = GetFieldSetter<BuildMessageEventArgs, int>("lineNumber"); | ||
| public static void SetLineNumber(BuildMessageEventArgs args, int lineNumber) | ||
| { | ||
| lineNumberSetter(args, lineNumber); | ||
| } | ||
|
|
||
| private static Action<BuildMessageEventArgs, int> columnNumberSetter = GetFieldSetter<BuildMessageEventArgs, int>("columnNumber"); | ||
| public static void SetColumnNumber(BuildMessageEventArgs args, int columnNumber) | ||
| { | ||
| columnNumberSetter(args, columnNumber); | ||
| } | ||
|
|
There was a problem hiding this comment.
I don't think we need this anymore as TaskParameterEventArgs2 contains new props representing line and column number.
ladipro
force-pushed
the
support-task-parameters
branch
from
8a5a4b3
to
92890ba
Compare
JanKrivanek
approved these changes
May 16, 2024
| @@ -165,7 +165,7 @@ public static Build ReadBuild(Stream stream, Progress progress, byte[] projectIm | |||
|
|
|||
| if (errorByType.Any(i => i != 0)) | |||
| { | |||
| string summary = string.Join(", ", errorByType.Where((count, index) => count > 0).Select((count, index) => $"{((ReaderErrorType)index)}: {count}")); | |||
| string summary = string.Join(", ", errorByType.Where((count, index) => count > 0).Select((count, index) => $"{((ReaderErrorType)index)}: {count} cases")); | |||
There was a problem hiding this comment.
Indeed very confusing. Thanks!
Comment on lines
-32
to
+35
| if (line != 0) | ||
| { | ||
| Reflector.SetLineNumber(args, line); | ||
| } | ||
|
|
||
| if (column != 0) | ||
| { | ||
| Reflector.SetColumnNumber(args, column); | ||
| } | ||
|
|
||
| // Should probably make these public | ||
| // args.LineNumber = line; | ||
| // args.ColumnNumber = column; | ||
| args.LineNumber = line; | ||
| args.ColumnNumber = column; |
|
Published https://github.com/KirillOsenkov/MSBuildStructuredLog/releases/tag/v2.2.243 - please try it out to make sure it all works as expected. Thanks! |
|
@KirillOsenkov I have tested it on version 2.2.248 and all seems to be good. Please confirm that it's OK to merge the MSBuild PR now. |
|
yes, go ahead |
ladipro
added a commit
to dotnet/msbuild
that referenced
this pull request
May 23, 2024
Contributes to #9881 ### Context When `TaskParameterEventArgs` represents an output parameter of a task, it currently does not capture the name of the parameter. This information is generally useful as multiple output parameters can be assigned to the same item, and it's important for the BuildCheck infrastructure to provide a consistent object model to build analyzers. Additionally, output parameters that are assigned to properties are currently logged as textual log messages `Output Property: {0}={1}` which miss the name of the parameter as well. ### Changes Made Added two new properties on the `TaskParameterEventArgs` class, including the requisite serialization support. Updated logging to populate the properties with the name of the task parameter and the name of the build property, respectively, and updated BuildCheck infra to expose it to analyzers subscribed to task parameter events. All task parameters are now logged as the structured `TaskParameterEventArgs` and the kind of parameter (input, output to property, output to item) can be determined by examining the args. ### Testing - Tweaked existing unit test and updated the relevant BuildCheck test. - Verified that when using the standard console logger, the resulting textual log is identical. - Verified that binlog viewer without the corresponding changes displays the forward-compat warning about unknown data. ### Notes - Complementary binlog viewer PR: KirillOsenkov/MSBuildStructuredLog#780 - The change is behind a changewave because it is technically a breaking change for third party loggers. Co-authored-by: Rainer Sigwald <raines@microsoft.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
dotnet/msbuild#10130 is making MSBuild log
TaskParameterEventArgsfor all task parameters, including all output parameters. This PR adds support for this format change to the viewer.Changes
TaskParameterEventArgsused for all parameters.Testing
Old binlog with new viewer - no changes.
New binlog with old viewer - shows the
Skipped some data unknown to this version ...warning and misplaces output parameters assigned to properties. They are incorrectly displayed underOutputItemswithN/Aname.This is assumed to be an acceptable break for users who haven't updated to the new version.
This is just a quick proof-of-concept as I'm not sure what the best way of presenting this data is or if there's appetite for enriching the UI at all. We could certainly not show the parameter name anywhere for now.