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

NuGet.Client VSIX migration to new interop assemblies #3999

Merged
merged 36 commits into from
Apr 15, 2021

Conversation

debonte
Copy link
Contributor

@debonte debonte commented Apr 14, 2021

Bug

Fixes: https://github.com/NuGet/Client.Engineering/issues/754

Regression? No

Description

Follow up to #3975 which was focused on NuGet.VisualStudio. This PR updates the rest of the NuGet.Client projects to consume the new VS interop assemblies.

Integration tests and functional tests have been disabled because they are not passing yet and the focus of this PR is to build a VSIX which can be inserted into the interop branch to unblock other teams.

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

dtivel and others added 30 commits March 31, 2021 06:57
… Microsoft.VisualStudio.Imaging.Interop.ImageMoniker
…ageManagement.VisualStudio.Test and NuGet.SolutionRestoreManager.Test
…d property for Microsoft.VisualStudio.ProjectSystem.Managed* version to keep PackageDownloads in sync
@debonte debonte requested review from rrelyea and dtivel April 14, 2021 22:55
@debonte debonte requested a review from a team as a code owner April 14, 2021 22:55
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.

Given the urgency of this change, my feedback is primarily that we should create a few issues to clean a few things up before this makes it to dev branch.

<PackageReference Update="Microsoft.VisualStudio.Language.Intellisense" Version="16.10.31" />
<PackageReference Update="Microsoft.VisualStudio.Language.StandardClassification" Version="16.10.31" />
<PackageReference Update="Microsoft.VisualStudio.Editor" Version="17.0.24-g4265d215ae" />
<PackageReference Update="Microsoft.VisualStudio.Imaging.Interop.14.0.DesignTime" Version="14.3.26931" NoWarn="NU1605" />
Copy link
Member

Choose a reason for hiding this comment

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

NoWarn/ExcludeAssets/IncludeAssets should not be set in the packages.targets file cause it messes with visibility.

See: https://github.com/NuGet/NuGet.Client/blob/dev/build/packages.targets for example, it contains none of these metadata items.

Copy link
Member

Choose a reason for hiding this comment

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

Note that I understand that this is something we're trying to get done sooner than later, so I'm not necessarily asking for a change, but rather that we should create an issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nkolev92 Understood. There are still hacks here. Some of them may not even be needed anymore. To give you some context, I only had my first successful lab build yesterday and I have no way to test these changes in VS. So everything is still rough.

The goal of this PR is just to produce packages that will unblock the interop work of other teams since we're trying to have the interop work all-up completed by EOW. The initial PR in this branch was hacky and this one builds upon that one. There will be more changes coming.

  1. I would prefer not to create issues to track the remaining work here, but I will do so if that's what the team wants. In my mind it's all covered by https://github.com/NuGet/Client.Engineering/issues/754 which is not yet complete, even with this PR.
  2. I was hoping that once this work was complete I could create a PR to review and merge all of the interop-related changes from this branch into the "real" 6.0.x branch. Perhaps that doesn't make sense given the official-sounding name of this branch though. I wonder if @dtivel and @rrelyea had a long-term branch plan for this work. Perhaps calling the target branch release-6.0.x was a mistake? Or perhaps it was necessary so the right things (release things) would happen when changes were made in this branch.

Copy link
Member

Choose a reason for hiding this comment

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

Discussed offline.
We'll just review these changes in more detail when they are ported into the long term branch.

<PackageReference Update="Microsoft.VisualStudio.Text.Logic" Version="17.0.24-g4265d215ae" />
<PackageReference Update="Microsoft.VisualStudio.Text.UI" Version="17.0.24-g4265d215ae" />
<PackageReference Update="Microsoft.VisualStudio.Text.UI.Wpf" Version="17.0.24-g4265d215ae" />
<PackageReference Update="Microsoft.VisualStudio.TextManager.Interop" Version="17.0.0-preview-1-31108-209" ExcludeAssets="all" PrivateAssets="all" />
Copy link
Member

Choose a reason for hiding this comment

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

Same feedback as above.

@@ -322,7 +322,8 @@ public async Task PackageExpander_Recovers_WhenStreamIsCorrupt()
}
}

[Fact]
// ERIKD TODO REENABLE
Copy link
Member

Choose a reason for hiding this comment

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

@debonte debonte merged commit 10b004c into release-6.0.x Apr 15, 2021
@debonte debonte deleted the dev-dtivel-dev17 branch April 15, 2021 05:51
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.

3 participants