BuildCheck does not run on restore#9907
BuildCheck does not run on restore#9907maridematte wants to merge 4 commits intodotnet:exp/build-analyzersfrom
Conversation
JanKrivanek
left a comment
There was a problem hiding this comment.
It looks like it accomplishes axactly what we need - which is great!
It would be good to verify it within the test(s) (no duplicated checks when we run -restore).
In the worker node we have the restore state known to the BuildCheckManager (via the IsRestore property) as well as to the caller (via the isRestore variable) one of which is not used - so we should remove the unused code.
Other then those things it looks good and once addressed - ready for signoff
| private void EventSource_AnyEventRaised(object sender, BuildEventArgs e) | ||
| { | ||
| if (e is ProjectEvaluationFinishedEventArgs projectEvaluationFinishedEventArgs) | ||
| if (!buildCheckManager.isRestore) |
There was a problem hiding this comment.
I'd suggest to invert the condition and return - it'll reduce nesting and as well simplify the diff.
| if (!buildCheckManager.isRestore) | |
| if (buildCheckManager.isRestore) | |
| { | |
| return; | |
| } |
| /// </summary> | ||
| internal interface IBuildCheckManager | ||
| { | ||
| bool isRestore { get; set; } |
There was a problem hiding this comment.
This doesn't seem to be used - it can be safely removed (along with implementations).
There was a problem hiding this comment.
This check is used within BuildCheckConnectorLogger.
The check for the restore needs to be in two points of the code, on the worker node (that is the RequestBuilder class and checks), and within the main node (within the BuildCheckConnectorLogger).
If we do not have the check within the ConnectorLogger, the main node will still execute an analysis during restore, as it receives a message that a project evaluation is starting from normal MSBuild process, and that triggers a BuildCheck run.
There was a problem hiding this comment.
Oh I see.
In such case - the logger and the worker node can - and should - maintain their own private indication of whether restore is happening (no need to store it in the manager - as it anyways is not usable globally). The logger should maintain it per project.
- The worker node is not guaranteed to be part of the entrypoint node (that runs the loggers). Currently only
RequestBuilderis setting the indication - that would not work for the case (you can experiment with this via theEndToEndTestthat setsNOINPROCNODEin one of the cases to1- then the logger runs alone) - Different projects can run their different phases during the same build - logger cannot determine the restore status globally - only per project. And it will need to get the indication from BuildEventArgs - as there might not be anything else available in the same process.
| if (!isRestore) | ||
| { | ||
| buildCheckManager = (_componentHost.GetComponent(BuildComponentType.BuildCheck) as IBuildCheckManagerProvider)!.Instance; | ||
| buildCheckManager.isRestore = false; | ||
| buildCheckManager.SetDataSource(BuildCheckDataSource.BuildExecution); | ||
| } |
There was a problem hiding this comment.
| if (!isRestore) | |
| { | |
| buildCheckManager = (_componentHost.GetComponent(BuildComponentType.BuildCheck) as IBuildCheckManagerProvider)!.Instance; | |
| buildCheckManager.isRestore = false; | |
| buildCheckManager.SetDataSource(BuildCheckDataSource.BuildExecution); | |
| } | |
| IBuildCheckManager buildCheckManager = isRestore ? null : (_componentHost.GetComponent(BuildComponentType.BuildCheck) as IBuildCheckManagerProvider)!.Instance; | |
| buildCheckManager?.SetDataSource(BuildCheckDataSource.BuildExecution); |
Similarly all the conditions below can be simplified with null check operator.
Context
We currently run BuildCheck during the restore phase of the build, because of this we end up running BuildCheck twice per project. This PR disables BuildCheck during restore phase.
Changes Made
BuildCheck manager has a new property
isRestorewhich indicates if a build is in restore phase. It is by default true, and during build it is set tofalseso BuildCheck can run.At the start of the build on proc nodes, the global variable
MSBuildIsRestoringis checked to see if the build is currently running the restore phase. If it is a restore, theBuildCheckManagerobject is initialized tonull, otherwise initialized as normal. This relies on the assumption that there is only one restore per project.On the main process, the process start relies on events that the
BuildCheckConnectorLoggercaptures. So it is not started on a restore phase, theisRestoreis set to true, until an actual build starts and the variable is set to false.