-
Notifications
You must be signed in to change notification settings - Fork 4.2k
HotReloadMSBuildWorkspace #81577
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
HotReloadMSBuildWorkspace #81577
Conversation
src/Workspaces/MSBuild/BuildHost/MSBuild/ProjectFile/ProjectInstanceReader.cs
Show resolved
Hide resolved
src/Workspaces/MSBuild/BuildHost/MSBuild/ProjectFile/ProjectInstanceReader.cs
Show resolved
Hide resolved
jasonmalinowski
left a comment
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.
Leaving comments so far; no real concerns so far.
src/Features/ExternalAccess/HotReload/Api/HotReloadMSBuildWorkspace.cs
Outdated
Show resolved
Hide resolved
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.
Just a note to reviewers, this is moving from https://github.com/dotnet/sdk/blob/2b7468dad75a2c8dc419cd30da4443ac6d699aa6/src/BuiltInTools/Watch/HotReload/IncrementalMSBuildWorkspace.cs.
| ImmutableArray<DocumentInfo> MapDocuments(ProjectId mappedProjectId, IReadOnlyList<DocumentInfo> documents) | ||
| => documents.Select(docInfo => | ||
| { | ||
| // TODO: can there be multiple documents of the same path in the project? |
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.
Just to document it somewhere, yes that can happen if a file is added twice. Or is added as both a source file and an additional file. None of these are "real" scenarios, but can happen in broken project scenarios. The only expectation is we don't crash. There might be a bug here since maybe if a source file is being added as an additional file we might try to reuse the same ID which would be bad in this case. Maybe file a bug for tracking, if we refactor this later this code might go away.
jasonmalinowski
left a comment
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.
Fantastic job cleaning this up. Although I imagine there's further refactorings here that will delete some of the code that migrated, I'm much happier to see all the "complexity" of the loading be in Roslyn rather than being in the SDK. That means we can clean it up without cross-cutting changes.
src/Workspaces/MSBuild/BuildHost/MSBuild/ProjectFile/ProjectInstanceReader.cs
Show resolved
Hide resolved
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
Refactors BuildHost and MSBuildProjectLoader to allow dotnet-watch to populate workspace in-proc based on already evaluated ProjectInstances (loaded via ProjectGraph).
ProjectFileinto separate types:XyzProjectCommandLineProvider.ProjectFileInfobased onProjectInstanceinto a separate type:ProjectInstanceReader. The reader takes project instance and optionally MSBuildProjectand extracts the necessary info from its properties and items. TheProjectis optional - it is only needed to populateFileGlobs.HotReloadMSBuildWorkspace- this is a replacement for dotnet-watch's IncrementalMSBuildWorkspaceIProjectFileInfoProviderthat abstract away retrieval ofProjectFileInfos.MSBuildWorkspaceuses an implementation that dispatches to OOP whileHotReloadMSBuildWorkspaceuses impl that calculates the information directly (in-proc) from project instances provided by dotnet-watch.dotnet-watch usage: dotnet/sdk#52163