BuildCheck does not run on restore #10018

maridematte · 2024-04-16T16:42:20Z

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

Added two check for the restore phase on the code.

The first one is on the RequestBuilder, it reads the global property IsRestore and creates a BuildCheck instance as necessary.

The second one is within the BuildCheckConnectorLogger so we do not start a new BuildCheck run because of received events. In this case I added a bool to ProjectEvaluationStartedEventArgs so we can have access to the IsRestore variable within the logger. We skip all event messages until we get a ProjectFinishedEventArgs, which signals the end of a build / the restore phase.

Tests

Added extra tests that only runs the restore target.

Reopened from #9907

eng/BootStrapMSBuild.props

JanKrivanek

There still seem to be some merge leftovers

src/Build/BuildCheck/Infrastructure/BuildCheckConnectorLogger.cs

src/Build/BuildCheck/Logging/BuildAnalysisLoggingContextExtensions.cs

src/Build/BuildCheck/Logging/IBuildAnalysisLoggingContext.cs

src/Build/BackEnd/Components/Logging/ILoggingService.cs

src/Framework/ProjectEvaluationStartedEventArgs.cs

rainersigwald · 2024-04-26T20:37:03Z

src/Build/BackEnd/Components/RequestBuilder/RequestBuilder.cs

-
-            var buildCheckManager = (_componentHost.GetComponent(BuildComponentType.BuildCheckManagerProvider) as IBuildCheckManagerProvider)!.Instance;
-            buildCheckManager.SetDataSource(BuildCheckDataSource.BuildExecution);
+            var propertyEntry = _requestEntry.RequestConfiguration.GlobalProperties[MSBuildConstants.MSBuildIsRestoring];


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 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 the BuildCheckConnectorLogger 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.

rainersigwald · 2024-04-26T22:00:34Z

src/Build/BuildCheck/Infrastructure/BuildCheckConnectorLogger.cs

