Microsoft.Build.Locator excluded from MSBuild.BuildHost.deps.json file when building with .NET 10 SDK

[mthalman]

The source built SDK for .NET 10 currently doesn't have a functional dotnet-format tool. Attempting to run it on a project results in the following error:

Unhandled exception: Microsoft.CodeAnalysis.MSBuild.RemoteInvocationException: An exception of type System.IO.FileNotFoundException was thrown: Could not load file or assembly 'Microsoft.Build.Locator, Version=1.6.10.0, Culture=neutral, PublicKeyToken=9dff12846e04bfbd'. The system cannot find the file specified.


at Microsoft.CodeAnalysis.MSBuild.RpcClient.InvokeCoreAsync(Int32 targetObject, String methodName, List1 parameters, Type expectedReturnType, CancellationToken cancellationToken)    at Microsoft.CodeAnalysis.MSBuild.RpcClient.InvokeAsync[T](Int32 targetObject, String methodName, List1 parameters, CancellationToken cancellationToken)
at Microsoft.CodeAnalysis.MSBuild.RemoteBuildHost.LoadProjectFileAsync(String projectFilePath, String languageName, CancellationToken cancellationToken)
at Microsoft.CodeAnalysis.MSBuild.MSBuildProjectLoader.Worker.DoOperationAndReportProgressAsync[TResult](ProjectLoadOperation operation, String projectPath, String targetFramework, Func1 doFunc)    at Microsoft.CodeAnalysis.MSBuild.MSBuildProjectLoader.Worker.LoadProjectFileInfosAsync(String projectPath, DiagnosticReportingOptions reportingOptions, CancellationToken cancellationToken)    at Microsoft.CodeAnalysis.MSBuild.MSBuildProjectLoader.Worker.LoadProjectInfosFromPathAsync(String projectPath, DiagnosticReportingOptions reportingOptions, CancellationToken cancellationToken)    at Microsoft.CodeAnalysis.MSBuild.MSBuildProjectLoader.Worker.LoadAsync(CancellationToken cancellationToken)    at Microsoft.CodeAnalysis.MSBuild.MSBuildProjectLoader.LoadProjectInfoAsync(String projectFilePath, ProjectMap projectMap, IProgress1 progress, ILogger msbuildLogger, CancellationToken cancellationToken)
at Microsoft.CodeAnalysis.MSBuild.MSBuildProjectLoader.LoadProjectInfoAsync(String projectFilePath, ProjectMap projectMap, IProgress1 progress, ILogger msbuildLogger, CancellationToken cancellationToken)    at Microsoft.CodeAnalysis.MSBuild.MSBuildWorkspace.OpenProjectAsync(String projectFilePath, ILogger msbuildLogger, IProgress1 progress, CancellationToken cancellationToken)
at Microsoft.CodeAnalysis.Tools.Workspaces.MSBuildWorkspaceLoader.LoadAsync(String solutionOrProjectPath, WorkspaceType workspaceType, String binaryLogPath, Boolean logWorkspaceWarnings, ILogger logger, CancellationToken cancellationToken)
at Microsoft.CodeAnalysis.Tools.CodeFormatter.OpenMSBuildWorkspaceAsync(String solutionOrProjectPath, WorkspaceType workspaceType, Boolean noRestore, Boolean requiresSemantics, String binaryLogPath, Boolean logWorkspaceWarnings, ILogger logger, CancellationToken cancellationToken)
at Microsoft.CodeAnalysis.Tools.CodeFormatter.FormatWorkspaceAsync(FormatOptions formatOptions, ILogger logger, CancellationToken cancellationToken, String binaryLogPath)
at Microsoft.CodeAnalysis.Tools.FormatCommandCommon.FormatAsync(FormatOptions formatOptions, ILogger`1 logger, CancellationToken cancellationToken)
at Microsoft.CodeAnalysis.Tools.Commands.RootFormatCommand.FormatCommandDefaultHandler.InvokeAsync(ParseResult parseResult, CancellationToken cancellationToken)
at System.CommandLine.Invocation.InvocationPipeline.InvokeAsync(ParseResult parseResult, CancellationToken cancellationToken)

This was identified in dotnet/sdk#46055 (comment).

This occurs because Microsoft.Build.Locator is not referenced by the sdk/<version>/DotnetTools/dotnet-format/BuildHost-netcore/Microsoft.CodeAnalysis.Workspaces.MSBuild.BuildHost.deps.json file. If you manually update the file to reference Microsoft.Build.Locator, dotnet-format will run successfully.

The reason Microsoft.Build.Locator is not in the deps.json file is because a recent SDK change (dotnet/sdk#45259) combined with the Microsoft.Build.Locator package reference being defined with PrivateAssets="All":

roslyn/src/Workspaces/Core/MSBuild.BuildHost/Microsoft.CodeAnalysis.Workspaces.MSBuild.BuildHost.csproj

Line 45 in c2b719c

<PackageReference Include="Microsoft.Build.Locator" PrivateAssets="All" />

This doesn't currently affect the Microsoft-built SDK because that is using assets produced by Roslyn's build which is currently using the .NET 9 SDK. So it will eventually be an issue for the Microsoft-built SDK whenever roslyn updates its global.json to use a .NET 10 SDK. However, this needs to be addressed now because source build builds all repos with the .NET 10 SDK and we want to have a usable dotnet-format tool for the source built SDK in .NET 10 Preview 1.

[ViktorHofer]

cc @Forgind

[Forgind]

Triage:
Microsoft.Build.Locator is not a development dependency in this case. Can you explain why you marked it as PrivateAssets="all"? We did not expect this to be a breaking change like this, so we're interested in hearing more about your scenario.

Given time is a bit short, is it reasonable to just remove the PrivateAssets="all" metadata?

[mthalman]

cc @tmat

[Forgind]

Further triage:
We're thinking the short-term solution may still be deleting PrivateAssets=all, but we're also concerned that this may break others. We're thinking it would be good to revert the deps.json change (i.e., put M.B.Locator back in this case), then tweak it to add the file to the deps.json if the asset is present.

[tmat]

@jasonmalinowski

[jasonmalinowski]

Let me at least try to explain first what we want PrivateAssets="all" to mean here:

roslyn/src/Workspaces/Core/MSBuild.BuildHost/Microsoft.CodeAnalysis.Workspaces.MSBuild.BuildHost.csproj

Line 45 in c2b719c

<PackageReference Include="Microsoft.Build.Locator" PrivateAssets="All" />

Roslyn's MSBuildWorkspace is an API that you can point at a .csproj or .sln on disk, and we'll use MSBuild behind the covers to run a design time build, collect the stuff the compiler cares about, and give you a Roslyn Compilation that represents it so you can write your own tools to analyze your projects/solution. Because running MSBuild inside the user's application process is fraught with peril, we moved all the MSBuild interacting stuff to a separate project (the BuildHost project), and we deploy copies of that app in the user's app folder as NuGet content; the library they reference then finds that, launches the process and communicates with it, and shuts down the process once it's done.

The BuildHost needs Microsoft.Build.Locator to be deployed in that mini app, and it needs it to be in the deps.json. The BuildHost is (as of the code in main) also consumes the BuildHost as a library, here:

roslyn/src/Workspaces/Core/MSBuild/Microsoft.CodeAnalysis.Workspaces.MSBuild.csproj

Lines 34 to 40 in c2b719c

	<!-- For the following package we directly include the binary into this NuGet package because we don't want/need a separate NuGet package
	for it. PrivateAssets="all" is needed to prevent this reference from becoming a package reference in the package, as a workaround for
	/~https://github.com/NuGet/Home/issues/3891.
	-->
	<ProjectReference Include="..\MSBuild.BuildHost\Microsoft.CodeAnalysis.Workspaces.MSBuild.BuildHost.csproj" PrivateAssets="all" ReferenceOutputAssembly="false">
	<IncludeOutputInThisPackage>true</IncludeOutputInThisPackage>
	</ProjectReference>

The customer is referencing the NuGet package for Microsoft.CodeAnalysis.Workspaces.MSBuild; the user does not get Microsoft.Build.Locator anywhere within their app; everything is hidden away in the BuildHost from them.

Microsoft.Build.Locator having PrivateAssets="all" here trying to say "this app needs this as an asset for compile and runtime, but should not flow as a project reference to any consuming projects, namely Microsoft.CodeAnalysis.Workspaces.MSBuild".

Now that the explanation is over, let me give a few comments for what we should do for the code change on the short term:

1. It is important that the final Workspaces.MSBuild project does not get Microsoft.Build.Locator as a dependency. It's possible the PrivateAssets="true" on the project ref from the library to the BuildHost also has the same effect, and we could remove the problematic PrivateAssets="true" in the BuildHost project, and everything would be fine. Somebody would have to test to see what happens.
2. This question of what to do here might moot the moment Make Workspaces.MSBuild build with a netstandard target #76832 merges, which will probably happen in the next day or two, since it's removing that ProjectReference from the main library to the BuildHost. I see it's not removing the PrivateAssets="true" but it might be something that can be added to that PR.

---

Generally, this raises the question for me of "so what does PrivateAssets=all" actually mean?

My reading of that would mean the use here wasn't bad: we are consuming that reference as a compile-time and run-time asset in that project, but we don't want it to flow further. And looking in Roslyn I can see other PrivateAssets="true" that I'd still expect to mean that the asset is in the deps.json and can be loaded. But I can also see places where I'd imagine the asset wasn't being deployed at all, which makes me think even we don't know what this actually means.

This particular bit of MSBuild/NuGet schenanigans we have in our build here is admittedly quite complex, and is working around a number of SDK issues and missing features. If you're going to break something, this is probably what you'd break. That said, there are a number of other places like these:

roslyn/src/EditorFeatures/CSharpTest/Microsoft.CodeAnalysis.CSharp.EditorFeatures.UnitTests.csproj

Line 46 in c050b7f

<PackageReference Include="Xunit.Combinatorial" PrivateAssets="all" />

Where I would imagine the binary not being in the deps.json might also break unit tests. There's also this other bit of magic in MSBuildWorkspace:

roslyn/src/Workspaces/MSBuild/Core/Microsoft.CodeAnalysis.Workspaces.MSBuild.csproj

Lines 41 to 43 in c050b7f

	<ProjectReference Include="..\..\..\Tools\ExternalAccess\RazorCompiler\Microsoft.CodeAnalysis.ExternalAccess.RazorCompiler.csproj" PrivateAssets="all">
	<IncludeOutputInThisPackage>true</IncludeOutputInThisPackage>
	</ProjectReference>

Where we'd also expect that binary to land in deps.json, and that's not nearly as magic as the other bit. We also have places that use both PrivateAssets="true" and ExcludeAssets="true", which I'd imagine would mean the binary doesn't get deployed at that point, and shouldn't appear in deps.json.

I'm absolutely willing to concede that PrivateAssets="true" should do something different than it was doing (and maybe the new behavior is "better") but I'd say it's definitely a breaking change and should be documented/communicated accordingly. If the answer is roll it back on the short term and do some wider testing, great.

[mthalman]

@Forgind - Can this be closed now due to dotnet/sdk#46182 and dotnet/sdk#46324?

[Forgind]

I'm not sure why we backported to main when it isn't in release/9.0.3xx, and I have a fix-forward PR out there. My inclination would be to wait for that to merge, but I'm not sure it matters that much.

[ViktorHofer]

I'm not sure why we backported to main

See the ".NET 10 Pre-Preview 1" Teams chat offline. Because code complete for .NET 10 P1 is today and this is already effecting multiple repositories negatively. We would have a completely broken product if we would have let this slip through.

[dsplaisted]

@Forgind Same reason we backed out the fix in 9.0.2xx, we need it fixed quickly so we back out the breaking change and then take our time with the right long-term fix.