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 Replay Mode #10224

Merged
merged 44 commits into from
Jun 26, 2024

Conversation

surayya-MS
Copy link
Member

@surayya-MS surayya-MS commented Jun 11, 2024

Fixes #9760

Refactoring of BuildCheck infrastructure

Currently BuildCheck uses LoggingService and LoggingContext produced by the AnalyzerLoggingContextFactory.

  1. Removed usage of LoggingService instance and instead used LoggingContext to log events. This was needed because BuildCheckManager could use the LoggingService instance from the very first build that is no longer available afterwards. The commits:
    31af242
    e47a8ff

  2. Now I needed to decouple from AnalyzerLoggingContext : LoggingContext. The only usage of AnalyzerLoggingContext are these methods: LogComment, LogCommentFromText, LogErrorFromText and LogBuildEvent. So, I created IAnalysisContext with similar methods, and the names start with Dispatch:

namespace Microsoft.Build.Experimental.BuildCheck;

internal interface IAnalysisContext
{
    BuildEventContext BuildEventContext { get; }

    void DispatchAsComment(MessageImportance importance, string messageResourceName, params object?[] messageArgs);

    void DispatchBuildEvent(BuildEventArgs buildEvent);

    void DispatchAsErrorFromText(string? subcategoryResourceName, string? errorCode, string? helpKeyword, BuildEventFileInfo file, string message);

    void DispatchAsCommentFromText(MessageImportance importance, string message);
}

Then created AnalysisLoggingContext : IAnalysis that uses LoggingService and AnalysisLoggingContextFactory similar to the existing AnalyzerLoggingContextFactory.

Implementing BuildCheck Replay Mode

  1. Now, I need to be able to produce new BuildEventArgs and invoke it for the loggers to pick it up. Similar to what LoggingService does. For that reason, implemented AnalysisDispatchingContext : IAnalysisContext and the factory - AnalysisDispatchingContextFactory.
    Because the code for the creation of the events was duplicated, I put it all in a helper class EventsCreatorHelper.

  2. Seperated the logic of handling the events from BuildCheckConnectorLogger and put it into BuildCheckBuildEventHandler to reuse it later.

  3. The last part. When replaying binary log with BuildCheck enabled, I create a new IEventSource mergedEventSource. The logic is extracted into a new static class BuildCheckReplayModeConnector

  • The events from the replayEventSource are passed to the mergedEventSource unchanged.
  • Attached BuildCheckBuildEventHandler.HandleBuildEvent to the replayEventSource.AnyEventRaised event, in order to produce new events from BuildCheck.
  • The BuildCheckBuildEventHandler instance uses mergedEventSource to invoke new events
  • All the loggers initialize with this event source, not with BinaryLogReplayEventSource replaySource

diagram drawio (2)

Notes

…o reuse it for binlog replay

2. removed inheritance of EventArgsDispatcher from BinaryLogReplayEventSource, instead use it as a field
3. created BuildCheckEventArgsDispatcher
… AnalysisDisatcherContext;

remove commented code
@surayya-MS
Copy link
Member Author

Looks like there is a compatibility problem

.dotnet\sdk\8.0.201\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.ApiCompat.ValidatePackage.targets(39,5): error CP0007: (NETCORE_ENGINEERING_TELEMETRY=Build) Type 'Microsoft.Build.Logging.BinaryLogReplayEventSource' does not inherit from base type 'Microsoft.Build.Logging.EventArgsDispatcher' on ref/net472/Microsoft.Build.dll but it does on [Baseline] ref/net472/Microsoft.Build.dll

I will change the implementation slightly to make it compatible

@surayya-MS surayya-MS marked this pull request as ready for review June 14, 2024 14:36
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.

Overall looks good!

It feels that it deserves a small diagram or a design description

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 @surayya-MS - to me it appears to be a solid implementation!!

I'd love to see the description of the replay mechanics, together with the diagram as part of the https://github.com/dotnet/msbuild/blob/main/documentation/specs/proposed/BuildCheck-Architecture.md

The new interfaces and classes would probably benefit from brief xml comments (doesn't have to be on each method - but at least for the whole type)

@rainersigwald
Copy link
Member

One diagram I'd like (not blocking for this PR but I think it'd help) is something like this:

graph LR
Real[Live build] -->|EventArgs| mux[???] -->|BuildCheck API| check[BuildCheck<br/><i>#40;doesn't care about<br/>live vs replay#41;</i>]
log[Replayed log] -->|EventArgs| mux

check -->|Check results| out[???]
Loading

with those question-mark blocks filled in.

@surayya-MS
Copy link
Member Author

surayya-MS commented Jun 21, 2024

Events come from BinaryLogReplayEventSource and passed to

  • the IEvent mergedEventSource unchanged
  • the BuildCheckBuildEventHandler, where new events are produced from the BuildCheck

The new events from BuildCheck are passed to the mergedEventSource
Loggers getting events from the mergedEventSource

@surayya-MS
Copy link
Member Author

surayya-MS commented Jun 21, 2024

BuildCheckDiagram

@surayya-MS surayya-MS enabled auto-merge (squash) June 26, 2024 07:47
@surayya-MS surayya-MS disabled auto-merge June 26, 2024 08:08
@surayya-MS surayya-MS enabled auto-merge (squash) June 26, 2024 08:08
@surayya-MS surayya-MS merged commit fb4bc63 into dotnet:main Jun 26, 2024
10 checks passed
@surayya-MS surayya-MS deleted the buildCheck-replay-mode-dispatcher branch June 26, 2024 10:57
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.

BuildCheck connector for binlog replaying
3 participants