Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 does not run on restore #10018
BuildCheck does not run on restore #10018
Changes from 15 commits
5b33d8f
916d822
8363efc
7d223bc
f72913e
08b0cd1
288adff
e3ea5ba
f19e200
15c61c8
b047dd4
a014fdb
d0c84b0
8da2ae4
f0c8c19
5626302
39cadf8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think I understand, since we have this, why we need the change to the evaluation-started event. Can you go over that again?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are two points in BuildCheck that actually run the analyzer. One of them is through the
RequestBuilder
which works on the worker nodes. And through theBuildCheckConnectorLogger
which runs on the main node. The modification to the evaluation-started event is to get information about the restore phase to the connector logger.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't make sense to me, won't there be a bunch of ProjectFinished events that will fire one after the other, causing the first one to unset this and the others to do check stuff?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I figured that out after I ran this for a bit. I had hoped the event was just fired once. Is there an event that is fired at the end of the restore phase that I can use to to control the flow on the connector logger?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are relying on the restore happen fully before the build - then the first
ProjectEvaluationStartedEventArgs
that is not marked withIsRestore
denotes the start of the build after the restore.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed the
isRestore
reset to happen when we receive anotherProjectEvaluationStartedEventArgs
event.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
super-nit: Indentation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We currently set the property only to
true
but it's not unconceivable that at some point someone would want to set it tofalse
to mean "we're not restoring". For future proofing, would you consider parsing the value instead of just checking that the prop is defined?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's worth considering keeping the property internal for now. It's needed only internally for the infra. For a public API I wonder if exposing all global build properties wouldn't be potentially more useful. But since it's generally hard to remove or update an API once shipped, I think I would vote for not exposing anything publicly for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually - assuming that we also want to use this flag when replaying a binlog, we need to make a public surface change anyway. In other words, the new flag should likely be serialized. cc @JanKrivanek to confirm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's correct.
BuildEventArgsWritter (and BuildEventArgsReader) need to be adjusted accordingly, plus the BinLogger version needs to be incremented
msbuild/src/Build/Logging/BinaryLogger/BinaryLogger.cs
Line 81 in a8e224f
Then we'll need to copy the identical change to the https://github.com/KirillOsenkov/MSBuildStructuredLog/blob/main/src/StructuredLogger/BinaryLogger/BuildEventArgsReader.cs (and preferrably BuildEventArgsWritter as well) - so that viewer is fine with the change (it'd grace handle skip the extra field, but would report it as unknown change).
The analogous change seem to have been forgotten for tracing: #10016 (comment), so alternatively we can have bothe of those changes be reflected in a single PR with single BinaryLogger version bump.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI @KirillOsenkov (just a heads up that same small changes will be comming. We should handle them proactively ourselves)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps @JanKrivanek we should have a wiki page somewhere with a walkthrough of how to do BinaryLogger related changes? Including the viewer PR, version increments etc. Feel free to add to the wiki on the viewer repo or here, wherever it makes more sense for the team.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea - I'll find a time to do this