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

Update Microsoft.VSSDK.BuildTools from 17.0.1600 to 17.12.2069 #6078

Merged
merged 13 commits into from
Oct 17, 2024

Conversation

donnie-msft
Copy link
Contributor

@donnie-msft donnie-msft commented Oct 2, 2024

Bug

Fixes: NuGet/Home#13832

Description

It seems we don't regularly update Microsoft.VSSDK.BuildTools and therefore we have a very old 17.0.x version of that dependency

<PackageVersion Include="Microsoft.VSSDK.BuildTools" Version="17.0.1600" />

  • Update Microsoft.VSSDK.BuildTools from 17.0.1600 to 17.12.2069
    • Note that I did try some slightly older packages from 17.11.x, but experienced vulnerable transitive dependencies and decided the latest stable version was best to work towards.
  • Add reference to the new Microsoft.VisualStudio.ExtensionEngineContract
  • Add/Remove assemblies from the VSIX as necessary from the change to dependent assemblies

Likely, there was some breaking change that's occurred between 17.0 and 17.12.

Side effects

  • The VSSDK was now finding Lucene.net from a different directory via a Project Reference which was not signed, and therefore failed our signing verification. @jeffkl added a step to remove the unsigned version of this assembly when it's being pulled from a Project Reference.
  • The VSSDK was now adding all localization resource assemblies which it did not do previously. This caused the wrong root path to be used for localized assemblies. @jeffkl put in a removal of all of those *resources.dll assemblies items just before our build adds them as it has historically with the GetLocalizedDlls Target that follows.

VSIX content changes

  • Our VSIX no longer includes a NuGet.VisualStudio.Client.clientenabledpkg since the CodeSpaces functionality for this was moved to GitHub Codespaces by 2022
  • The VSSDK is now including generated XML Documentation files for assemblies. I've added this to the ignore list since we do not want to ship them. Our build logic now considers .XML file extensions when loading files for the vsixinclude/vsixignore comparison.
  • Include in the VSIX the NuGet.PackageManagement.PowerShellCmdlets.dll-Help.xml which is the only XML file we ship. It appears to be used for PowerShell Console help (eg, get-help install-package).
Delta in our VSIX content with this PR:
Removed from VSIX
NuGet.VisualStudio.Client.clientenabledpkg
System.Buffers.dll
System.Memory.dll
System.Numerics.Vectors.dll
System.Runtime.CompilerServices.Unsafe.dll

Flattened directory content files used for comparison:
dev branch files.txt
this branch fe0f5e1ed files.txt

Special thanks to @jeffkl for helping address the above side effects with his MS Build expertise :)

PR Checklist

  • Meaningful title, helpful description and a linked NuGet/Home issue
  • Added tests - N/A - I tested that the VSIX builds, installs, and can open PM UI.
  • Link to an issue or pull request to update docs if this PR changes settings, environment variables, new feature, etc.

@donnie-msft donnie-msft requested a review from a team as a code owner October 2, 2024 20:08
jeffkl
jeffkl previously approved these changes Oct 2, 2024
Copy link
Contributor

@jeffkl jeffkl left a comment

Choose a reason for hiding this comment

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

🎉

jebriede
jebriede previously approved these changes Oct 2, 2024
@donnie-msft donnie-msft dismissed stale reviews from jebriede and jeffkl via c0de869 October 3, 2024 17:51
@donnie-msft donnie-msft force-pushed the dev-donnie-msft-buildToolsUpdate branch from c0de869 to 58d8e13 Compare October 3, 2024 18:47
@donnie-msft donnie-msft marked this pull request as draft October 9, 2024 18:19
@donnie-msft donnie-msft force-pushed the dev-donnie-msft-buildToolsUpdate branch from c5582e7 to b9f3b53 Compare October 11, 2024 23:52
@jeffkl jeffkl force-pushed the dev-donnie-msft-buildToolsUpdate branch from b9f3b53 to 758a1b5 Compare October 14, 2024 20:21
@donnie-msft donnie-msft marked this pull request as ready for review October 15, 2024 00:49
@jeffkl jeffkl force-pushed the dev-donnie-msft-buildToolsUpdate branch 3 times, most recently from a28ba09 to 0382cdd Compare October 15, 2024 16:50
@jeffkl jeffkl force-pushed the dev-donnie-msft-buildToolsUpdate branch from 0382cdd to bace686 Compare October 15, 2024 16:51
@@ -272,6 +272,7 @@
<ItemGroup>
<VsixIncludeFile Include=".vsixinclude" />
<VsixIgnoreFile Include=".vsixignore" />
<VsixIgnoreFile Include=".vsixignore.CI" Condition="'$(IsCIBuild)' == 'true'" />
Copy link
Member

Choose a reason for hiding this comment

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

I'd just like to confirm if someone downloaded the vsix from the dev branch, and from this branch, and validated that the files in both vsix's are the same?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did, and they are not. See these commits:
image

Copy link
Member

Choose a reason for hiding this comment

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

Is the PR ready to be reviewed and merged, or is it still being worked on while the file differences are still being worked on?

When this PR pops up in my notification list, I'm not sure if I should review and approve, or if I should wait.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is ready for review. Sorry for the noise in the past day or so, we kept thinking it was done but little things kept cropping up. It appears my build is green which is why I'm updating the description now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a nifty table.

Copy link
Contributor Author

@donnie-msft donnie-msft Oct 16, 2024

Choose a reason for hiding this comment

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

When this PR pops up in my notification list, I'm not sure if I should review and approve, or if I should wait.

Open to suggestions. Even if I had put it back to a draft, you'll still get those notifications as GitHub does not unsubscribe anyone.

Copy link
Member

Choose a reason for hiding this comment

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

yes, but if I see a notification for a draft PR, I can immediately move on, knowing I'll get another notification when it's ready for review, at which time I should look at the diff.

Copy link
Contributor

@jeffkl jeffkl left a comment

Choose a reason for hiding this comment

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

🎉

@donnie-msft donnie-msft merged commit 8a5348f into dev Oct 17, 2024
31 of 33 checks passed
@donnie-msft donnie-msft deleted the dev-donnie-msft-buildToolsUpdate branch October 17, 2024 21:31
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.

Unable to build NuGet.Client VSIX - "GetDeploymentPathFromVsixManifest" task failed unexpectedly
4 participants