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

make the workload resolver only light up for the specific entrypoint SDKs we control #41268

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

baronfel
Copy link
Member

This is part of #26009

This makes NuGet/Home#13471 easier to diagnose because it removes the workload resolver from the list of resolvers - you can see the before and after here:

before:

D:\temp\test01\test01.csproj : error : Could not resolve SDK "MSTest.Sdk". Exactly one of the probing messages below indicates why we could not resolve the SDK. Investigate and resolve that message to correctly specify the SDK.
D:\temp\test01\test01.csproj : error :   SDK resolver "Microsoft.DotNet.MSBuildWorkloadSdkResolver" returned null.
D:\temp\test01\test01.csproj : error :   Unable to resolve 'MSTest.Sdk (= 3.3.1)' for '.NETStandard,Version=v0.0'. PackageSourceMapping is enabled, the following source(s) were not considered: Microsoft Visual Studio Offline Packages, NuGet official package source.
D:\temp\test01\test01.csproj : error MSB4236: The SDK 'MSTest.Sdk' specified could not be found.

after:

E:\Code\Scratch\test-app\test-app.csproj : error : Could not resolve SDK "MSTest.Sdk". Exactly one of the probing messages below indicates why we could not resolve the SDK. Investigate and resolve that message to correctly specify the SDK.
E:\Code\Scratch\test-app\test-app.csproj : error :   Unable to resolve 'MSTest.Sdk (= 3.3.1)' for '.NETStandard,Version=v0.0'. PackageSourceMapping is enabled, the following source(s) were not considered: nuget.
E:\Code\Scratch\test-app\test-app.csproj : error MSB4236: The SDK 'MSTest.Sdk/3.3.1' specified could not be found.

We save one line here, and with additional work on the MSBuild side we may be able to reduce the MSBuild resolver wrapper noise in this message.

@dsplaisted
Copy link
Member

The workload resolver also resolves imports from workload packs, using the pack ID. So the pattern as is is insufficient. It needs to cover things like Microsoft.Android.Sdk.net9, Microsoft.NETCore.App.Runtime.AOT.Cross.net6.android-x86, and any other workload pack that has targets in it (normally these are imported via the WorkloadManifest.targets files).

@baronfel baronfel force-pushed the ResolvableSdkPattern-for-workloadresolver branch from 8618076 to 3a60947 Compare December 4, 2024 16:07
@baronfel
Copy link
Member Author

baronfel commented Dec 4, 2024

Thanks @dsplaisted - I checked on GitHub and made some regex patterns that would match all props/targets imports from the packs in the WorkloadManifests.targets that I found there. They are intentionally broad to help reduce the potential for missing something.

Copy link
Member

@dsplaisted dsplaisted left a comment

Choose a reason for hiding this comment

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

Do you plan to try to do Microsoft.DotNet.MSBuildSdkResolver also? That could improve the perf and experience in Visual Studio and Full Framework MSBuild.

@@ -1,3 +1,4 @@
<SdkResolver>
<Path>..\..\Microsoft.NET.Sdk.WorkloadMSBuildSdkResolver.dll</Path>
<ResolvableSdkPattern>Microsoft\.NET\.SDK\.WorkloadAutoImportPropsLocator|Microsoft\.NET\.SDK\.WorkloadManifestTargetsLocator|Microsoft\..*|Samsung\..*|GtkSharp\..*</ResolvableSdkPattern>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<ResolvableSdkPattern>Microsoft\.NET\.SDK\.WorkloadAutoImportPropsLocator|Microsoft\.NET\.SDK\.WorkloadManifestTargetsLocator|Microsoft\..*|Samsung\..*|GtkSharp\..*</ResolvableSdkPattern>
<ResolvableSdkPattern>Microsoft\..*|Samsung\..*|GtkSharp\..*</ResolvableSdkPattern>

So it looks like anything starting with Microsoft., Samsung., or GtkSharp. will match. Given that, I don't think we need to special case the WorkloadAutoImportPropsLocator or WorkloadManifestTargetsLocator since they both start with Microsoft.

Did you look at all the packs from all the manifests (including Tizen) to make sure that they all match these patterns?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call, I can remove the specific matches. These broad patterns are sufficient to cover all the packs I could find on github - so there is a risk that there's some workload that isn't OSS and is broadly used that these patterns wouldn't cover.

@baronfel
Copy link
Member Author

baronfel commented Dec 4, 2024

Do you plan to try to do Microsoft.DotNet.MSBuildSdkResolver also

