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

[BuildCheck] Add OM and infra for tracking task invocations #10100

Merged
merged 9 commits into from
May 13, 2024

Conversation

ladipro
Copy link
Member

@ladipro ladipro commented May 3, 2024

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 ladipro requested a review from JanKrivanek May 3, 2024 10:53
Copy link
Member

@JanKrivanek JanKrivanek left a comment

Choose a reason for hiding this comment

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

This looks good - thank you!!

The only objection I have is about the expressing of the location info.

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.

src/Build/BuildCheck/OM/ParsedItemsAnalysisData.cs Outdated Show resolved Hide resolved
src/Build/BuildCheck/OM/ParsedItemsAnalysisData.cs Outdated Show resolved Hide resolved
src/MSBuild/XMake.cs Show resolved Hide resolved
@ladipro
Copy link
Member Author

ladipro commented May 3, 2024

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.

@JanKrivanek
Copy link
Member

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.

@KirillOsenkov
Copy link
Member

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.

@ladipro
Copy link
Member Author

ladipro commented May 6, 2024

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 ladipro self-assigned this May 6, 2024
@ladipro
Copy link
Member Author

ladipro commented May 10, 2024

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.

Copy link
Member

@JanKrivanek JanKrivanek left a comment

Choose a reason for hiding this comment

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

Thank you!!
:shipit:

@ladipro ladipro enabled auto-merge (squash) May 13, 2024 13:35
@ladipro ladipro merged commit 77f8221 into dotnet:main May 13, 2024
10 checks passed
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.

4 participants