Simplify exception handling for graph builds by dfederm · Pull Request #7831 · dotnet/msbuild

dfederm · 2022-07-15T19:24:37Z

Just some cleanup and simplification in the exception handling for graph builds.

Functionally here are the differences:

ParallelWorkSet now throws an aggregate exception containing all exceptions instead of just the first exception that gets thrown.
ParallelWorkSet tasks no longer go away when they throw. They're caught and added to a list used in the point above.
BuildManager now properly logs invalid project errors for each invalid project (due to the help of the points above). Previously it was attempting to catch the agg exception which was never thrown and also the way it was in the catch effectively made it unreachable in the first place.
GraphBuildResult now exposes the CircularDependencyException via the Exception field instead of Exception being null while CircularDependency was true.

Forgind · 2022-07-15T19:55:54Z

src/Build/BackEnd/BuildManager/BuildManager.cs

+
+            if (ex is CircularDependencyException)
+            {
+                LogInvalidProjectFileError(new InvalidProjectFileException(ex.Message, ex));


Two questions:
Does this get the right line number, column number, etc.? Maybe have to use one of the constructors with more parameters?
Doesn't this mean we aren't retaining if this has already been logged?

After the latest iteration, it looks like this:

MSBuild version 17.4.0-dev-22365-01+1223c9a6b for .NET Framework Build started 7/15/2022 1:28:30 PM. MSBUILD : error : MSB4251: There is a circular dependency involving the following projects: MSBUILD : error : C:\Users\David\Code\tmp\a.proj -> MSBUILD : error : C:\Users\David\Code\tmp\b.proj -> MSBUILD : error : C:\Users\David\Code\tmp\a.proj Build FAILED. MSBUILD : error : MSB4251: There is a circular dependency involving the following projects: MSBUILD : error : C:\Users\David\Code\tmp\a.proj -> MSBUILD : error : C:\Users\David\Code\tmp\b.proj -> MSBUILD : error : C:\Users\David\Code\tmp\a.proj 0 Warning(s) 1 Error(s) Time Elapsed 00:00:00.25

I don't think there are line numbers to give really since this isn't like non-graph builds where the circular dependency comes from a specific <MSBuild> task call. I guess in a graph build it comes from a <ProjectReference>, but we don't really keep the xml elements around to figure out where those are.

Compare with today's pretty awful behavior:

Microsoft (R) Build Engine version 17.2.1+52cd2da31 for .NET Framework Copyright (C) Microsoft Corporation. All rights reserved. Build started 7/15/2022 1:43:31 PM. Build FAILED. 0 Warning(s) 0 Error(s) Time Elapsed 00:00:00.03 MSBUILD : error MSB1025: An internal failure occurred while running MSBuild. Microsoft.Build.Exceptions.CircularDependencyException: MSB4251: There is a circular dependency involving the following projects: C:\Users\David\Code\tmp\a.proj -> C:\Users\David\Code\tmp\b.proj -> C:\Users\David\Code\tmp\a.proj at Microsoft.Build.Graph.GraphBuilder.<DetectCycles>g__VisitNode|32_0(ProjectGraphNode node, IDictionary`2 nodeState) at Microsoft.Build.Graph.GraphBuilder.DetectCycles(IReadOnlyCollection`1 entryPointNodes, ProjectInterpretation projectInterpretation, Dictionary`2 allParsedProjects) at Microsoft.Build.Graph.GraphBuilder.BuildGraph() at Microsoft.Build.Graph.ProjectGraph..ctor(IEnumerable`1 entryPoints, ProjectCollection projectCollection, ProjectInstanceFactoryFunc projectInstanceFactory, Int32 degreeOfParallelism, CancellationToken cancellationToken) at Microsoft.Build.Execution.BuildManager.ExecuteGraphBuildScheduler(GraphBuildSubmission submission) at Microsoft.Build.Execution.BuildManager.<>c__DisplayClass85_0.<ExecuteSubmission>b__0() MSBUILD : error MSB1025: An internal failure occurred while running MSBuild. Microsoft.Build.Exceptions.CircularDependencyException: MSB4251: There is a circular dependency involving the following projects: C:\Users\David\Code\tmp\a.proj -> C:\Users\David\Code\tmp\b.proj -> C:\Users\David\Code\tmp\a.proj at Microsoft.Build.CommandLine.MSBuildApp.BuildProject(String projectFile, String[] targets, String toolsVersion, Dictionary`2 globalProperties, Dictionary`2 restoreProperties, ILogger[] loggers, LoggerVerbosity verbosity, DistributedLoggerRecord[] distributedLoggerRecords, Boolean needToValidateProject, String schemaFile, Int32 cpuCount, Boolean enableNodeReuse, TextWriter preprocessWriter, TextWriter targetsWriter, Boolean detailedSummary, ISet`1 warningsAsErrors, ISet`1 warningsNotAsErrors, ISet`1 warningsAsMessages, Boolean enableRestore, ProfilerLogger profilerLogger, Boolean enableProfiler, Boolean interactive, Boolean isolateProjects, GraphBuildOptions graphBuildOptions, Boolean lowPriority, String[] inputResultsCaches, String outputResultsCache, String commandLine) at Microsoft.Build.CommandLine.MSBuildApp.Execute(String commandLine) Unhandled Exception: Microsoft.Build.Exceptions.CircularDependencyException: MSB4251: There is a circular dependency involving the following projects: C:\Users\David\Code\tmp\a.proj -> C:\Users\David\Code\tmp\b.proj -> C:\Users\David\Code\tmp\a.proj at Microsoft.Build.CommandLine.MSBuildApp.BuildProject(String projectFile, String[] targets, String toolsVersion, Dictionary`2 globalProperties, Dictionary`2 restoreProperties, ILogger[] loggers, LoggerVerbosity verbosity, DistributedLoggerRecord[] distributedLoggerRecords, Boolean needToValidateProject, String schemaFile, Int32 cpuCount, Boolean enableNodeReuse, TextWriter preprocessWriter, TextWriter targetsWriter, Boolean detailedSummary, ISet`1 warningsAsErrors, ISet`1 warningsNotAsErrors, ISet`1 warningsAsMessages, Boolean enableRestore, ProfilerLogger profilerLogger, Boolean enableProfiler, Boolean interactive, Boolean isolateProjects, GraphBuildOptions graphBuildOptions, Boolean lowPriority, String[] inputResultsCaches, String outputResultsCache, String commandLine) at Microsoft.Build.CommandLine.MSBuildApp.Execute(String commandLine) at Microsoft.Build.CommandLine.MSBuildApp.Main()

Forgind · 2022-07-15T20:05:16Z

src/Build.UnitTests/Graph/ParallelWorkSet_Tests.cs

                    }
                },
