-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Update Workspace.MSBuild to reference Microsoft.Build.Framework. #78647
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
Changes from 7 commits
a20562c
4921b88
faa5e07
2dc6aa9
b628c33
8c3e512
11ec4fa
92f9599
2e58349
016e4b0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -167,51 +167,20 @@ public async Task<SolutionInfo> LoadSolutionInfoAsync( | |
throw new ArgumentNullException(nameof(solutionFilePath)); | ||
} | ||
|
||
if (!_pathResolver.TryGetAbsoluteSolutionPath(solutionFilePath, baseDirectory: Directory.GetCurrentDirectory(), DiagnosticReportingMode.Throw, out var absoluteSolutionPath)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Moved this bit into a new |
||
{ | ||
// TryGetAbsoluteSolutionPath should throw before we get here. | ||
return null!; | ||
} | ||
|
||
var projectFilter = ImmutableHashSet<string>.Empty; | ||
if (SolutionFilterReader.IsSolutionFilterFilename(absoluteSolutionPath) && | ||
!SolutionFilterReader.TryRead(absoluteSolutionPath, _pathResolver, out absoluteSolutionPath, out projectFilter)) | ||
{ | ||
throw new Exception(string.Format(WorkspaceMSBuildResources.Failed_to_load_solution_filter_0, solutionFilePath)); | ||
} | ||
var (absoluteSolutionPath, projects) = await SolutionFileReader.ReadSolutionFileAsync(solutionFilePath, _pathResolver, cancellationToken).ConfigureAwait(false); | ||
var projectPaths = projects.SelectAsArray(p => p.ProjectPath); | ||
|
||
using (_dataGuard.DisposableWait(cancellationToken)) | ||
{ | ||
this.SetSolutionProperties(absoluteSolutionPath); | ||
SetSolutionProperties(absoluteSolutionPath); | ||
} | ||
|
||
var solutionFile = MSB.Construction.SolutionFile.Parse(absoluteSolutionPath); | ||
var reportingMode = GetReportingModeForUnrecognizedProjects(); | ||
|
||
var reportingOptions = new DiagnosticReportingOptions( | ||
onPathFailure: reportingMode, | ||
onLoaderFailure: reportingMode); | ||
|
||
var projectPaths = ImmutableArray.CreateBuilder<string>(); | ||
|
||
// load all the projects | ||
foreach (var project in solutionFile.ProjectsInOrder) | ||
{ | ||
cancellationToken.ThrowIfCancellationRequested(); | ||
|
||
if (project.ProjectType == MSB.Construction.SolutionProjectType.SolutionFolder) | ||
{ | ||
continue; | ||
} | ||
|
||
// Load project if we have an empty project filter and the project path is present. | ||
if (projectFilter.IsEmpty || | ||
projectFilter.Contains(project.AbsolutePath)) | ||
{ | ||
projectPaths.Add(project.RelativePath); | ||
} | ||
} | ||
|
||
var buildHostProcessManager = new BuildHostProcessManager(Properties, loggerFactory: _loggerFactory); | ||
await using var _ = buildHostProcessManager.ConfigureAwait(false); | ||
|
||
|
@@ -221,7 +190,7 @@ public async Task<SolutionInfo> LoadSolutionInfoAsync( | |
_pathResolver, | ||
_projectFileExtensionRegistry, | ||
buildHostProcessManager, | ||
projectPaths.ToImmutable(), | ||
projectPaths, | ||
// TryGetAbsoluteSolutionPath should not return an invalid path | ||
baseDirectory: Path.GetDirectoryName(absoluteSolutionPath)!, | ||
Properties, | ||
|
@@ -231,14 +200,14 @@ public async Task<SolutionInfo> LoadSolutionInfoAsync( | |
discoveredProjectOptions: reportingOptions, | ||
preferMetadataForReferencesOfDiscoveredProjects: false); | ||
|
||
var projects = await worker.LoadAsync(cancellationToken).ConfigureAwait(false); | ||
var projectInfos = await worker.LoadAsync(cancellationToken).ConfigureAwait(false); | ||
|
||
// construct workspace from loaded project infos | ||
return SolutionInfo.Create( | ||
SolutionId.CreateNewId(debugName: absoluteSolutionPath), | ||
version: default, | ||
absoluteSolutionPath, | ||
projects); | ||
projectInfos); | ||
} | ||
|
||
/// <summary> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,9 +11,9 @@ namespace Microsoft.CodeAnalysis.MSBuild; | |
|
||
internal sealed class PathResolver | ||
{ | ||
private readonly DiagnosticReporter _diagnosticReporter; | ||
private readonly DiagnosticReporter? _diagnosticReporter; | ||
|
||
public PathResolver(DiagnosticReporter diagnosticReporter) | ||
public PathResolver(DiagnosticReporter? diagnosticReporter) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Made the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we be though? |
||
{ | ||
_diagnosticReporter = diagnosticReporter; | ||
} | ||
|
@@ -26,14 +26,14 @@ public bool TryGetAbsoluteSolutionPath(string path, string baseDirectory, Diagno | |
} | ||
catch (Exception) | ||
{ | ||
_diagnosticReporter.Report(reportingMode, string.Format(WorkspacesResources.Invalid_solution_file_path_colon_0, path)); | ||
_diagnosticReporter?.Report(reportingMode, string.Format(WorkspacesResources.Invalid_solution_file_path_colon_0, path)); | ||
absolutePath = null; | ||
return false; | ||
} | ||
|
||
if (!File.Exists(absolutePath)) | ||
{ | ||
_diagnosticReporter.Report( | ||
_diagnosticReporter?.Report( | ||
reportingMode, | ||
string.Format(WorkspacesResources.Solution_file_not_found_colon_0, absolutePath), | ||
msg => new FileNotFoundException(msg)); | ||
|
@@ -51,14 +51,14 @@ public bool TryGetAbsoluteProjectPath(string path, string baseDirectory, Diagnos | |
} | ||
catch (Exception) | ||
{ | ||
_diagnosticReporter.Report(reportingMode, string.Format(WorkspacesResources.Invalid_project_file_path_colon_0, path)); | ||
_diagnosticReporter?.Report(reportingMode, string.Format(WorkspacesResources.Invalid_project_file_path_colon_0, path)); | ||
absolutePath = null; | ||
return false; | ||
} | ||
|
||
if (!File.Exists(absolutePath)) | ||
{ | ||
_diagnosticReporter.Report( | ||
_diagnosticReporter?.Report( | ||
reportingMode, | ||
string.Format(WorkspacesResources.Project_file_not_found_colon_0, absolutePath), | ||
msg => new FileNotFoundException(msg)); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,79 @@ | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT license. | ||
// See the LICENSE file in the project root for more information. | ||
|
||
using System; | ||
using System.Collections.Immutable; | ||
using System.IO; | ||
using System.Threading; | ||
using System.Threading.Tasks; | ||
using Microsoft.VisualStudio.SolutionPersistence.Serializer; | ||
using Roslyn.Utilities; | ||
|
||
namespace Microsoft.CodeAnalysis.MSBuild; | ||
|
||
internal partial class SolutionFileReader | ||
{ | ||
public static Task<(string AbsoluteSolutionPath, ImmutableArray<(string ProjectPath, string ProjectGuid)> Projects)> ReadSolutionFileAsync(string solutionFilePath, CancellationToken cancellationToken) | ||
{ | ||
return ReadSolutionFileAsync(solutionFilePath, new PathResolver(diagnosticReporter: null), cancellationToken); | ||
} | ||
|
||
public static async Task<(string AbsoluteSolutionPath, ImmutableArray<(string ProjectPath, string ProjectGuid)> Projects)> ReadSolutionFileAsync(string solutionFilePath, PathResolver pathResolver, CancellationToken cancellationToken) | ||
{ | ||
if (!pathResolver.TryGetAbsoluteSolutionPath(solutionFilePath, baseDirectory: Directory.GetCurrentDirectory(), DiagnosticReportingMode.Throw, out var absoluteSolutionPath)) | ||
{ | ||
// TryGetAbsoluteSolutionPath should throw before we get here. | ||
JoeRobich marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
return (solutionFilePath, []); | ||
} | ||
|
||
// When passed a solution filter, we need to read the filter file to get the solution path and included project paths. | ||
var projectFilter = ImmutableHashSet<string>.Empty; | ||
if (SolutionFilterReader.IsSolutionFilterFilename(absoluteSolutionPath) && | ||
!SolutionFilterReader.TryRead(absoluteSolutionPath, pathResolver, out absoluteSolutionPath, out projectFilter)) | ||
{ | ||
throw new Exception(string.Format(WorkspaceMSBuildResources.Failed_to_load_solution_filter_0, solutionFilePath)); | ||
} | ||
|
||
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)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
} | ||
|
||
return (absoluteSolutionPath, projects.Value); | ||
} | ||
|
||
private static async Task<ImmutableArray<(string ProjectPath, string ProjectGuid)>?> TryReadSolutionFileAsync(string solutionFilePath, PathResolver pathResolver, ImmutableHashSet<string> projectFilter, CancellationToken cancellationToken) | ||
{ | ||
var serializer = SolutionSerializers.GetSerializerByMoniker(solutionFilePath); | ||
if (serializer == null) | ||
{ | ||
return null; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what does the serializer being null here mean? Are there different serializers for sln vs slnx? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes! And I imagine when solution filter support is add it will be a separate serializer. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?) |
||
} | ||
|
||
// The solution folder is the base directory for project paths. | ||
var baseDirectory = Path.GetDirectoryName(solutionFilePath); | ||
RoslynDebug.AssertNotNull(baseDirectory); | ||
|
||
var solutionModel = await serializer.OpenAsync(solutionFilePath, cancellationToken).ConfigureAwait(false); | ||
|
||
var builder = ImmutableArray.CreateBuilder<(string ProjectPath, string ProjectGuid)>(); | ||
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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you have an empty filter, what happens? Should this use a nullable set to make it more clear? |
||
{ | ||
if (!pathResolver.TryGetAbsoluteProjectPath(projectModel.FilePath, baseDirectory, DiagnosticReportingMode.Throw, out var absoluteProjectPath) | ||
JoeRobich marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|| !projectFilter.Contains(absoluteProjectPath)) | ||
{ | ||
continue; | ||
} | ||
} | ||
|
||
builder.Add((projectModel.FilePath, projectModel.Id.ToString())); | ||
jasonmalinowski marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
return builder.ToImmutable(); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,10 +22,11 @@ | |
</PackageDescription> | ||
</PropertyGroup> | ||
<ItemGroup> | ||
<PackageReference Include="Microsoft.Build" /> | ||
<PackageReference Include="Microsoft.Build.Tasks.Core" /> | ||
<PackageReference Include="Microsoft.Build.Framework" /> | ||
<PackageReference Include="Microsoft.Extensions.Logging" /> | ||
<PackageReference Include="Microsoft.VisualStudio.SolutionPersistence" /> | ||
<PackageReference Include="Newtonsoft.Json" /> | ||
<PackageReference Include="System.Text.Json" Condition="'$(TargetFramework)' == 'net472'" /> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Originally brought in by There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why do we only need this for net472? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. System.Text.Json is part of the .NET BCL. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will add all that as a comment. |
||
</ItemGroup> | ||
<ItemGroup> | ||
<ProjectReference Include="..\..\Core\Portable\Microsoft.CodeAnalysis.Workspaces.csproj" /> | ||
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
In the case that
solutionFilePath
points to a .slnfReadSolutionFileAsync
returns the filtered solution's file path. Should we be assigning that path to theProjectFactory.SolutionPath
?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.
Are you basically asking whether to use the path to the
slnf
or the path to the associatedsln
? I think we would want to use theslnf
, which this PR appears to be doing right now.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.
True, it is working today without this change and is currently using the .slnf path.
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.
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.
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.
(if that's bad, maybe the fix is we should be passing around the workspace path or something else for that...)