Reload and update MSBuild projects in response to dotnet restore #1003
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
I recently noticed an issue that has likely been present for a long while. Essentially, metadata references are never removed from the workspace. The reason for this is because Roslyn's MetadataReference class does not implement value equality -- it just inherits the default `Equals()`/`GetHashCode()` implementations from `System.Object`. However, the algorithm for adding and removing metadata references assumes value equality. This is easily fixed by adding our own `IEqualityComparer<MetadataReference>` that first checks to ensure that `MetadataReferences` are `PortableExecutableMetadataReferences` and then uses their file paths for equality.
This change unifies file and directory watching, separates watching and notification concerns in the API, and fixes the following issues: 1. Paths are treated case-insensitively as they are in the rest of OmniSharp. 2. Access to the internal dictionary of callbacks is synchronized.
This is a pretty substantial change that reworks the core logic of the MSBuild project system to enable an important scenario: updating a project when several files change in quick succession. In order to fix an issue with OmniSharp not reloading and updating a project in response to a 'dotnet restore', we must watch four files that might be touched during a restore: * project.asset.json * <project-file>.nuget.cache * <project-file>.nuget.g.props * <project-file>.nuget.g.targets To ensure that we don't reload and update a project multiple times in response to multiple file changes, this PR introduces a simple queue and processing loop using a TPL DataFlow BufferBlock.
filipw
reviewed
Nov 3, 2017
|
|
||
| public PackageDependencyResolver(ILoggerFactory loggerFactory) | ||
| public PackageDependencyChecker(ILoggerFactory loggerFactory, IEventEmitter eventEmitter, DotNetCliService dotNetCli, MSBuildOptions options) |
There was a problem hiding this comment.
when we resolve inline package references (#r "nuget: Foo, 1.0.0") in scripts, we create behind the scenes a temp csproj and restore using .NET CLI to resolve them. However, currently, we don't handle changes after project system init anymore. Looks like we'll be able to reuse this logic there 👍
filipw
reviewed
Nov 3, 2017
| } | ||
| } | ||
|
|
||
| private class MetadataReferenceComparer : IEqualityComparer<MetadataReference> |
There was a problem hiding this comment.
I think this needs to be extracted into a reusable place - it will be useful i.e. here
There was a problem hiding this comment.
Yes, good idea. I'll make it a utility in OmniSharp.Roslyn.
david-driscoll
approved these changes
Nov 3, 2017
Previously, we tracked whether a project update *could* invoke an automatic 'dotnet restore' and this PR adds that back. Essentially, we allow auto restore in all cases that a project is updated *except* when the update comes via a restore.
filipw
approved these changes
Nov 3, 2017
|
Note: Because this PR changes the MSBuild project system at a fundamental level, I'm going to hold off on merging this until we can get a C# for VS Code release out early next week. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes dotnet/vscode-csharp#1583, dotnet/vscode-csharp#1661, dotnet/vscode-csharp#1785
This is a fairly substantial change in order to address a problem where MSBuild projects would not be updated in response to a dotnet restore. Essentially, the MSBuild project system must watch four different files per project in order to respond to a dotnet restore:
To ensure that we don't reload and update a project multiple times in response to multiple file
changes, this PR introduces a simple queue and processing loop using a TPL DataFlow BufferBlock. The implication of this is that the MSBuild project system is now considered "initialized" only once the initial project files are queued for an update. So, OmniSharp is ready to respond to requests more quickly, but the projects won't necessarily be loaded yet.
In addition, I've made a few other changes while fixing this issue:
MetadataReferencedoes not implement value equality. To address this, I create our ownIEqualityComparer<MetadataReference>implementation.a. Paths are now treated case-insensitively as they are in the rest of OmniSharp.
b. Access to the internal callback dictionary is now serialized.