-                ShouldExpectException = true
+                NumExpectedExceptions = 3


Forgind · 2022-07-15T20:06:24Z

src/Build.UnitTests/BackEnd/BuildManager_Tests.cs

+
+            AggregateException aggException = result.Exception.ShouldBeOfType<AggregateException>();
+            aggException.InnerExceptions.Count.ShouldBe(2);
+            aggException.InnerExceptions[0].ShouldBeOfType<InvalidProjectFileException>().ProjectFile.ShouldBeOneOf(project2, project3);


Can you assert the ordering here? The previous version seemed to assert project2 was always first.

Oh, because there was no project3. Never mind.

Forgind · 2022-07-15T20:30:52Z

src/MSBuild/XMake.cs

                        // InvalidProjectFileExceptions and its aggregates have already been logged.
                        if (exception is not InvalidProjectFileException
-                            && !(exception is AggregateException aggregateException && aggregateException.InnerExceptions.All(innerException => innerException is InvalidProjectFileException)))
+                            && !(exception is AggregateException aggregateException && aggregateException.InnerExceptions.All(innerException => innerException is InvalidProjectFileException))


Suggested change

&& !(exception is AggregateException aggregateException && aggregateException.InnerExceptions.All(innerException => innerException is InvalidProjectFileException))

&& !(exception is AggregateException aggregateException && aggregateException.InnerExceptions.All(innerException => innerException is InvalidProjectFileException || innerException is CircularDependencyException))

?

It actually has to be && exception is not CircularDependencyException

You already added that one; I meant within the AggregateException. I imagine it's ok if it's an aggregation of InvalidProjectFileExceptions and CircularDependencyExceptions, right?