I certainly can - it should be pretty simple to do.

@baronfel
Copy link
Member Author

baronfel commented Dec 4, 2024

@dsplaisted is the Microsoft.DotNet.MSBuildSdkResolver used by the .NET CLI, or just by VS? I was looking at it and it only targets net472? If not, what's used in the CLI to resolve the 'base' SDKs?

@@ -37,6 +37,7 @@
<ItemGroup>
<ManifestLines Include="&lt;SdkResolver&gt;" />
<ManifestLines Include="&lt;Path&gt;$(_TargetPathRelativePath)&lt;/Path&gt;" />
<ManifestLines Include="&lt;ResolvableSdkPattern&gt;FSharp\.NET\.Sdk|Microsoft\.Build\.Tasks\.Git|Microsoft\.Docker\.Sdk|Microsoft\.NET\.Sdk|Microsoft\.NET\.Sdk\.BlazorWebAssembly|Microsoft\.NET\.Sdk\.Publish|Microsoft\.NET\.Sdk\.Razor|Microsoft\.NET\.Sdk\.StaticWebAssets|Microsoft\.NET\.Sdk\.Web|Microsoft\.NET\.Sdk\.Web\.ProjectSystem|Microsoft\.NET\.Sdk\.WebAssembly|Microsoft\.NET\.Sdk\.WindowsDesktop|Microsoft\.NET\.Sdk\.Worker|Microsoft\.SourceLink\.AzureRepos\.Git|Microsoft\.SourceLink\.Bitbucket\.Git|Microsoft\.SourceLink\.Common|Microsoft\.SourceLink\.GitHub|Microsoft\.SourceLink\.GitLab|NuGet\.Build\.Tasks\.Pack&lt;/ResolvableSdkPattern&gt;" />
Copy link
Member Author

Choose a reason for hiding this comment

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

This is every SDK that's included in-box today, matched explicitly for ease of review.

Copy link
Member

Choose a reason for hiding this comment

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

All of them except FSharp.NET.Sdk and NuGet.Build.Tasks.Pack start with Microsoft. anyway. Do you think it's worth listing all those separately? It also means there will be more regexes to match.

@@ -45,6 +45,7 @@
<ItemGroup>
<ManifestLines Include="&lt;SdkResolver&gt;" />
<ManifestLines Include="&lt;Path&gt;$(_TargetPathRelativePath)&lt;/Path&gt;" />
<ManifestLines Include="&lt;ResolvableSdkPattern&gt;Microsoft\..*|Samsung\..*|GtkSharp\..*&lt;/ResolvableSdkPattern&gt;" />
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the content from the xml, just replicated in this 'generation' step for the .NET framework version of this resolver.

@dsplaisted
Copy link
Member

If not, what's used in the CLI to resolve the 'base' SDKs?

I think that happens in MSBuild here: https://github.com/dotnet/msbuild/blob/main/src/Build/BackEnd/Components/SdkResolution/DefaultSdkResolver.cs

This works because with a .NET SDK install, MSBuild is already part of it so you can use a relative path from MSBuild instead of having to resolve an SDK version.

@@ -37,6 +37,7 @@
<ItemGroup>
<ManifestLines Include="&lt;SdkResolver&gt;" />
<ManifestLines Include="&lt;Path&gt;$(_TargetPathRelativePath)&lt;/Path&gt;" />
<ManifestLines Include="&lt;ResolvableSdkPattern&gt;FSharp\.NET\.Sdk|Microsoft\.Build\.Tasks\.Git|Microsoft\.Docker\.Sdk|Microsoft\.NET\.Sdk|Microsoft\.NET\.Sdk\.BlazorWebAssembly|Microsoft\.NET\.Sdk\.Publish|Microsoft\.NET\.Sdk\.Razor|Microsoft\.NET\.Sdk\.StaticWebAssets|Microsoft\.NET\.Sdk\.Web|Microsoft\.NET\.Sdk\.Web\.ProjectSystem|Microsoft\.NET\.Sdk\.WebAssembly|Microsoft\.NET\.Sdk\.WindowsDesktop|Microsoft\.NET\.Sdk\.Worker|Microsoft\.SourceLink\.AzureRepos\.Git|Microsoft\.SourceLink\.Bitbucket\.Git|Microsoft\.SourceLink\.Common|Microsoft\.SourceLink\.GitHub|Microsoft\.SourceLink\.GitLab|NuGet\.Build\.Tasks\.Pack&lt;/ResolvableSdkPattern&gt;" />
Copy link
Member

Choose a reason for hiding this comment

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

All of them except FSharp.NET.Sdk and NuGet.Build.Tasks.Pack start with Microsoft. anyway. Do you think it's worth listing all those separately? It also means there will be more regexes to match.

@dsplaisted
Copy link
Member

Looks like there are a bunch of Full Framework test failures, maybe something is off with the filter for that resolver.

@baronfel
Copy link
Member Author

baronfel commented Dec 5, 2024

If not, what's used in the CLI to resolve the 'base' SDKs?

I think that happens in MSBuild here: https://github.com/dotnet/msbuild/blob/main/src/Build/BackEnd/Components/SdkResolution/DefaultSdkResolver.cs

This works because with a .NET SDK install, MSBuild is already part of it so you can use a relative path from MSBuild instead of having to resolve an SDK version.

Hilariously, after I asked this question Rainer and I spent 45m talking about the additions to the sdk resolvers required for dotnet/msbuild#11086 so I learned the answer that way 🗡️

Looks like there are a bunch of Full Framework test failures, maybe something is off with the filter for that resolver.

I'll do a bit of debugging today 👍

@baronfel
Copy link
Member Author

baronfel commented Dec 6, 2024

After digging in a bit, the reason for the failures is that for full framework the MSBuildSdkResolver actually contains all of the different resolvers in a single assembly. The main reason for this is to prevent adding more assembly loads to VS, which regresses performance gates in that product.

What this means for this PR is that

  • the netfx resolver XML file needs to have the combined set of patterns in it (which should be just <ResolvableSdkPattern>Microsoft\..*|Samsung\..*|GtkSharp\..*|FSharp\.NET\.Sdk|NuGet\.Build\.Tasks\.Pack</ResolvableSdkPattern>)
  • this needs to be set in the MSBuild customizations and the static file
  • I need to figure out how the xml static file would get bundled into the VS component - likely will need help from @joeloff on that once I get there.

@@ -56,6 +56,7 @@
<VSMSBuildExtensionsContent Include="$(ArtifactsBinDir)Microsoft.DotNet.MSBuildSdkResolver\$(Configuration)\net472\arm64\hostfxr.dll" DeploymentSubpath="MSBuildSdkResolver/arm64/" />
<VSMSBuildExtensionsContent Include="$(ArtifactsBinDir)Microsoft.DotNet.MSBuildSdkResolver\$(Configuration)\net472\**\Microsoft.Deployment.DotNet.Releases*.dll" DeploymentSubpath="MSBuildSdkResolver/" />
<VSMSBuildExtensionsContent Include="$(ArtifactsBinDir)Microsoft.DotNet.MSBuildSdkResolver\$(Configuration)\net472\**\Microsoft.DotNet.MSBuildSdkResolver*.dll" DeploymentSubpath="MSBuildSdkResolver/" />
<VSMSBuildExtensionsContent Include="$(ArtifactsBinDir)Microsoft.DotNet.MSBuildSdkResolver\$(Configuration)\net472\SdkResolvers\**\Microsoft.DotNet.MSBuildSdkResolver.xml" DeploymentSubpath="MSBuildSdkResolver/" />
Copy link
Member Author

Choose a reason for hiding this comment

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

@joeloff I think this is the right place to add the generated resolver XML to ensure it gets added to the VSIX, but I don't know how to create/inspect the VSIX to verify. Any guidance I can follow here?

Copy link
Member

Choose a reason for hiding this comment

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

The VSIX is just a zip, but it doesn't get produced in the SDK. We only generate the source (.swr file) and VS generates the actual zip, but if you do build -pack, you should end up with a file under artifacts\packages\Debug\NonShipping.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, that's very helpful! I cracked open the SWR and found:

folder "InstallDir:\MSBuild\Current\Bin\SdkResolvers\Microsoft.DotNet.MSBuildSdkResolver\Microsoft.DotNet.MSBuildSdkResolver\"
  file source="$(PkgVS_Redist_Common_Net_Core_SDK_MSBuildExtensions)\MSBuildSdkResolver\Microsoft.DotNet.MSBuildSdkResolver\Microsoft.DotNet.MSBuildSdkResolver.xml"

That to me suggests that I have the layout incorrect because I believe the xml needs to be either adjacent to the dll, or in the SdkResolvers\<ResolverName>\<ResolverName>.xml layout pattern. So I'll need to go fix that.

That's validated by digging into the nupkg as well:
image

Here I can see that the directory structure is incorrect.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Infrastructure untriaged Request triage from a team member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants