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

Add Build Submission Started event #10424

Merged
merged 23 commits into from
Aug 7, 2024
Merged

Add Build Submission Started event #10424

merged 23 commits into from
Aug 7, 2024

Conversation

maridematte
Copy link
Contributor

Fixes #10145

Context

It is currently not possible to check for global variables during build execution. To have access to that data it needs to be passed on events to the processing nodes. None of the events we had previously were good matches to pass along this information, so it was decided to create one for build submission started, as this point in the build we have all properties loaded with the correct value, but its not too late to make use of them.

Changes Made

  • Added a new BuildSubmissionStartedEventArgs based on BuildStatusEventArgs and added it to the event handlers.
  • A copy of the enum BuildRequestDataFlags was added to Microsoft.Build.Framework.

Testing

To be added.

Notes

Blocker for #9747

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 as an initial draft. It needs couple finalization steps:

  • de/serialization of the event for node to node communication.
  • de/serialization of the event for BinaryLogger (plus version increase)
  • unit tests - for the de/serialization
  • fixing the build failures

src/Build/BackEnd/BuildManager/BuildManager.cs Outdated Show resolved Hide resolved
src/Build/BackEnd/BuildManager/BuildManager.cs Outdated Show resolved Hide resolved
src/Framework/BuildSubmissionStartedEventArgs.cs Outdated Show resolved Hide resolved
src/Framework/BuildSubmissionStartedEventArgs.cs Outdated Show resolved Hide resolved
src/Framework/BuildSubmissionStartedEventArgs.cs Outdated Show resolved Hide resolved
src/Framework/BuildSubmissionStartedEventArgs.cs Outdated Show resolved Hide resolved
src/Build/BackEnd/BuildManager/BuildManager.cs Outdated Show resolved Hide resolved
src/Framework/BuildSubmissionStartedEventArgs.cs Outdated Show resolved Hide resolved
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.

Looks good.

There are couple adjustments needed (versioning, nullable strings ...) - captured in comments

src/Build.UnitTests/BackEnd/BuildManager_Tests.cs Outdated Show resolved Hide resolved
src/Build/Logging/BinaryLogger/BuildEventArgsReader.cs Outdated Show resolved Hide resolved
src/Build/Logging/BinaryLogger/BinaryLogRecordKind.cs Outdated Show resolved Hide resolved
src/Framework/BuildSubmissionStartedEventArgs.cs Outdated Show resolved Hide resolved
src/Framework/BuildSubmissionStartedEventArgs.cs Outdated Show resolved Hide resolved
@maridematte maridematte requested a review from JanKrivanek August 7, 2024 06:46
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.

Looks good!

Please add the created issue into the sprint - so that it gets handled soon

Copy link
Member

@AR-May AR-May left a comment

Choose a reason for hiding this comment

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

LGTM

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 BuildSubmissionStartedEventArgs event
4 participants