Skip to content

Add Buildcheck events support + BuildSubmissionStarted#797

Merged
KirillOsenkov merged 11 commits into
KirillOsenkov:mainfrom
YuliiaKovalova:dev/ykovalova/add_buildcheck_events_support
Aug 28, 2024
Merged

Add Buildcheck events support + BuildSubmissionStarted#797
KirillOsenkov merged 11 commits into
KirillOsenkov:mainfrom
YuliiaKovalova:dev/ykovalova/add_buildcheck_events_support

Conversation

@YuliiaKovalova
Copy link
Copy Markdown
Contributor

@YuliiaKovalova YuliiaKovalova commented Aug 13, 2024

Fixes #796

Context

Add BuildCheck events (dotnet/msbuild@ac76b77#diff-d88dc5683a31c5508ad8efa3aeecdfb56949fe6c0d234ff233afee954f93deb6) support and BuildSubmissionStarted (dotnet/msbuild#10424)

@KirillOsenkov
Copy link
Copy Markdown
Owner

Ping me when this is ready for review!

@YuliiaKovalova YuliiaKovalova marked this pull request as ready for review August 23, 2024 16:02
@YuliiaKovalova
Copy link
Copy Markdown
Contributor Author

@KirillOsenkov , it's ready for review, please take a look!

Comment thread src/StructuredLogger/BinaryLogger/BuildEventArgsReader.cs
Comment thread src/StructuredLogger/BinaryLogger/BuildSubmissionStartedEvent.cs Outdated
@KirillOsenkov
Copy link
Copy Markdown
Owner

We should also update our copy of the Writer here:
https://github.com/KirillOsenkov/MSBuildStructuredLog/blob/main/src/StructuredLogger/BinaryLogger/BuildEventArgsWriter.cs#L158

It's rarely used (and maybe we should delete it?) but that's a separate conversation I think

@JanKrivanek if you could review this PR too when you get a chance.

Also didn't we want to write down some instructions on updating the viewer copy of the reader/writer? I can't remember if we already have these somewhere or whether we still need to do it.

Appreciate the help @YuliiaKovalova!

@JanKrivanek
Copy link
Copy Markdown
Collaborator

Writer is needed - it's used for redacting functionality.

Documenting the viewer updating steps is as well still needed - I still didn't get a chance to fix it dotnet/msbuild#10146

I'll surely and gladly review this - great thanks @YuliiaKovalova for adding all the missing types!!

Copy link
Copy Markdown
Collaborator

@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.

Looks good!

var fields = ReadBuildEventArgsFields(readImportance: true);
var rawTracingData = ReadStringDictionary() ?? new Dictionary<string, string>();

var e = new BuildCheckTracingEventArgs(rawTracingData.ToDictionary(
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is this class already known to viewer?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

image

@KirillOsenkov
Copy link
Copy Markdown
Owner

Just needs the writer changes.

@KirillOsenkov
Copy link
Copy Markdown
Owner

Also do we have a unit-test like BinaryLoggerRoundtrip that reads and writes the same file? I think we do have something... Would be nice to make sure we didn't "forget" the writer changes somehow.

@YuliiaKovalova
Copy link
Copy Markdown
Contributor Author

BuildEventArgsWriter.cs#L158

We have this class:https://github.com/dotnet/msbuild/blob/5c934f81bcdf63168050f7083c4fd5b09faab9cc/src/Build.UnitTests/BuildEventArgsSerialization_Tests.cs but I see some missed recent events.
I would suggest to add a note to https://github.com/dotnet/msbuild/blob/a03ee4ba9c5e71a361d86a6ef7923c44a0dc5f39/src/Build/Logging/BinaryLogger/BinaryLogger.cs#L81 to keep BuildEventArgsWriter(s) in msbuild and in Viewer consistent.

Copy link
Copy Markdown
Owner

@KirillOsenkov KirillOsenkov left a comment

Choose a reason for hiding this comment

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

👍🏻

@KirillOsenkov KirillOsenkov merged commit e670189 into KirillOsenkov:main Aug 28, 2024
@KirillOsenkov
Copy link
Copy Markdown
Owner

Thanks!

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.

Add suport for the new binlog version (BuildSubmissionStartedEventArgs added)

3 participants