I don't think a InvalidProjectFileException and CircularDependencyException are both possible at the same time. I think all projects are evaluated and then as a separate pass the edges are added and that's where circular dependencies are detected. The 1st pass would error with InvalidProjectFileExceptions

dfederm · 2022-07-15T20:53:30Z

Compare before and after when there are multiple invalid projects in the graph:

C:\Users\David\Code\tmp>"C:\Program Files\Microsoft Visual Studio\2022\Enterprise\MSBuild\Current\Bin\amd64\MSBuild.exe" a.proj /graph
Microsoft (R) Build Engine version 17.2.1+52cd2da31 for .NET Framework
Copyright (C) Microsoft Corporation. All rights reserved.

Build started 7/15/2022 1:52:16 PM.
C:\Users\David\Code\tmp\d.proj(1,1): error MSB4068: The element <ProjectWhat> is unrecognized, or not supported in this context.

Build FAILED.

  C:\Users\David\Code\tmp\d.proj(1,1): error MSB4068: The element <ProjectWhat> is unrecognized, or not supported in this context.

    0 Warning(s)
    1 Error(s)

Time Elapsed 00:00:00.02

C:\Users\David\Code\tmp>C:\Users\David\Code\msbuild\artifacts\bin\bootstrap\net472\MSBuild\Current\Bin\amd64\msbuild.exe a.proj /graph
MSBuild version 17.4.0-dev-22365-01+1223c9a6b for .NET Framework
Build started 7/15/2022 1:52:22 PM.
C:\Users\David\Code\tmp\c.proj(1,1): error MSB4068: The element <ProjectWhat> is unrecognized, or not supported in this context.
C:\Users\David\Code\tmp\d.proj(1,1): error MSB4068: The element <ProjectWhat> is unrecognized, or not supported in this context.

Build FAILED.

  C:\Users\David\Code\tmp\c.proj(1,1): error MSB4068: The element <ProjectWhat> is unrecognized, or not supported in this context.
  C:\Users\David\Code\tmp\d.proj(1,1): error MSB4068: The element <ProjectWhat> is unrecognized, or not supported in this context.

    0 Warning(s)
    2 Error(s)

Time Elapsed 00:00:00.25

dfederm and others added 3 commits July 15, 2022 12:19

Simplify exception handling for graph builds

3a0b58e

Fix UT

56737a1

More UT fixes

1223c9a

Forgind approved these changes Jul 15, 2022

View reviewed changes

Don't dump stack for CircularDependencyException in XMake.cs

cf45f58

Forgind reviewed Jul 15, 2022

View reviewed changes

rainersigwald approved these changes Jul 15, 2022

View reviewed changes

Forgind added the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label Jul 15, 2022

Forgind merged commit f252746 into dotnet:main Jul 16, 2022

dfederm deleted the cleanup-graph-exception-handling branch July 16, 2022 05:30

jeffkl mentioned this pull request Sep 19, 2022

[DCR]: Static graph-based restore should handle an AggregateException from MSBuild NuGet/Home#12100

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify exception handling for graph builds#7831

Simplify exception handling for graph builds#7831
Forgind merged 4 commits intodotnet:mainfrom
dfederm:cleanup-graph-exception-handling

dfederm commented Jul 15, 2022

Uh oh!

Forgind Jul 15, 2022

Uh oh!

dfederm Jul 15, 2022

Uh oh!

dfederm Jul 15, 2022

Uh oh!

Forgind Jul 15, 2022

Uh oh!

Forgind Jul 15, 2022

Uh oh!

Forgind Jul 15, 2022

Uh oh!

Forgind Jul 15, 2022

Uh oh!

dfederm Jul 15, 2022

Uh oh!

Forgind Jul 15, 2022

Uh oh!

dfederm Jul 15, 2022

Uh oh!

dfederm commented Jul 15, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	&& !(exception is AggregateException aggregateException && aggregateException.InnerExceptions.All(innerException => innerException is InvalidProjectFileException))
	&& !(exception is AggregateException aggregateException && aggregateException.InnerExceptions.All(innerException => innerException is InvalidProjectFileException \|\| innerException is CircularDependencyException))

Conversation

dfederm commented Jul 15, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dfederm commented Jul 15, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants