-
Notifications
You must be signed in to change notification settings - Fork 91
Make the MSBuild runtime assembly check more correct #360
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
Conversation
| @@ -1,25 +1,24 @@ | |||
| <Project xmlns="http://schemas.microsoft.com/developer/msbuild/2003"> | |||
| <Target Name="EnsureMSBuildAssembliesNotCopied" AfterTargets="Build" Condition="'$(DisableMSBuildAssemblyCopyCheck)' != 'true'"> | |||
| <Target Name="EnsureMSBuildAssembliesNotCopied" AfterTargets="ResolvePackageAssets" Condition="'$(DisableMSBuildAssemblyCopyCheck)' != 'true'"> | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ResolvePackageAssets creates RuntimeCopyLocalItems for all dependencies, so we have to filter those down to just the ones that came from the MSbuild-related dependencies we want to flag.
| <_MSBuildAssembliesCopyLocalItems Include="@(RuntimeCopyLocalItems->WithMetadataValue('NuGetPackageId', 'Microsoft.Build.Engine'))" /> | ||
| <_MSBuildAssembliesCopyLocalItems Include="@(RuntimeCopyLocalItems->WithMetadataValue('NuGetPackageId', 'Microsoft.NET.StringTools'))" /> | ||
| <_MSBuildAssembliesCopyLocalItems Include="@(RuntimeCopyLocalItems->WithMetadataValue('NuGetPackageId', 'NuGet.Frameworks'))" /> | ||
| <MSBuildAssembliesCopyLocalItems Include="@(_MSBuildAssembliesCopyLocalItems->WithMetadataValue('CopyLocal', 'true')->WithMetadataValue('AssetType', 'runtime'))" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once we have the candidate set, we can find just the ones that are runtime assets and were copylocal'd.
| <_MSBuildAssembliesCopyLocalItems Include="@(RuntimeCopyLocalItems->WithMetadataValue('NuGetPackageId', 'Microsoft.NET.StringTools'))" /> | ||
| <_MSBuildAssembliesCopyLocalItems Include="@(RuntimeCopyLocalItems->WithMetadataValue('NuGetPackageId', 'NuGet.Frameworks'))" /> | ||
| <MSBuildAssembliesCopyLocalItems Include="@(_MSBuildAssembliesCopyLocalItems->WithMetadataValue('CopyLocal', 'true')->WithMetadataValue('AssetType', 'runtime'))" /> | ||
| <_DistinctMSBuildPackagesReferencedPoorly Include="@(MSBuildAssembliesCopyLocalItems->Metadata('NuGetPackageId')->Distinct())" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
such assets will have the nuget package id and version metadata on them, so we can distinct the IDs easily and make nice error messages.
…code + link to enable searching
While debugging test failures in CommunityToolkit/Aspire#949 with @aaronpowell we discovered that the Microsoft.SqlServer.DacFx package includes a dependency on Microsoft.Build that seems to be causing MSbuild runtime dlls to end up in the test project!
This is very likely an authoring error that we should track down with them, but what surprised me was that the check for the RuntimeAssets=exclude provided by this library didn't flag this. On further investigation I discovered two gaps in the check:
The Aspire scenario is
To fix this, we need to do two things
buildTransitivecheckGiven this lib (
dotnet new classlib):and this test project referencing the lib (
dotnet new mstest):Building the
testproject does now trigger our error:While in here, I also added a small README for the package so that the NuGet.org experience is nicer.