Migrate all NuGet.Client projects to SDK style #5167
Conversation
| @@ -0,0 +1,4 @@ | |||
| <Project> | |||
| <Import Project="$([MSBuild]::GetPathOfFileAbove('Directory.Build.targets', '$(MSBuildThisFileDirectory)../'))" /> | |||
| <Import Project="$(PkgMicrosoft_VSSDK_BuildTools)\tools\VSSDK\Microsoft.VsSDK.targets" Condition="'$(PkgMicrosoft_VSSDK_BuildTools)' != ''" /> | |||
There was a problem hiding this comment.
The Microsoft.VsSDK.targets file needs to be imported AFTER all the .NET/msbuild common targets are imported. Since the project file uses <Project Sdk="Microsoft.NET.Sdk>, the project file no longer explicitly imports the targets in the file, allowing us to add extra MSBuild script after the targets import.
However, MSBuild allows you to explicitly import the SDK's props and targets files in the project file. If we change to this syntax, we shouldn't need to add these Directory.Build.targets files. What do people think?
For what it's worth, I also tried moving this import to build/common.targets, but for reasons I don't understand, it caused two resources files in the NuGet.Tools project to have the same output path, which failed the build.
There was a problem hiding this comment.
I think this more elegant, I'm hoping they fix it in the future too so its even easier to have and SDK-style project for VSIX.
Here's the alternative in action for others to compare: https://github.com/microsoft/slngen/blob/main/src/Microsoft.VisualStudio.SlnGen.Extension/Microsoft.VisualStudio.SlnGen.Extension.csproj#L49-L50
| @@ -14,7 +14,7 @@ | |||
| </PropertyGroup> | |||
|
|
|||
| <PropertyGroup> | |||
| <ReferenceOutputPath>$(ArtifactsDirectory)NuGet.VisualStudio.Client\bin\$(Configuration)\</ReferenceOutputPath> | |||
| <ReferenceOutputPath>$(ArtifactsDirectory)NuGet.VisualStudio.Client\bin\$(Configuration)\net472\</ReferenceOutputPath> | |||
There was a problem hiding this comment.
An alternative would be to set <AppendTargetFrameworkToOutputPath>false</AppendTargetFrameworkToOutputPath> in NuGet.VisualStudio.Client.csproj since it won't ever multi-target.
| <Compile Include="Properties\AssemblyInfo.cs" /> | ||
| </ItemGroup> | ||
| <ItemGroup> | ||
| <EmbeddedResource Include="Resources.resx"> |
There was a problem hiding this comment.
SDK-style projects usually need an <EmeddedResource Update="" /> to make things look right in VS.
| @@ -0,0 +1,4 @@ | |||
| <Project> | |||
| <Import Project="$([MSBuild]::GetPathOfFileAbove('Directory.Build.targets', '$(MSBuildThisFileDirectory)../'))" /> | |||
| <Import Project="$(PkgMicrosoft_VSSDK_BuildTools)\tools\VSSDK\Microsoft.VsSDK.targets" Condition="'$(PkgMicrosoft_VSSDK_BuildTools)' != ''" /> | |||
There was a problem hiding this comment.
I think this more elegant, I'm hoping they fix it in the future too so its even easier to have and SDK-style project for VSIX.
Here's the alternative in action for others to compare: https://github.com/microsoft/slngen/blob/main/src/Microsoft.VisualStudio.SlnGen.Extension/Microsoft.VisualStudio.SlnGen.Extension.csproj#L49-L50
src/NuGet.Clients/NuGet.VisualStudio.Client/NuGet.VisualStudio.Client.csproj
Show resolved
Hide resolved
src/NuGet.Clients/NuGet.VisualStudio.Client/NuGet.VisualStudio.Client.csproj
Outdated
Show resolved
Hide resolved
Bug
Fixes: https://github.com/NuGet/Client.Engineering/issues/273
Regression? No
Description
Migrate all non-SDK style projects in the solution to SDK style. VS still providers designers for WPF and WinForms, so I don't believe there's any downside to this. I tested that code changes and F5 debug into VS experimental instance still works. The solution can now be restored and evaluated with the
dotnetCLI, but the VSSDK.BuildTools tasks only run on msbuild, so we can't build with the dotnet CLI.The changes are mostly "simple" conversions, but there's a few non-trivial things that needed to be done:
<Name>metadata on all the<ProjectReferenceitems that are defined in the vsixmanifest file, because the VSSDK BuildTools needs it. We also need all the ngen metadata. We can convert the syntax to XML attributes rather than child elements, but I don't find either syntax better or worse than the other.PR Checklist
PR has a meaningful title
PR has a linked issue.
Described changes
Tests
Documentation