From 61277a802a0e591b9745d9928d4fcd015681b305 Mon Sep 17 00:00:00 2001 From: Dustin Campbell Date: Wed, 17 May 2017 03:18:01 -0700 Subject: [PATCH 1/5] Improve initial unresolved dependencies checks against lock file --- src/OmniSharp.MSBuild/MSBuildProjectSystem.cs | 59 ++++++++++++++++--- 1 file changed, 52 insertions(+), 7 deletions(-) diff --git a/src/OmniSharp.MSBuild/MSBuildProjectSystem.cs b/src/OmniSharp.MSBuild/MSBuildProjectSystem.cs index 8421df0e9e..06d1cbbcc7 100644 --- a/src/OmniSharp.MSBuild/MSBuildProjectSystem.cs +++ b/src/OmniSharp.MSBuild/MSBuildProjectSystem.cs @@ -579,8 +579,7 @@ private void CheckForUnresolvedDependences(ProjectFileInfo projectFileInfo, Proj var lockFileFormat = new LockFileFormat(); var lockFile = lockFileFormat.Read(projectFileInfo.ProjectAssetsFile); - unresolvedPackageReferences = projectFileInfo.PackageReferences - .Where(pr => !ContainsPackageReference(lockFile, pr)); + unresolvedPackageReferences = FindUnresolvedPackageReferencesInLockFile(projectFileInfo, lockFile); } unresolvedDependencies = CreatePackageDependencies(unresolvedPackageReferences); @@ -620,18 +619,64 @@ private List CreatePackageDependencies(IEnumerable FindUnresolvedPackageReferencesInLockFile(ProjectFileInfo projectFileInfo, LockFile lockFile) { + if (projectFileInfo.PackageReferences.Length == 0) + { + return ImmutableArray.Empty; + } + + // Create map of all libraries in the lock file by their name. + // Note that the map's key is case-insensitive. + + var libraryMap = new Dictionary>( + capacity: lockFile.Libraries.Count, + comparer: StringComparer.OrdinalIgnoreCase); + foreach (var library in lockFile.Libraries) { - if (string.Equals(library.Name, reference.Dependency.Id, StringComparison.OrdinalIgnoreCase) && - reference.Dependency.VersionRange.Satisfies(library.Version)) + if (!libraryMap.TryGetValue(library.Name, out var libraries)) { - return true; + libraries = new List(); + libraryMap.Add(library.Name, libraries); + } + + libraries.Add(library); + } + + var unresolved = ImmutableArray.CreateBuilder(); + + // Iterate through each package reference and see if we can find a library with the same name + // that satisfies the reference's version range in the lock file. + + foreach (var reference in projectFileInfo.PackageReferences) + { + if (!libraryMap.TryGetValue(reference.Dependency.Id, out var libraries)) + { + _logger.LogWarning($"{projectFileInfo.Name}: Did not find '{reference.Dependency.Id}' in lock file."); + unresolved.Add(reference); + } + else + { + var found = false; + foreach (var library in libraries) + { + if (reference.Dependency.VersionRange.Satisfies(library.Version)) + { + found = true; + break; + } + } + + if (!found) + { + _logger.LogDebug($"{projectFileInfo.Name}: Found '{reference.Dependency.Id}' in lock file, but none of the versions satisfy {reference.Dependency.VersionRange}"); + unresolved.Add(reference); + } } } - return false; + return unresolved.ToImmutable(); } private void FireUnresolvedDependenciesEvent(ProjectFileInfo projectFileInfo, List unresolvedDependencies) From 551ca2a7fd60c3585b1b5fd912384bd87f210538 Mon Sep 17 00:00:00 2001 From: Dustin Campbell Date: Wed, 17 May 2017 04:30:54 -0700 Subject: [PATCH 2/5] Factor unresolved package dependency scanning to separate class --- src/OmniSharp.MSBuild/MSBuildProjectSystem.cs | 153 +++--------------- .../Resolution/PackageDependencyResolver.cs | 101 ++++++++++++ 2 files changed, 119 insertions(+), 135 deletions(-) create mode 100644 src/OmniSharp.MSBuild/Resolution/PackageDependencyResolver.cs diff --git a/src/OmniSharp.MSBuild/MSBuildProjectSystem.cs b/src/OmniSharp.MSBuild/MSBuildProjectSystem.cs index 06d1cbbcc7..2cef3f375c 100644 --- a/src/OmniSharp.MSBuild/MSBuildProjectSystem.cs +++ b/src/OmniSharp.MSBuild/MSBuildProjectSystem.cs @@ -9,7 +9,6 @@ using Microsoft.CodeAnalysis.CSharp; using Microsoft.Extensions.Configuration; using Microsoft.Extensions.Logging; -using NuGet.ProjectModel; using OmniSharp.Eventing; using OmniSharp.FileWatching; using OmniSharp.Models.Events; @@ -17,6 +16,7 @@ using OmniSharp.MSBuild.Models; using OmniSharp.MSBuild.Models.Events; using OmniSharp.MSBuild.ProjectFile; +using OmniSharp.MSBuild.Resolution; using OmniSharp.MSBuild.SolutionParsing; using OmniSharp.Options; using OmniSharp.Services; @@ -34,6 +34,7 @@ public class MSBuildProjectSystem : IProjectSystem private readonly IFileSystemWatcher _fileSystemWatcher; private readonly ILoggerFactory _loggerFactory; private readonly ILogger _logger; + private readonly PackageDependencyResolver _packageDepedencyResolver; private readonly object _gate = new object(); private readonly Queue _projectsToProcess; @@ -67,6 +68,7 @@ public MSBuildProjectSystem( _projects = new ProjectFileInfoCollection(); _projectsToProcess = new Queue(); _logger = loggerFactory.CreateLogger(); + _packageDepedencyResolver = new PackageDependencyResolver(loggerFactory); } public void Initalize(IConfiguration configuration) @@ -362,7 +364,7 @@ private void OnProjectChanged(string projectFilePath, bool allowAutoRestore) _projects[projectFilePath] = newProjectFileInfo; UpdateProject(newProjectFileInfo); - CheckForUnresolvedDependences(newProjectFileInfo, oldProjectFileInfo, allowAutoRestore); + CheckForUnresolvedDependences(newProjectFileInfo, allowAutoRestore); } } @@ -532,154 +534,35 @@ private void UpdateReferences(Project project, ImmutableArray references } } - private void CheckForUnresolvedDependences(ProjectFileInfo projectFileInfo, ProjectFileInfo previousProjectFileInfo = null, bool allowAutoRestore = false) + private void CheckForUnresolvedDependences(ProjectFileInfo projectFileInfo, bool allowAutoRestore) { - List unresolvedDependencies; - - if (!File.Exists(projectFileInfo.ProjectAssetsFile)) - { - // Simplest case: If there's no lock file and the project file has package references, - // there are certainly unresolved dependencies. - unresolvedDependencies = CreatePackageDependencies(projectFileInfo.PackageReferences); - } - else - { - // Note: This is a bit of misnmomer. It's entirely possible that a package reference has been removed - // and a restore needs to happen in order to update project.assets.json file. Otherwise, the MSBuild targets - // will still resolve the removed reference as a reference in the user's project. In that case, the package - // reference isn't so much "unresolved" as "incorrectly resolved". - IEnumerable unresolvedPackageReferences; - - // Did the project file change? Diff the package references and see if there are unresolved dependencies. - if (previousProjectFileInfo != null) - { - var remainingPackageReferences = new HashSet(previousProjectFileInfo.PackageReferences); - var packageReferencesToAdd = new HashSet(); - - foreach (var packageReference in projectFileInfo.PackageReferences) - { - if (remainingPackageReferences.Contains(packageReference)) - { - remainingPackageReferences.Remove(packageReference); - } - else - { - packageReferencesToAdd.Add(packageReference); - } - } - - unresolvedPackageReferences = packageReferencesToAdd.Concat(remainingPackageReferences); - } - else - { - // Finally, if the project.assets.json file exists but there's no old project to compare against, - // we'll just check to ensure that all of the project's package references can be found in the - // current project.assets.json file. - - var lockFileFormat = new LockFileFormat(); - var lockFile = lockFileFormat.Read(projectFileInfo.ProjectAssetsFile); - - unresolvedPackageReferences = FindUnresolvedPackageReferencesInLockFile(projectFileInfo, lockFile); - } - - unresolvedDependencies = CreatePackageDependencies(unresolvedPackageReferences); - } - - if (unresolvedDependencies.Count > 0) + var unresolvedPackageReferences = _packageDepedencyResolver.FindUnresolvedPackageReferences(projectFileInfo); + if (unresolvedPackageReferences.IsEmpty) { - if (allowAutoRestore && _options.EnablePackageAutoRestore) - { - _dotNetCli.RestoreAsync(projectFileInfo.Directory, onFailure: () => - { - FireUnresolvedDependenciesEvent(projectFileInfo, unresolvedDependencies); - }); - } - else - { - FireUnresolvedDependenciesEvent(projectFileInfo, unresolvedDependencies); - } + return; } - } - - private List CreatePackageDependencies(IEnumerable packageReferences) - { - var list = new List(); - foreach (var packageReference in packageReferences) - { - var dependency = new PackageDependency + var unresolvedDependencies = unresolvedPackageReferences.Select(packageReference => + new PackageDependency { Name = packageReference.Dependency.Id, Version = packageReference.Dependency.VersionRange.ToNormalizedString() - }; - - list.Add(dependency); - } - - return list; - } - - private ImmutableArray FindUnresolvedPackageReferencesInLockFile(ProjectFileInfo projectFileInfo, LockFile lockFile) - { - if (projectFileInfo.PackageReferences.Length == 0) - { - return ImmutableArray.Empty; - } - - // Create map of all libraries in the lock file by their name. - // Note that the map's key is case-insensitive. - - var libraryMap = new Dictionary>( - capacity: lockFile.Libraries.Count, - comparer: StringComparer.OrdinalIgnoreCase); + }); - foreach (var library in lockFile.Libraries) + if (allowAutoRestore && _options.EnablePackageAutoRestore) { - if (!libraryMap.TryGetValue(library.Name, out var libraries)) + _dotNetCli.RestoreAsync(projectFileInfo.Directory, onFailure: () => { - libraries = new List(); - libraryMap.Add(library.Name, libraries); - } - - libraries.Add(library); + FireUnresolvedDependenciesEvent(projectFileInfo, unresolvedDependencies); + }); } - - var unresolved = ImmutableArray.CreateBuilder(); - - // Iterate through each package reference and see if we can find a library with the same name - // that satisfies the reference's version range in the lock file. - - foreach (var reference in projectFileInfo.PackageReferences) + else { - if (!libraryMap.TryGetValue(reference.Dependency.Id, out var libraries)) - { - _logger.LogWarning($"{projectFileInfo.Name}: Did not find '{reference.Dependency.Id}' in lock file."); - unresolved.Add(reference); - } - else - { - var found = false; - foreach (var library in libraries) - { - if (reference.Dependency.VersionRange.Satisfies(library.Version)) - { - found = true; - break; - } - } - - if (!found) - { - _logger.LogDebug($"{projectFileInfo.Name}: Found '{reference.Dependency.Id}' in lock file, but none of the versions satisfy {reference.Dependency.VersionRange}"); - unresolved.Add(reference); - } - } + FireUnresolvedDependenciesEvent(projectFileInfo, unresolvedDependencies); } - - return unresolved.ToImmutable(); } - private void FireUnresolvedDependenciesEvent(ProjectFileInfo projectFileInfo, List unresolvedDependencies) + private void FireUnresolvedDependenciesEvent(ProjectFileInfo projectFileInfo, IEnumerable unresolvedDependencies) { _eventEmitter.Emit(EventTypes.UnresolvedDependencies, new UnresolvedDependenciesMessage() diff --git a/src/OmniSharp.MSBuild/Resolution/PackageDependencyResolver.cs b/src/OmniSharp.MSBuild/Resolution/PackageDependencyResolver.cs new file mode 100644 index 0000000000..986856ddb1 --- /dev/null +++ b/src/OmniSharp.MSBuild/Resolution/PackageDependencyResolver.cs @@ -0,0 +1,101 @@ +using System; +using System.Collections.Generic; +using System.Collections.Immutable; +using System.IO; +using Microsoft.Extensions.Logging; +using NuGet.ProjectModel; +using OmniSharp.MSBuild.ProjectFile; + +namespace OmniSharp.MSBuild.Resolution +{ + internal class PackageDependencyResolver + { + private readonly ILogger _logger; + + public PackageDependencyResolver(ILoggerFactory loggerFactory) + { + _logger = loggerFactory.CreateLogger(); + } + + public ImmutableArray FindUnresolvedPackageReferences(ProjectFileInfo projectFile) + { + if (projectFile.PackageReferences.Length == 0) + { + return ImmutableArray.Empty; + } + + // If the lock file does not exist, all of the package references are unresolved. + if (!File.Exists(projectFile.ProjectAssetsFile)) + { + return projectFile.PackageReferences; + } + + var lockFileFormat = new LockFileFormat(); + var lockFile = lockFileFormat.Read(projectFile.ProjectAssetsFile); + + return FindUnresolvedPackageReferencesInLockFile(projectFile, lockFile); + } + + private ImmutableArray FindUnresolvedPackageReferencesInLockFile(ProjectFileInfo projectFile, LockFile lockFile) + { + var libraryMap = CreateLibraryMap(lockFile); + + var unresolved = ImmutableArray.CreateBuilder(); + + // Iterate through each package reference and see if we can find a library with the same name + // that satisfies the reference's version range in the lock file. + + foreach (var reference in projectFile.PackageReferences) + { + if (!libraryMap.TryGetValue(reference.Dependency.Id, out var libraries)) + { + _logger.LogWarning($"{projectFile.Name}: Did not find '{reference.Dependency.Id}' in lock file."); + unresolved.Add(reference); + } + else + { + var found = false; + foreach (var library in libraries) + { + if (reference.Dependency.VersionRange.Satisfies(library.Version)) + { + found = true; + break; + } + } + + if (!found) + { + _logger.LogDebug($"{projectFile.Name}: Found '{reference.Dependency.Id}' in lock file, but none of the versions satisfy {reference.Dependency.VersionRange}"); + unresolved.Add(reference); + } + } + } + + return unresolved.ToImmutable(); + } + + private static Dictionary> CreateLibraryMap(LockFile lockFile) + { + // Create map of all libraries in the lock file by their name. + // Note that the map's key is case-insensitive. + + var libraryMap = new Dictionary>( + capacity: lockFile.Libraries.Count, + comparer: StringComparer.OrdinalIgnoreCase); + + foreach (var library in lockFile.Libraries) + { + if (!libraryMap.TryGetValue(library.Name, out var libraries)) + { + libraries = new List(); + libraryMap.Add(library.Name, libraries); + } + + libraries.Add(library); + } + + return libraryMap; + } + } +} From 885151675593bb04788d7bb738c2be1282109671 Mon Sep 17 00:00:00 2001 From: Dustin Campbell Date: Wed, 17 May 2017 04:41:40 -0700 Subject: [PATCH 3/5] Unify code to emit unresolved dependencies events --- .../Eventing/IEventEmitterExtensions.cs | 12 +++++++++ src/OmniSharp.DotNet/DotNetProjectSystem.cs | 27 ++++++++++--------- src/OmniSharp.MSBuild/MSBuildProjectSystem.cs | 20 ++++---------- 3 files changed, 31 insertions(+), 28 deletions(-) diff --git a/src/OmniSharp.Abstractions/Eventing/IEventEmitterExtensions.cs b/src/OmniSharp.Abstractions/Eventing/IEventEmitterExtensions.cs index eb936afde6..0d093a4502 100644 --- a/src/OmniSharp.Abstractions/Eventing/IEventEmitterExtensions.cs +++ b/src/OmniSharp.Abstractions/Eventing/IEventEmitterExtensions.cs @@ -1,4 +1,5 @@ using System; +using System.Collections.Generic; using OmniSharp.Models.Events; namespace OmniSharp.Eventing @@ -29,5 +30,16 @@ public static void RestoreFinished(this IEventEmitter emitter, string projectPat Succeeded = succeeded }); } + + public static void UnresolvedDepdendencies(this IEventEmitter emitter, string projectFilePath, IEnumerable unresolvedDependencies) + { + emitter.Emit( + EventTypes.UnresolvedDependencies, + new UnresolvedDependenciesMessage + { + FileName = projectFilePath, + UnresolvedDependencies = unresolvedDependencies + }); + } } } diff --git a/src/OmniSharp.DotNet/DotNetProjectSystem.cs b/src/OmniSharp.DotNet/DotNetProjectSystem.cs index cf4c04f7a5..0f35d6328a 100644 --- a/src/OmniSharp.DotNet/DotNetProjectSystem.cs +++ b/src/OmniSharp.DotNet/DotNetProjectSystem.cs @@ -302,29 +302,30 @@ private void UpdateUnresolvedDependencies(ProjectState state, bool allowRestore) { var libraryManager = state.ProjectContext.LibraryManager; var allDiagnostics = libraryManager.GetAllDiagnostics(); - var unresolved = libraryManager.GetLibraries().Where(dep => !dep.Resolved); - var needRestore = allDiagnostics.Any(diag => diag.ErrorCode == ErrorCodes.NU1006) || unresolved.Any(); + var unresolvedLibraries = libraryManager.GetLibraries().Where(dep => !dep.Resolved); + var needRestore = allDiagnostics.Any(diag => diag.ErrorCode == ErrorCodes.NU1006) || unresolvedLibraries.Any(); if (needRestore) { + var unresolvedDependencies = unresolvedLibraries.Select(library => + new PackageDependency + { + Name = library.Identity.Name, + Version = library.Identity.Version?.ToString() + }); + + var projectFile = state.ProjectContext.ProjectFile; + if (allowRestore && _enableRestorePackages) { - _dotNetCliService.RestoreAsync(state.ProjectContext.ProjectFile.ProjectDirectory, onFailure: () => + _dotNetCliService.RestoreAsync(projectFile.ProjectDirectory, onFailure: () => { - _eventEmitter.Emit(EventTypes.UnresolvedDependencies, new UnresolvedDependenciesMessage() - { - FileName = state.ProjectContext.ProjectFile.ProjectFilePath, - UnresolvedDependencies = unresolved.Select(d => new PackageDependency { Name = d.Identity.Name, Version = d.Identity.Version?.ToString() }) - }); + _eventEmitter.UnresolvedDepdendencies(projectFile.ProjectFilePath, unresolvedDependencies); }); } else { - _eventEmitter.Emit(EventTypes.UnresolvedDependencies, new UnresolvedDependenciesMessage() - { - FileName = state.ProjectContext.ProjectFile.ProjectFilePath, - UnresolvedDependencies = unresolved.Select(d => new PackageDependency { Name = d.Identity.Name, Version = d.Identity.Version?.ToString() }) - }); + _eventEmitter.UnresolvedDepdendencies(projectFile.ProjectFilePath, unresolvedDependencies); } } } diff --git a/src/OmniSharp.MSBuild/MSBuildProjectSystem.cs b/src/OmniSharp.MSBuild/MSBuildProjectSystem.cs index 2cef3f375c..e769bb9d7d 100644 --- a/src/OmniSharp.MSBuild/MSBuildProjectSystem.cs +++ b/src/OmniSharp.MSBuild/MSBuildProjectSystem.cs @@ -534,9 +534,9 @@ private void UpdateReferences(Project project, ImmutableArray references } } - private void CheckForUnresolvedDependences(ProjectFileInfo projectFileInfo, bool allowAutoRestore) + private void CheckForUnresolvedDependences(ProjectFileInfo projectFile, bool allowAutoRestore) { - var unresolvedPackageReferences = _packageDepedencyResolver.FindUnresolvedPackageReferences(projectFileInfo); + var unresolvedPackageReferences = _packageDepedencyResolver.FindUnresolvedPackageReferences(projectFile); if (unresolvedPackageReferences.IsEmpty) { return; @@ -551,27 +551,17 @@ private void CheckForUnresolvedDependences(ProjectFileInfo projectFileInfo, bool if (allowAutoRestore && _options.EnablePackageAutoRestore) { - _dotNetCli.RestoreAsync(projectFileInfo.Directory, onFailure: () => + _dotNetCli.RestoreAsync(projectFile.Directory, onFailure: () => { - FireUnresolvedDependenciesEvent(projectFileInfo, unresolvedDependencies); + _eventEmitter.UnresolvedDepdendencies(projectFile.FilePath, unresolvedDependencies); }); } else { - FireUnresolvedDependenciesEvent(projectFileInfo, unresolvedDependencies); + _eventEmitter.UnresolvedDepdendencies(projectFile.FilePath, unresolvedDependencies); } } - private void FireUnresolvedDependenciesEvent(ProjectFileInfo projectFileInfo, IEnumerable unresolvedDependencies) - { - _eventEmitter.Emit(EventTypes.UnresolvedDependencies, - new UnresolvedDependenciesMessage() - { - FileName = projectFileInfo.FilePath, - UnresolvedDependencies = unresolvedDependencies - }); - } - private ProjectFileInfo GetProjectFileInfo(string path) { if (!_projects.TryGetValue(path, out var projectFileInfo)) From 162c14a0ffefc7c4f58203cf2a05a08b511ba7b0 Mon Sep 17 00:00:00 2001 From: Dustin Campbell Date: Wed, 17 May 2017 04:45:08 -0700 Subject: [PATCH 4/5] Log warnings from package dependency resolver --- src/OmniSharp.MSBuild/Resolution/PackageDependencyResolver.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/OmniSharp.MSBuild/Resolution/PackageDependencyResolver.cs b/src/OmniSharp.MSBuild/Resolution/PackageDependencyResolver.cs index 986856ddb1..74a251833c 100644 --- a/src/OmniSharp.MSBuild/Resolution/PackageDependencyResolver.cs +++ b/src/OmniSharp.MSBuild/Resolution/PackageDependencyResolver.cs @@ -66,7 +66,7 @@ private ImmutableArray FindUnresolvedPackageReferencesInLockFi if (!found) { - _logger.LogDebug($"{projectFile.Name}: Found '{reference.Dependency.Id}' in lock file, but none of the versions satisfy {reference.Dependency.VersionRange}"); + _logger.LogWarning($"{projectFile.Name}: Found '{reference.Dependency.Id}' in lock file, but none of the versions satisfy {reference.Dependency.VersionRange}"); unresolved.Add(reference); } } From 1b140cbc5ed9f5750f86ee40c09f177a978ae8ec Mon Sep 17 00:00:00 2001 From: Dustin Campbell Date: Wed, 17 May 2017 05:11:56 -0700 Subject: [PATCH 5/5] Clean up warning logged by package dependency resolver --- .../Resolution/PackageDependencyResolver.cs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/OmniSharp.MSBuild/Resolution/PackageDependencyResolver.cs b/src/OmniSharp.MSBuild/Resolution/PackageDependencyResolver.cs index 74a251833c..01cbc37cf3 100644 --- a/src/OmniSharp.MSBuild/Resolution/PackageDependencyResolver.cs +++ b/src/OmniSharp.MSBuild/Resolution/PackageDependencyResolver.cs @@ -2,6 +2,7 @@ using System.Collections.Generic; using System.Collections.Immutable; using System.IO; +using System.Linq; using Microsoft.Extensions.Logging; using NuGet.ProjectModel; using OmniSharp.MSBuild.ProjectFile; @@ -66,7 +67,13 @@ private ImmutableArray FindUnresolvedPackageReferencesInLockFi if (!found) { - _logger.LogWarning($"{projectFile.Name}: Found '{reference.Dependency.Id}' in lock file, but none of the versions satisfy {reference.Dependency.VersionRange}"); + var referenceText = reference.IsImplicitlyDefined + ? "implicit package reference" + : "package reference"; + + var versions = string.Join(", ", libraries.Select(l => '"' + l.Version.ToString() + '"')); + + _logger.LogWarning($"{projectFile.Name}: Found {referenceText} '{reference.Dependency.Id}', but none of the versions in the lock file ({versions}) satisfy {reference.Dependency.VersionRange}"); unresolved.Add(reference); } }