+    {
+        if (isRestore)
+        {
+            isRestore = false;


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?

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?

If we are relying on the restore happen fully before the build - then the first ProjectEvaluationStartedEventArgs that is not marked with IsRestore denotes the start of the build after the restore.

I changed the isRestore reset to happen when we receive another ProjectEvaluationStartedEventArgs event.

ladipro · 2024-05-07T06:50:55Z

src/Build/BuildCheck/Infrastructure/BuildCheckConnectorLogger.cs

+            {
+                if (!isRestore)
+                {
+                _buildCheckManager.StartProjectRequest(BuildCheckDataSource.EventArgs, e.BuildEventContext!);


super-nit: Indentation

ladipro · 2024-05-07T06:59:44Z

src/Framework/ProjectEvaluationStartedEventArgs.cs

+        /// <summary>
+        /// Gets or sets is the project is currently on restore phase.
+        /// </summary>
+        public bool IsRestore { get; internal set; }


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.

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.

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

internal const int FileFormatVersion = 20;

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

FYI @KirillOsenkov (just a heads up that same small changes will be comming. We should handle them proactively ourselves)

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.

Good idea - I'll find a time to do this

ladipro · 2024-05-07T07:09:02Z

src/Build/Evaluation/Evaluator.cs

@@ -626,7 +626,7 @@ private void Evaluate()
                    }
                }

-                _evaluationLoggingContext.LogProjectEvaluationStarted();
+                _evaluationLoggingContext.LogProjectEvaluationStarted(_data.GlobalPropertiesDictionary[MSBuildConstants.MSBuildIsRestoring] is not null); ;


We currently set the property only to true but it's not unconceivable that at some point someone would want to set it to false to mean "we're not restoring". For future proofing, would you consider parsing the value instead of just checking that the prop is defined?

JanKrivanek · 2024-05-15T15:13:47Z

** UPDATE May/15 **
As decided collectively on a sync:

~~Let's try to see if we can put the GlobalVariables to BuildStartedEventArgs~~ no - BuildStartedEventArgs are sent through logging infra way before we detect the fact that restore is requested
~~Verify this covers tha case of msbuild /t:restore~~ Yes - Do not import project extensions during restore #9748
This will probably cause that case of msbuild /t:restore;build will completely skip analyzers for the build part - this is accepted and fine. Let's just verify the fact and if true let's put it in the BuildCheck doc, plus let's create item for an analyzer that detects and flags this anti-pattern ('/t:restore;') /t:restore,build wont be captured: Do not import project extensions during restore #9748 (comment) and buildcheck item for that already exists here: [Feature Request]: Warns a user if they use Restore as an explicit target (/t:Restore;Build) instead of the /restore flag #9690
The way of skipping the analysis during the resote (boolean flag, vs null proxy) can be discussed later on

maridematte added 7 commits March 21, 2024 15:28

First pass on disabling BuildCheck on restore

5b33d8f

BuildCheck doesn't run on restore

916d822

Addressed PR comments

8363efc

Modified how logger registers isRestore

7d223bc

Merge branch 'main' into 9747

f72913e

Fixed merge errors

08b0cd1

Clean merge v2

288adff

JanKrivanek reviewed Apr 17, 2024

View reviewed changes

eng/BootStrapMSBuild.props Outdated Show resolved Hide resolved

maridematte added 2 commits April 18, 2024 18:47

Updated existing tests

e3ea5ba

Added test

f19e200

JanKrivanek reviewed Apr 19, 2024

View reviewed changes

maridematte added 3 commits April 19, 2024 16:33

Fix failing tests

15c61c8

Clean up merge, one again

b047dd4

changed case on test file

a014fdb

maridematte marked this pull request as ready for review April 23, 2024 09:40

maridematte requested review from JanKrivanek and rainersigwald April 23, 2024 10:05

maridematte added 3 commits April 24, 2024 16:46

Solved merge conflicts

d0c84b0

Merge main

8da2ae4

Reqorked on ConnectorLogger due changes

f0c8c19

maridematte requested review from ladipro and YuliiaKovalova April 25, 2024 15:10

rainersigwald reviewed Apr 26, 2024

View reviewed changes

maridematte added 2 commits April 29, 2024 15:38

Changed isRestore reset to project evaluation started

5626302

Added documentation

39cadf8

JanKrivanek mentioned this pull request Apr 30, 2024

[WIP] Prototype for discussion #10091

Closed

ladipro reviewed May 7, 2024

View reviewed changes

JanKrivanek mentioned this pull request May 16, 2024

Document the binlog update necessary steps #10146

Closed

YuliiaKovalova assigned maridematte May 29, 2024

maridematte closed this Jul 1, 2024

JanKrivanek mentioned this pull request Jul 1, 2024

Analyzers prototyping - Detect and skip restore (evaluation and execution) #9747

Closed

3 tasks

maridematte deleted the 9747 branch August 7, 2024 10:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BuildCheck does not run on restore #10018

BuildCheck does not run on restore #10018

maridematte commented Apr 16, 2024 •

edited

Loading

JanKrivanek left a comment

rainersigwald Apr 26, 2024

maridematte Apr 29, 2024

rainersigwald Apr 26, 2024

maridematte Apr 29, 2024

JanKrivanek Apr 29, 2024

maridematte Apr 29, 2024

ladipro May 7, 2024

ladipro May 7, 2024

ladipro May 13, 2024

JanKrivanek May 13, 2024

JanKrivanek May 13, 2024 •

edited

Loading

KirillOsenkov May 13, 2024

JanKrivanek May 16, 2024

ladipro May 7, 2024

JanKrivanek commented May 15, 2024 •

edited

Loading

BuildCheck does not run on restore #10018

BuildCheck does not run on restore #10018

Conversation

maridematte commented Apr 16, 2024 • edited Loading

Context

Changes Made

Tests

JanKrivanek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JanKrivanek May 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JanKrivanek commented May 15, 2024 • edited Loading

maridematte commented Apr 16, 2024 •

edited

Loading

JanKrivanek May 13, 2024 •

edited

Loading

JanKrivanek commented May 15, 2024 •

edited

Loading