Update Workspace.MSBuild to reference Microsoft.Build.Framework. #78647

JoeRobich · 2025-05-19T20:41:14Z

Workspace.MSBuild requires a MSBuild dependency because our API references their ILogger interface. This PR changes the MSBuild references to be Microsoft.Build.Framework, which is where the interface is defined.

In order to fully drop the Microsoft.Build dependency I also pulled in the VS-SolutionPersister changes which bring solution parsing back in-proc.

RikkiGibson · 2025-05-19T20:49:21Z

What is the effect of the change? Is it reducing the net msbuild surface we are depending on / making it harder to inadvertently use msbuild APIs that we should not be using?

JoeRobich · 2025-05-19T20:55:12Z

What is the effect of the change?

Currently our package brings in a broad set of MSBuild dependencies (see below). This change limits it to only the dependency that is necessary.

RikkiGibson · 2025-05-19T21:12:01Z

It looks like possibly a subset of the transitive dependencies need to be brought back to fix the build

… into dev/jorobich/msbuild-reference

Removes the GetProjectsInSolution BuildHost API. Instead we can use the SolutionPersister library in processes to parse the projects from a solution file.

JoeRobich · 2025-05-21T22:14:39Z

...ageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/LanguageServerProjectSystem.cs

-        }
-
-        var projects = await buildHost.GetProjectsInSolutionAsync(solutionFilePath, CancellationToken.None);
+        var (_, projects) = await SolutionFileReader.ReadSolutionFileAsync(solutionFilePath, CancellationToken.None);


In the case that solutionFilePath points to a .slnf ReadSolutionFileAsync returns the filtered solution's file path. Should we be assigning that path to the ProjectFactory.SolutionPath?

Are you basically asking whether to use the path to the slnf or the path to the associated sln? I think we would want to use the slnf, which this PR appears to be doing right now.

True, it is working today without this change and is currently using the .slnf path.

I'd say that's probably reasonable anyways; the only real thing the solution path is used for is caches, and I could imagine what we have to cache is different for different filters over the same solution.

(if that's bad, maybe the fix is we should be passing around the workspace path or something else for that...)

JoeRobich · 2025-05-21T22:16:06Z

src/Workspaces/MSBuild/Core/MSBuild/MSBuildProjectLoader.cs

            throw new ArgumentNullException(nameof(solutionFilePath));
        }

-        if (!_pathResolver.TryGetAbsoluteSolutionPath(solutionFilePath, baseDirectory: Directory.GetCurrentDirectory(), DiagnosticReportingMode.Throw, out var absoluteSolutionPath))


Moved this bit into a new SolutionFileReader class.

JoeRobich · 2025-05-21T22:17:39Z

src/Workspaces/MSBuild/Core/MSBuild/PathResolver.cs

+    private readonly DiagnosticReporter? _diagnosticReporter;

-    public PathResolver(DiagnosticReporter diagnosticReporter)
+    public PathResolver(DiagnosticReporter? diagnosticReporter)


Made the DiagnosticReporter optional since this will also be indirectly used by the LanguageServerProjectSystem which does not use a reporter.

Should we be though?

JoeRobich · 2025-05-21T22:21:39Z

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

    <PackageReference Include="Microsoft.Extensions.Logging" />
+    <PackageReference Include="Microsoft.VisualStudio.SolutionPersistence" />
    <PackageReference Include="Newtonsoft.Json" />
+    <PackageReference Include="System.Text.Json" Condition="'$(TargetFramework)' == 'net472'" />


Originally brought in by Microsoft.Build and is already available as part of .NET.

why do we only need this for net472?

System.Text.Json is part of the .NET BCL.

Having it as a PackageReference for .NET TFM will pin the version to whatever we use in the build instead of allowing it to use the version included with newer runtimes.

I will add all that as a comment.

src/Workspaces/MSBuild/Core/MSBuild/SolutionFileReader.SolutionFilterReader.cs

src/Workspaces/MSBuild/Core/MSBuild/SolutionFileReader.cs

src/Workspaces/MSBuild/Core/Rpc/RemoteBuildHost.cs

src/Workspaces/MSBuild/Test/Resources/XmlSolutionFiles/CSharpXmlSolution.slnx

src/Workspaces/MSBuild/Test/VisualStudioMSBuildWorkspaceTests.cs

dibarbet · 2025-05-22T22:14:03Z

src/Workspaces/MSBuild/Core/MSBuild/SolutionFileReader.cs

+        var serializer = SolutionSerializers.GetSerializerByMoniker(solutionFilePath);
+        if (serializer == null)
+        {
+            return null;


what does the serializer being null here mean? Are there different serializers for sln vs slnx?

Yes! And I imagine when solution filter support is add it will be a separate serializer.

dibarbet · 2025-05-22T22:16:24Z

src/Workspaces/MSBuild/Core/MSBuild/SolutionFileReader.cs

+            if (!projectFilter.IsEmpty)
+            {
+                Contract.ThrowIfFalse(pathResolver.TryGetAbsoluteProjectPath(projectModel.FilePath, baseDirectory, DiagnosticReportingMode.Throw, out var absoluteProjectPath));
+                if (!projectFilter.Contains(absoluteProjectPath))


interesting, does the solution parsing library not give us a way to get the subset of projects out of a solution filter? We have to do that ourselves?

Not presently. However, there is an open feature request microsoft/vs-solutionpersistence#85

dibarbet · 2025-05-22T22:16:58Z

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

    <PackageReference Include="Microsoft.Extensions.Logging" />
+    <PackageReference Include="Microsoft.VisualStudio.SolutionPersistence" />
    <PackageReference Include="Newtonsoft.Json" />
+    <PackageReference Include="System.Text.Json" Condition="'$(TargetFramework)' == 'net472'" />


why do we only need this for net472?

jasonmalinowski · 2025-05-28T23:38:47Z

src/Workspaces/MSBuild/Core/MSBuild/SolutionFileReader.cs

+
+    public static async Task<(string AbsoluteSolutionPath, ImmutableArray<(string ProjectPath, string ProjectGuid)> Projects)> ReadSolutionFileAsync(string solutionFilePath, PathResolver pathResolver, CancellationToken cancellationToken)
+    {
+        Contract.ThrowIfFalse(pathResolver.TryGetAbsoluteSolutionPath(solutionFilePath, baseDirectory: Directory.GetCurrentDirectory(), DiagnosticReportingMode.Throw, out var absoluteSolutionPath));


I'd expect this Directory.GetCurrentDirectory() code is only used in the classic MSBuildWorkspace path; we shouldn't have relative paths bouncing around in the language server at all, and I'd actually be a bit worried what it might resolve to at all...

jasonmalinowski · 2025-05-28T23:41:04Z

src/Workspaces/MSBuild/Core/MSBuild/SolutionFileReader.cs

+        var projects = await TryReadSolutionFileAsync(absoluteSolutionPath, pathResolver, projectFilter, cancellationToken).ConfigureAwait(false);
+        if (!projects.HasValue)
+        {
+            throw new Exception(string.Format(WorkspaceMSBuildResources.Failed_to_load_solution_0, absoluteSolutionPath));


Is the only case this can happen is the format of the solution file isn't recognized? Should we give a better message in this case?

jasonmalinowski · 2025-05-28T23:41:37Z

src/Workspaces/MSBuild/Core/MSBuild/SolutionFileReader.cs

+        var serializer = SolutionSerializers.GetSerializerByMoniker(solutionFilePath);
+        if (serializer == null)
+        {
+            return null;


Should we have this just throw? Otherwise we're having to deal with the null return at the caller which isn't really helping much since it makes me think the rest of the method isn't supposed to throw (but absolutely would if something went wrong?)

jasonmalinowski · 2025-05-28T23:42:04Z

src/Workspaces/MSBuild/Core/MSBuild/SolutionFileReader.cs

+        foreach (var projectModel in solutionModel.SolutionProjects)
+        {
+            // If we are filtering based on a solution filter, then we need to verify the project is included.
+            if (!projectFilter.IsEmpty)


If you have an empty filter, what happens? Should this use a nullable set to make it more clear?

src/Workspaces/MSBuild/Core/MSBuild/SolutionFileReader.cs

jasonmalinowski · 2025-05-28T23:43:44Z

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

+      Since System.Text.Json is part of the .NET BCL we do not want to add
+      it as a package reference. Doing so will pin the version to what we use
+      in our build instead of allowing it to use the version from the .NET Runtime
+      we are running against.


Is System.Text.Json extra special here, or should we be doing this for other references too?

jasonmalinowski · 2025-05-28T23:44:56Z

src/Workspaces/MSBuild/Core/MSBuild/SolutionFileReader.SolutionFilterReader.cs

@@ -35,13 +35,7 @@ public static bool TryRead(string filterFilename, PathResolver pathResolver, [No
                    return false;
                }

-                if (!pathResolver.TryGetAbsoluteSolutionPath(solutionPath, baseDirectory, DiagnosticReportingMode.Throw, out solutionFilename))
-                {
-                    // TryGetAbsoluteSolutionPath should throw before we get here.


Might be good to bring this comment back -- since if you're not aware of the DiagnosticReportingMode bit this might seem a bit odd...

johnkors · 2025-06-04T12:46:00Z

Can someone tell me how to read a slnx-file programatically after this change?

Using Microsoft.CodeAnalysis.CSharp.Workspaces/4.14.0 &

await workspace.OpenSolutionAsync("some.slnx");

still throws exceptions.

Is there another way?

deepakrathore33 and others added 3 commits February 24, 2025 20:59

Add XML Solution (SLNX) support to MSBuildWorkspace

a20562c

Merge branch 'main' into slnx-support

4921b88

Update Workspace.MSBuild to reference Microsoft.Build.Framework.

faa5e07

JoeRobich requested a review from a team as a code owner May 19, 2025 20:41

github-actions bot added the Area-IDE label May 19, 2025

JoeRobich added 2 commits May 19, 2025 21:42

Merge commit 'refs/pull/77326/head' of https://github.com/dotnet/roslyn…

2dc6aa9

… into dev/jorobich/msbuild-reference

Use SolutionFileReader instead of calling out to BuildHost

b628c33

Removes the GetProjectsInSolution BuildHost API. Instead we can use the SolutionPersister library in processes to parse the projects from a solution file.

JoeRobich requested a review from a team as a code owner May 20, 2025 06:33

jasonmalinowski self-requested a review May 20, 2025 06:35

dotnet-policy-service bot added the VSCode label May 20, 2025

JoeRobich added 2 commits May 21, 2025 07:55

Make solution file reading async

8c3e512

Cleanup

11ec4fa

JoeRobich commented May 21, 2025

View reviewed changes

RikkiGibson self-assigned this May 21, 2025

JoeRobich commented May 21, 2025

View reviewed changes

RikkiGibson reviewed May 21, 2025

View reviewed changes

src/Workspaces/MSBuild/Core/MSBuild/SolutionFileReader.SolutionFilterReader.cs Show resolved Hide resolved

RikkiGibson reviewed May 21, 2025

View reviewed changes

src/Workspaces/MSBuild/Core/MSBuild/SolutionFileReader.cs Outdated Show resolved Hide resolved

RikkiGibson reviewed May 21, 2025

View reviewed changes

src/Workspaces/MSBuild/Core/MSBuild/SolutionFileReader.cs Outdated Show resolved Hide resolved

RikkiGibson reviewed May 21, 2025

View reviewed changes

src/Workspaces/MSBuild/Core/Rpc/RemoteBuildHost.cs Show resolved Hide resolved

RikkiGibson reviewed May 21, 2025

View reviewed changes

src/Workspaces/MSBuild/Test/Resources/XmlSolutionFiles/CSharpXmlSolution.slnx Show resolved Hide resolved

RikkiGibson reviewed May 21, 2025

View reviewed changes

src/Workspaces/MSBuild/Test/VisualStudioMSBuildWorkspaceTests.cs Show resolved Hide resolved

RikkiGibson approved these changes May 21, 2025

View reviewed changes

JoeRobich added 2 commits May 21, 2025 15:56

Use ThrowIfFalse when resolving paths

92f9599

Add test assertions

2e58349

RikkiGibson approved these changes May 22, 2025

View reviewed changes

dibarbet reviewed May 22, 2025

View reviewed changes

Add comment

016e4b0

dibarbet approved these changes May 22, 2025

View reviewed changes

JoeRobich merged commit 4d032fb into main May 23, 2025
28 checks passed

dotnet-policy-service bot added this to the Next milestone May 23, 2025

JoeRobich mentioned this pull request May 23, 2025

MSBuildWorkspace.OpenSolutionAsync("My.slnx") #78097

Closed

dotnet-bot mentioned this pull request May 25, 2025

[Automated] PRs inserted in VS build main-10624.04 #78709

Closed

jasonmalinowski reviewed May 28, 2025

View reviewed changes

This was referenced May 30, 2025

[Automated] PRs inserted in VS build feature.debugger.main-10629.142 #78767

Closed

[Automated] PRs inserted in VS build feature.telemetry.d18OpenTelemetry-10630.96 #78782

Closed

[Automated] PRs inserted in VS build feature.Wix5-10703.29 #78812

Closed

JoeRobich mentioned this pull request Jun 4, 2025

Should Microsoft.CodeAnalysis.Workspaces.MSBuild be exposing MSBuild references? #75058

Closed

This was referenced Jun 13, 2025

Stop Format redistributing MSBuild dotnet/sdk#49065

Open

Add support for .slnx files dotnet/format#2225

Closed

JoeRobich deleted the dev/jorobich/msbuild-reference branch July 15, 2025 02:27

RikkiGibson modified the milestones: Next, 18.0 P1 Aug 19, 2025

Update Workspace.MSBuild to reference Microsoft.Build.Framework. #78647

Update Workspace.MSBuild to reference Microsoft.Build.Framework. #78647

Uh oh!

Conversation

JoeRobich commented May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RikkiGibson commented May 19, 2025

Uh oh!

JoeRobich commented May 19, 2025

Uh oh!

RikkiGibson commented May 19, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoeRobich May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnkors commented Jun 4, 2025

Uh oh!

Uh oh!

JoeRobich commented May 19, 2025 •

edited

Loading

JoeRobich May 21, 2025 •

edited

Loading