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.

Sign up for GitHub

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

Jump to bottom

Add better handling of AggregateExceptions in static graph-based restore #4809

Merged
merged 5 commits into from
Sep 26, 2022
Merged

Add better handling of AggregateExceptions in static graph-based restore #4809

merged 5 commits into from
Sep 26, 2022

Conversation

jeffkl
Copy link
Contributor

@jeffkl jeffkl commented Sep 19, 2022 •
edited
Loading

Bug

Fixes: NuGet/Home#12100

Regression? Last working version:

Description

A behavior change in MSBuild's static graph API means that an AggregateException is now thrown instead of an InvalidProjectFileException. This change updates the exception handling in static graph-based restore to use a single method for handling logging of exceptions.

  1. If the exception is an AggregateException, call the LogErrorFromException() method on each inner exception
  2. If the exception is an InvalidProjectFileException, call the MSBuild logging API that surfaces the extra information contained in the exception
  3. All other exceptions are logged with the full stack trace so that user reports contain as much information as possible

PR Checklist

  • PR has a meaningful title

  • PR has a linked issue.

  • Described changes

  • Tests

    • Automated tests added
    • OR
    • Test exception
    • OR
    • N/A

  • Documentation

    • Documentation PR or issue filled
    • OR
    • N/A

@jeffkl jeffkl self-assigned this Sep 19, 2022
@jeffkl jeffkl requested a review from a team as a code owner September 19, 2022 19:01
@jeffkl
Copy link
Contributor Author

jeffkl commented Sep 19, 2022 •
edited
Loading

Before:

C:\Program Files\Visual Studio 2022\Common7\IDE\CommonExtensions\Microsoft\NuGet\NuGet.RestoreEx.targets(19,5): error : The project file could not be loaded. Could not find a part of the path '<PATHTOPROJECT>'.  <PATHTOPROJECT> [<PATHTOPROJECT>]
C:\Program Files\Visual Studio 2022\Common7\IDE\CommonExtensions\Microsoft\NuGet\NuGet.RestoreEx.targets(19,5): error :    at Microsoft.Build.Shared.ProjectFileErrorUtilities.VerifyThrowInvalidProjectFile(Boolean condition, String errorSubCategoryResourceName, BuildEventFileInfo projectFile, Exception innerException, String resourceName, Object[] args) [<PATHTOPROJECT>]
C:\Program Files\Visual Studio 2022\Common7\IDE\CommonExtensions\Microsoft\NuGet\NuGet.RestoreEx.targets(19,5): error :    at Microsoft.Build.Shared.ProjectFileErrorUtilities.ThrowInvalidProjectFile(BuildEventFileInfo projectFile, Exception innerException, String resourceName, Object[] args) [<PATHTOPROJECT>]
C:\Program Files\Visual Studio 2022\Common7\IDE\CommonExtensions\Microsoft\NuGet\NuGet.RestoreEx.targets(19,5): error :    at Microsoft.Build.Construction.ProjectRootElement.LoadDocument(String fullPath, Boolean preserveFormatting, Boolean loadAsReadOnly) [<PATHTOPROJECT>]
C:\Program Files\Visual Studio 2022\Common7\IDE\CommonExtensions\Microsoft\NuGet\NuGet.RestoreEx.targets(19,5): error :    at Microsoft.Build.Construction.ProjectRootElement..ctor(String path, ProjectRootElementCacheBase projectRootElementCache, Boolean preserveFormatting) [<PATHTOPROJECT>]
C:\Program Files\Visual Studio 2022\Common7\IDE\CommonExtensions\Microsoft\NuGet\NuGet.RestoreEx.targets(19,5): error :    at Microsoft.Build.Construction.ProjectRootElement.CreateProjectFromPath(String projectFile, ProjectRootElementCacheBase projectRootElementCache, Boolean preserveFormatting) [<PATHTOPROJECT>]
C:\Program Files\Visual Studio 2022\Common7\IDE\CommonExtensions\Microsoft\NuGet\NuGet.RestoreEx.targets(19,5): error :    at Microsoft.Build.Evaluation.ProjectRootElementCache.Get(String projectFile, OpenProjectRootElement loadProjectRootElement, Boolean isExplicitlyLoaded, Nullable`1 preserveFormatting) [<PATHTOPROJECT>]
C:\Program Files\Visual Studio 2022\Common7\IDE\CommonExtensions\Microsoft\NuGet\NuGet.RestoreEx.targets(19,5): error :    at Microsoft.Build.Construction.ProjectRootElement.OpenProjectOrSolution(String fullPath, IDictionary`2 globalProperties, String toolsVersion, ProjectRootElementCacheBase projectRootElementCache, Boolean isExplicitlyLoaded) [<PATHTOPROJECT>]
C:\Program Files\Visual Studio 2022\Common7\IDE\CommonExtensions\Microsoft\NuGet\NuGet.RestoreEx.targets(19,5): error :    at Microsoft.Build.Execution.ProjectInstance..ctor(String projectFile, IDictionary`2 globalProperties, String toolsVersion, String subToolsetVersion, ProjectCollection projectCollection, Nullable`1 projectLoadSettings, EvaluationContext evaluationContext) [<PATHTOPROJECT>]
C:\Program Files\Visual Studio 2022\Common7\IDE\CommonExtensions\Microsoft\NuGet\NuGet.RestoreEx.targets(19,5): error :    at Microsoft.Build.Execution.ProjectInstance.FromFile(String file, ProjectOptions options) [<PATHTOPROJECT>]
C:\Program Files\Visual Studio 2022\Common7\IDE\CommonExtensions\Microsoft\NuGet\NuGet.RestoreEx.targets(19,5): error :    at NuGet.Build.Tasks.Console.MSBuildStaticGraphRestore.<>c__DisplayClass38_1.<LoadProjects>b__0(String path, Dictionary`2 properties, ProjectCollection collection) [<PATHTOPROJECT>]
C:\Program Files\Visual Studio 2022\Common7\IDE\CommonExtensions\Microsoft\NuGet\NuGet.RestoreEx.targets(19,5): error :    at Microsoft.Build.Graph.GraphBuilder.ParseProject(ConfigurationMetadata configurationMetadata) [<PATHTOPROJECT>]
C:\Program Files\Visual Studio 2022\Common7\IDE\CommonExtensions\Microsoft\NuGet\NuGet.RestoreEx.targets(19,5): error :    at System.Lazy`1.CreateValue() [<PATHTOPROJECT>]
C:\Program Files\Visual Studio 2022\Common7\IDE\CommonExtensions\Microsoft\NuGet\NuGet.RestoreEx.targets(19,5): error :    at System.Lazy`1.LazyInitValue() [<PATHTOPROJECT>]
C:\Program Files\Visual Studio 2022\Common7\IDE\CommonExtensions\Microsoft\NuGet\NuGet.RestoreEx.targets(19,5): error :    at Microsoft.Build.Graph.ParallelWorkSet`2.ExecuteWorkItem() [<PATHTOPROJECT>]
C:\Program Files\Visual Studio 2022\Common7\IDE\CommonExtensions\Microsoft\NuGet\NuGet.RestoreEx.targets(19,5): error :    at Microsoft.Build.Graph.ParallelWorkSet`2.WaitForAllWorkAndComplete() [<PATHTOPROJECT>]
C:\Program Files\Visual Studio 2022\Common7\IDE\CommonExtensions\Microsoft\NuGet\NuGet.RestoreEx.targets(19,5): error :    at Microsoft.Build.Graph.GraphBuilder.FindGraphNodes() [<PATHTOPROJECT>]
C:\Program Files\Visual Studio 2022\Common7\IDE\CommonExtensions\Microsoft\NuGet\NuGet.RestoreEx.targets(19,5): error :    at Microsoft.Build.Graph.GraphBuilder.BuildGraph() [<PATHTOPROJECT>]
C:\Program Files\Visual Studio 2022\Common7\IDE\CommonExtensions\Microsoft\NuGet\NuGet.RestoreEx.targets(19,5): error :    at Microsoft.Build.Graph.ProjectGraph..ctor(IEnumerable`1 entryPoints, ProjectCollection projectCollection, ProjectInstanceFactoryFunc projectInstanceFactory, Int32 degreeOfParallelism, CancellationToken cancellationToken) [<PATHTOPROJECT>]
C:\Program Files\Visual Studio 2022\Common7\IDE\CommonExtensions\Microsoft\NuGet\NuGet.RestoreEx.targets(19,5): error :    at NuGet.Build.Tasks.Console.MSBuildStaticGraphRestore.LoadProjects(IEnumerable`1 entryProjects) [<PATHTOPROJECT>]
    0 Warning(s)
    1 Error(s)

After:

<PATHTOPROJECT> : error MSB4025: The project file could not be loaded. Could not find a part of the path '<PATHTOPROJECT>'.  <PATHTOPROJECT> [<PATHTOPROJECT>]
<PATHTOPROJECT> : error MSB4025: The project file could not be loaded. Could not find a part of the path '<PATHTOPROJECT>'.  <PATHTOPROJECT> [<PATHTOPROJECT>]
<PATHTOPROJECT> : error MSB4025: The project file could not be loaded. Could not find a part of the path '<PATHTOPROJECT>'.  <PATHTOPROJECT> [<PATHTOPROJECT>]
<PATHTOPROJECT> : error MSB4025: The project file could not be loaded. Could not find a part of the path '<PATHTOPROJECT>'.  <PATHTOPROJECT> [<PATHTOPROJECT>]
<PATHTOPROJECT> : error MSB4025: The project file could not be loaded. Could not find a part of the path '<PATHTOPROJECT>'.  <PATHTOPROJECT> [<PATHTOPROJECT>]
<PATHTOPROJECT> : error MSB4025: The project file could not be loaded. Could not find a part of the path '<PATHTOPROJECT>'.  <PATHTOPROJECT> [<PATHTOPROJECT>]
<PATHTOPROJECT> : error MSB4025: The project file could not be loaded. Could not find a part of the path '<PATHTOPROJECT>'.  <PATHTOPROJECT> [<PATHTOPROJECT>]
<PATHTOPROJECT> : error MSB4025: The project file could not be loaded. Could not find a part of the path '<PATHTOPROJECT>'.  <PATHTOPROJECT> [<PATHTOPROJECT>]
<PATHTOPROJECT> : error MSB4025: The project file could not be loaded. Could not find a part of the path '<PATHTOPROJECT>'.  <PATHTOPROJECT> [<PATHTOPROJECT>]
<PATHTOPROJECT> : error MSB4025: The project file could not be loaded. Could not find a part of the path '<PATHTOPROJECT>'.  <PATHTOPROJECT> [<PATHTOPROJECT>]
<PATHTOPROJECT> : error MSB4025: The project file could not be loaded. Could not find a part of the path '<PATHTOPROJECT>'.  <PATHTOPROJECT> [<PATHTOPROJECT>]
<PATHTOPROJECT> : error MSB4025: The project file could not be loaded. Could not find a part of the path '<PATHTOPROJECT>'.  <PATHTOPROJECT> [<PATHTOPROJECT>]
<PATHTOPROJECT> : error MSB4025: The project file could not be loaded. Could not find a part of the path '<PATHTOPROJECT>'.  <PATHTOPROJECT> [<PATHTOPROJECT>]
<PATHTOPROJECT> : error MSB4025: The project file could not be loaded. Could not find a part of the path '<PATHTOPROJECT>'.  <PATHTOPROJECT> [<PATHTOPROJECT>]
    0 Warning(s)
    14 Error(s)

/cc @AArnott

@AArnott
Copy link
Contributor

AArnott commented Sep 19, 2022

It looks good. But please advise the MSBuild team to consider making a similar change in their own logger method.

nkolev92
nkolev92 previously approved these changes Sep 19, 2022
View reviewed changes
Copy link
Member

@nkolev92 nkolev92 left a comment

Choose a reason for hiding this comment

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

I know the test wouldn't do as much, but we can at least assert the deconstruction is happening.

I'm fine with whatever you decide.

src/NuGet.Core/NuGet.Build.Tasks.Console/MSBuildStaticGraphRestore.cs Show resolved Hide resolved
zivkan
zivkan reviewed Sep 19, 2022
View reviewed changes
test/NuGet.Core.FuncTests/Msbuild.Integration.Test/MsbuildRestoreTaskTests.cs Outdated Show resolved Hide resolved
test/NuGet.Core.FuncTests/Msbuild.Integration.Test/MsbuildRestoreTaskTests.cs Outdated
Comment on lines 816 to 818
await SimpleTestPackageUtility.CreateFolderFeedV3Async(
pathContext.PackageSource,
packageX);
Copy link
Member

Choose a reason for hiding this comment

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

packageX doesn't seem to be used in any of the asserts, so I believe it can be removed from the arrange and save a little time not needing to create the nupkg.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without a PackageReference I get this:

Nothing to do. None of the projects specified contain packages to restore.

But it does look like I can get rid of the package creation since its not actually used

Copy link
Member

Choose a reason for hiding this comment

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

This tells me the project is not SDK style, and that the csproj does not contain <ProjectRestoreStyle>PackageReference</ProjectRestoreStyle> (I'm not 100% sure I got the property name right).

Is it worthwhile improving our test infrastructure code to do this?

@jeffkl
Copy link
Contributor Author

jeffkl commented Sep 19, 2022

It looks good. But please advise the MSBuild team to consider making a similar change in their own logger method.

I've filed dotnet/msbuild#7985

nkolev92
nkolev92 previously approved these changes Sep 19, 2022
View reviewed changes
@jeffkl
Copy link
Contributor Author

jeffkl commented Sep 20, 2022

My unit test is failing because the version of MSBuild we test against doesn't have the newer functionality. I'm going to have the test only have one invalid project which exhibits the same behavior in 17.3 and 17.4.

@jeffkl jeffkl merged commit 3808641 into dev Sep 26, 2022
@jeffkl jeffkl deleted the dev-jeffkl-static-graph-restore-aggregate-exception-handling branch September 26, 2022 17:02
AdmiringWorm added a commit to chocolatey/NuGet.Client that referenced this pull request Dec 19, 2022
@AdmiringWorm
Merge tag '6.4.0.123' into develop
6ab0442 
Insert 6.4.0-rc.123 into rel/d17.4 on 11/07/2022 23:47:12

* tag '6.4.0.123': (60 commits)
  fix a logic error that caused AbandonedMutexException while executing migrations (release-6.4.x) (NuGet#4895)
  unblock source build failing due to fatal: transport 'file' not allowed error (NuGet#4867) (NuGet#4874)
  Signing:  update to August 2022 CTL (NuGet#4791) (NuGet#4850)
  Merged PR 422933: Prefer BCL Directory create API over helper class (7.0.1xx-rc2)
  Fix empty combobox when package is not present in project file (NuGet#4844) (NuGet#4848)
  Fix component detection alert for microsoft.owin package (NuGet#4841) (NuGet#4845)
  Make release label RC, move to escrow mode
  Adds special case to include transitive origins in GetInstalledAndTransitivePackagesAsync API (NuGet#4824)
  Add longPathAware manifest to NuGet.Build.Tasks.Console (NuGet#4830)
  VsPackageInstallerServices should not post ProjectNotNominatedException faults (NuGet#4814)
  Skip test GetOrCreateAsync_WithUnhandledExceptionInPlugin_Throws (NuGet#4831)
  Improve OptProf pipeline job run names (NuGet#4825)
  Increase HttpClientHandler.MaxConnectionsPerServer to 64 to improve PM UI performance in Visual Studio (NuGet#4798)
  Suppress CA2213 warnings to unblock dev branch (NuGet#4823)
  Ensure IsVsOfflineFeed is calculated correctly on 64-bit machines (NuGet#4817)
  Add better handling of AggregateExceptions in static graph-based restore (NuGet#4809)
  Add Component Detection task into each pipeline (NuGet#4813)
  Localizes nuget.exe with default, embedded resource assembly lookup (NuGet#4773)
  Removes BrowseObjectBase class in NuGet Solution Explorer (NuGet#4807)
  Improve TryCreateContext  (NuGet#4762)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nkolev92 nkolev92 nkolev92 approved these changes

@kartheekp-ms kartheekp-ms Awaiting requested review from kartheekp-ms

@zivkan zivkan Awaiting requested review from zivkan

Assignees

@jeffkl jeffkl

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[DCR]: Static graph-based restore should handle an AggregateException from MSBuild
5 participants
@jeffkl @AArnott @nkolev92 @zivkan @kartheekp-ms