Investigate if uninstall have more than 1 restore problem for reference type projects. #9932
Assignees
Labels
Functionality:Restore
Functionality:VisualStudioUI
Package Manager UI et al
Priority:2
Issues for the current backlog.
Product:VS.Client
Tenet:Performance
Performance issues
Milestone
Comments
kartheekp-ms
added
Functionality:Restore
Functionality:VisualStudioUI
|
I think a good test case for this is; Project 1 -> Project 2 -> Project 3, where 2 & 3, have Package A. Quick test on my end, uninstall does have a problem. See how P2 is restored twice! |
|
It looks you got this log from public VS2019. I got similar duplicate restore on my TestRepo: I didn't really target uninstall scenarios but it looks already solved. https://gist.github.com/erdembayar/1a97b3d2853c5af432e0117392a01303 |
|
It looks both install and uninstall actions share same code path NugetPackageManager#Line2601 where code optimization change was made. So now uninstall scenario also benefit from my change too. |
Can you please add tests for it then? A scenario like the one I called out in #9932 (comment), would be great! Assigning to you given that your change is already addressing it. |
erdembayar
added a commit
to NuGet/NuGet.Client
that referenced
this issue
Aug 28, 2020
…n referenced projects. Address: NuGet/Home#9932 (comment)
|
I added 2 unit tests for this uninstall case: |
erdembayar
added a commit
to NuGet/NuGet.Client
that referenced
this issue
Sep 3, 2020
…for every project instead of after all projects are updated (#3559) * Added relationship table to hold ancesstors and descendents relationship for target projects. * Now 1065 is solved because we pass correct update PackageSpec of child before parent restores. * Remove RelationshipTree for now. * Add unshipped public api. * Remove unused item and add initial value to dictionary to prevent from null exception. * More streamline package reference type code restore. * Revert "More streamline package reference type code restore." This reverts commit 875968b. * Restore NugetBuiltIntegrated restore works only in 1 pass. * Minor refactoring for naming. * Explicit clearing of cache. * Initial refactoring of moving caching of updated PackageSpec near to usage, make it run parellel not finished yet. * Made BuiltIntegratedProject project upgrade/downgrade parallel run for faster run. * Fix minor typos. * Remove unused methods and variables. * Remove more unused item. * Revert "Remove more unused item." This reverts commit efa1782. * Patching in memory works. * Cleanup * Minor text change * More code cleanup. * Rename PreviewBuildIntegratedProjectActionsParallelAsync method to PreviewBuildIntegratedProjectsActionsAsync * Add unit tests for covering PreviewBuildIntegratedProjectsActionsAsync method in NuGetPackageManager. * Add more unit tests for BuildIntegratedTests * Address Andy's code review comment. * Add unit test for DependencyGraphSpec>>WithReplacedPackageSpecs new method I created. * Get Andy's advise for reduce runtime time complexity. * Change package versions because it's failing on .NetCore unit tests. * Check if Newtonsoft version matches. * Try different pacakge for unit test. * Test * Address comment about foreach loop variable name. * Remove 'parallel' word from comment. Because actual parallel code is called thown the line different method. * PackageReference/SDK style unit tests start working. * Making source works for unit tests. * Fixed problem of Unit test project not picking up my custom PackageSource. * Revert LegacyPackageReferenceProject related changes, maybe I don't need it for now. * Git clean up and fix added unit tests with new PackSource logic. * Fix typo. * Add NU1065 and new PackageSource handling to some unit test I forgot to add. * Fix formatting recommendation in code review. * Remove parallel wording from method comment, because actual parallelization happend down the line another called method here. * Fix typo. * Refactor code after Andy's code review. thinking about API design: Why does this method only accept BuildIntegratedNuGetProject? Why can't this method be made internal, and the existing public API PreviewInstallPackageAsync calls it for compatible project types? * Fix small typo. * After refactoring code PreviewBuildIntegratedProjectActionsAsync and PreviewBuildIntegratedProjectActionsAsync share same logic via PreviewBuildIntegratedProjectsActionsAsync method. * All PreviewProjectsInstallPackageAsync related unit test for NugetPackageManager are ported to NetCorePackageReferenceProject(reference) type unit test from project json type unit tests. * Remove the spaces and correct comments. * Revert changes to BuildIntegratedTests.cs since we already ported all new unit tests to NetCorePackageReferenceProjectTests.cs * Fix typo. * Revert changes to LegacyPackageReferenceProjectTests.cs * Revert back unneeded changes to NuGetPackageManagerTests.cs * Remove unneeded change. * Add missing copy right notice for new file GetPackageReferenceUtility.cs * Add missing copyright notice for ProjectInstalledPackages.cs * Remove wrong file. * Revert unneeded changes. * Fix typos. * Address code review comments and clean up. * Now also remembers we evaluated the parents too. * Rename method name after nkolve92 code review. * Address Nikolche code review comment about parent projects. * Remove method PreviewResolveActionsForBuildIntegratedProjectsAsync method since it's so simple. * Make sure restore happens only once during uninstall process for chain referenced projects. Address: NuGet/Home#9932 (comment) * Changed install package to all projects that it check for restore only happens once per project. * Clean up unused items. * Replace StringComparer.OrdinalIgnoreCase with PathUtility.GetStringComparerBasedOnOS() for other OS compatibility. * Refactor the unit tests after Nikolche's code review. * Change unit test names. * Fix formatting. * Fix with dot net formatting. * Add comment why we're separating BuildIntegratedNuGetProject vs packages.config projects. * Address more code review comments by Cristina. * Address Svet's code review comment. * Fix formatting. * Address Andy's latest code review. * Refactor unit test after Andy's code review. * Fix unit test after Nikolche review. * Fix formatting. Co-authored-by: Erick Yondon <eryondon@microsoft.com>
|
Closing per NuGet/NuGet.Client@53a7cf2 |
dtivel
pushed a commit
to NuGet/NuGet.Client
that referenced
this issue
Sep 14, 2020
…for every project instead of after all projects are updated (#3559) * Added relationship table to hold ancesstors and descendents relationship for target projects. * Now 1065 is solved because we pass correct update PackageSpec of child before parent restores. * Remove RelationshipTree for now. * Add unshipped public api. * Remove unused item and add initial value to dictionary to prevent from null exception. * More streamline package reference type code restore. * Revert "More streamline package reference type code restore." This reverts commit 875968b. * Restore NugetBuiltIntegrated restore works only in 1 pass. * Minor refactoring for naming. * Explicit clearing of cache. * Initial refactoring of moving caching of updated PackageSpec near to usage, make it run parellel not finished yet. * Made BuiltIntegratedProject project upgrade/downgrade parallel run for faster run. * Fix minor typos. * Remove unused methods and variables. * Remove more unused item. * Revert "Remove more unused item." This reverts commit efa1782. * Patching in memory works. * Cleanup * Minor text change * More code cleanup. * Rename PreviewBuildIntegratedProjectActionsParallelAsync method to PreviewBuildIntegratedProjectsActionsAsync * Add unit tests for covering PreviewBuildIntegratedProjectsActionsAsync method in NuGetPackageManager. * Add more unit tests for BuildIntegratedTests * Address Andy's code review comment. * Add unit test for DependencyGraphSpec>>WithReplacedPackageSpecs new method I created. * Get Andy's advise for reduce runtime time complexity. * Change package versions because it's failing on .NetCore unit tests. * Check if Newtonsoft version matches. * Try different pacakge for unit test. * Test * Address comment about foreach loop variable name. * Remove 'parallel' word from comment. Because actual parallel code is called thown the line different method. * PackageReference/SDK style unit tests start working. * Making source works for unit tests. * Fixed problem of Unit test project not picking up my custom PackageSource. * Revert LegacyPackageReferenceProject related changes, maybe I don't need it for now. * Git clean up and fix added unit tests with new PackSource logic. * Fix typo. * Add NU1065 and new PackageSource handling to some unit test I forgot to add. * Fix formatting recommendation in code review. * Remove parallel wording from method comment, because actual parallelization happend down the line another called method here. * Fix typo. * Refactor code after Andy's code review. thinking about API design: Why does this method only accept BuildIntegratedNuGetProject? Why can't this method be made internal, and the existing public API PreviewInstallPackageAsync calls it for compatible project types? * Fix small typo. * After refactoring code PreviewBuildIntegratedProjectActionsAsync and PreviewBuildIntegratedProjectActionsAsync share same logic via PreviewBuildIntegratedProjectsActionsAsync method. * All PreviewProjectsInstallPackageAsync related unit test for NugetPackageManager are ported to NetCorePackageReferenceProject(reference) type unit test from project json type unit tests. * Remove the spaces and correct comments. * Revert changes to BuildIntegratedTests.cs since we already ported all new unit tests to NetCorePackageReferenceProjectTests.cs * Fix typo. * Revert changes to LegacyPackageReferenceProjectTests.cs * Revert back unneeded changes to NuGetPackageManagerTests.cs * Remove unneeded change. * Add missing copy right notice for new file GetPackageReferenceUtility.cs * Add missing copyright notice for ProjectInstalledPackages.cs * Remove wrong file. * Revert unneeded changes. * Fix typos. * Address code review comments and clean up. * Now also remembers we evaluated the parents too. * Rename method name after nkolve92 code review. * Address Nikolche code review comment about parent projects. * Remove method PreviewResolveActionsForBuildIntegratedProjectsAsync method since it's so simple. * Make sure restore happens only once during uninstall process for chain referenced projects. Address: NuGet/Home#9932 (comment) * Changed install package to all projects that it check for restore only happens once per project. * Clean up unused items. * Replace StringComparer.OrdinalIgnoreCase with PathUtility.GetStringComparerBasedOnOS() for other OS compatibility. * Refactor the unit tests after Nikolche's code review. * Change unit test names. * Fix formatting. * Fix with dot net formatting. * Add comment why we're separating BuildIntegratedNuGetProject vs packages.config projects. * Address more code review comments by Cristina. * Address Svet's code review comment. * Fix formatting. * Address Andy's latest code review. * Refactor unit test after Andy's code review. * Fix unit test after Nikolche review. * Fix formatting. Co-authored-by: Erick Yondon <eryondon@microsoft.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Details about Problem
It's follow up for
Nuget Solution Package manager UI is slow because restore is run for every project instead of after all projects are updated#6010 task. I solved install/upgrade/downgrade cases, but my changes doesn't cover uninstall case. For 2-3 sample solutions I tried doesn't have multi restore problem, but just in case need to check all code path to make sure.The text was updated successfully, but these errors were encountered: