msbuild icon indicating copy to clipboard operation
msbuild copied to clipboard

[BuildCheck] Add OM and infra for tracking task invocations

Open ladipro opened this issue 1 year ago • 2 comments

Contributes to #9881

Context

Some build checks will want to analyze tasks executed during build. This PR is adding support for these by introducing several new types and implementing processing of task-related logger events in BuildEventsProcessor.

Changes Made

  • Added RegisterTaskInvocationAction for analyzers to call to subscribe to task events.
  • Added TaskInvocationAnalysisData as a unit of reporting task events to analyzers.
  • Implemented a transform from TaskStartedEventArgs, TaskFinishedEventArgs, and TaskParameterEventArgs to TaskInvocationAnalysisData.

Testing

New unit tests.

ladipro avatar May 03 '24 10:05 ladipro

Thank you for the quick review.

Btw. - don't you want to extend the TaskParameterEventArgs as part of this PR? If not - would be nice to have addition PR for that - possibly merged prior this one.

The fact that assigning task outputs to properties is currently logged as text messages will need to be addressed in a careful and viewer-friendly way. Let me think about it a bit. I would prefer to do it in a separate PR without blocking this one. It should not be an issue for the double writes analyzer, for example.

ladipro avatar May 03 '24 13:05 ladipro

Thank you for the quick review.

Btw. - don't you want to extend the TaskParameterEventArgs as part of this PR? If not - would be nice to have addition PR for that - possibly merged prior this one.

The fact that assigning task outputs to properties is currently logged as text messages will need to be addressed in a careful and viewer-friendly way. Let me think about it a bit. I would prefer to do it in a separate PR without blocking this one. It should not be an issue for the double writes analyzer, for example.

FYI @KirillOsenkov - just a heads up that another change is comming ;-) But I bet that move from textual to structured info is allways appreciated.

JanKrivanek avatar May 04 '24 10:05 JanKrivanek

Do we have a sample analyzer that would exercise this? It's always better to design APIs driven by a real-world use case. Would love to see how this API can be used at the consumer side.

KirillOsenkov avatar May 05 '24 22:05 KirillOsenkov

True that we don't currently have an analyzer that would need this. It is motivated by completeness - when something is interested in task parameters, it feels natural to provide both inputs and outputs.

The current model follows the MSBuild syntax: The viewer renders Parameters (input parameters), OutputProperties (output parameters that are assigned to props), and OutputItems (output parameters that are assigned to items). As an analyzer author I think I would expect the model to be task centric, i.e. I wouldn't so much be interested in where the output is going, but I would want to know the name of the output parameter so I can implement rules like "output parameter Foo of task Bar always has this particular shape". And again, the major issue is that the name "Foo" is currently not included anywhere. If a task assigns multiple output parameters to the same item, for example, the binlog is ambiguous.

ladipro avatar May 06 '24 07:05 ladipro

Here's what I'm planning to do to address the missing output parameter name issue: https://github.com/ladipro/msbuild/pull/1/files I will continue after this PR is merged.

ladipro avatar May 10 '24 15:05 ladipro