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.

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

VsPackageInstallerServices should not post ProjectNotNominatedException faults #4814

Merged

Conversation

zivkan
Copy link
Member

@zivkan zivkan commented Sep 21, 2022

Bug

Fixes: NuGet/Home#12103

Regression? No

Description

Catch ProjectNotNominatedException in VsProjectInstallerServices, and rethrow without posting fault telemetry. These APIs are already marked obsolete, encouraging developers to use the async INuGetProjectService.GetInstalledPackagesAsync instead, where project status was already designed into the API.

PR Checklist

  • PR has a meaningful title

  • PR has a linked issue.

  • Described changes

  • Tests

    • Automated tests added
    • OR
    • Test exception: DTE makes it impossible to unit test. Scenario is timing based, making it very difficult to reliably reproduce in an integration (E2E or Apex) test, and the impact is not high enough to warrant the effort.
    • OR
    • N/A
  • Documentation

@zivkan zivkan requested a review from a team as a code owner September 21, 2022 01:51
nkolev92
nkolev92 previously approved these changes Sep 21, 2022
donnie-msft
donnie-msft previously approved these changes Sep 21, 2022
@JihedHalimi
Copy link

Same problem with IVsPackageInstaller.InstallPackage which is not marked as Obsolete. The problem occurs in a custom wizard after adding a new solution with a new project.

IComponentModel componentModel = (IComponentModel)Package.GetGlobalService(typeof(SComponentModel));
IVsPackageInstaller packageInstaller = componentModel.GetService();

packageInstaller.InstallPackage(null, project, package, version, ignoreDependencies: false);

What is the recommended way of adding a PackageReference to a project via EnvDTE?

@nkolev92
Copy link
Member

@JihedHalimi

The comment in this PR for the other API talks about using Microsoft.VisualStudio.OperationProgress to know when the project is ready.
Same recommendation applies to IVsPackageInstaller.

We should add that in the docs for the IVsPackageInstaller/IvsPackageUninstaller.

@zivkan
Copy link
Member Author

zivkan commented Sep 22, 2022

The problem is that @JihedHalimi said it happens in a "wizard", which I assume means "new project wizard" (template). I don't know how the wizard and project system is implemented, but I wouldn't be surprised if project system doesn't nominate (CPS might not fully load the project?) until after the template wizard finishes. If the wizard is waiting for OperationProgress, then there will be a deadlock.

I think this is a design gap. In any case, it's out of scope for this PR, so it's best to bring up in a new issue in the Home repo.

@nkolev92
Copy link
Member

oh I missed the part about the wizard.

For SDK style projects, you should declare the packages as PackageReference in the project file itself instead of using IVsPackageInstaller.
That's how the SDK based templates that have default packages work.

@zivkan zivkan dismissed stale reviews from donnie-msft and nkolev92 via 69c0354 September 27, 2022 01:53
@zivkan zivkan force-pushed the dev-zivkan-vs-getinstalledpackages-projectnotfoundexception branch from 533cd85 to 69c0354 Compare September 27, 2022 01:53
@zivkan zivkan force-pushed the dev-zivkan-vs-getinstalledpackages-projectnotfoundexception branch from 69c0354 to b2a25e4 Compare September 28, 2022 06:51
@zivkan zivkan merged commit 17b52a9 into dev Sep 28, 2022
@zivkan zivkan deleted the dev-zivkan-vs-getinstalledpackages-projectnotfoundexception branch September 28, 2022 20:51
AdmiringWorm added a commit to chocolatey/NuGet.Client that referenced this pull request Dec 19, 2022
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: IVsPackageInstallerServices APIs sometimes throw ProjectNotNominatedException
5 participants