Skip to content
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.

Sign up for GitHub

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

Handle CSProj files that are not C# projects that #1181

Merged
merged 3 commits into from
May 10, 2018
Merged

Handle CSProj files that are not C# projects that #1181

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e303905
The changes allow OmniSharp not to stop processing projects when the …
dmgonch May 8, 2018
e5161cd
Addressed PR feedback
dmgonch May 9, 2018
2689638
removed unused 'using'
dmgonch May 9, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 30 additions & 5 deletions src/OmniSharp.MSBuild/ProjectManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using System;
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.Collections.Immutable;
using System.IO;
Expand Down Expand Up @@ -42,6 +43,7 @@ public ProjectToUpdate(string filePath, bool allowAutoRestore)
private readonly MetadataFileReferenceCache _metadataFileReferenceCache;
private readonly PackageDependencyChecker _packageDependencyChecker;
private readonly ProjectFileInfoCollection _projectFiles;
private readonly ConcurrentDictionary<string, int> _failedToLoadProjectFiles;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is a ConcurrentDictionary really needed? MSBuild project loader really doesn't do much in the way of concurrency since it can't use MSBuild concurrently. All of the methods where _failedToLoadProjectFiles is updated also update _projectFiles, which is just a wrapper around a List and a Dictionary. In other words, the majority of this code is already synchronous. 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was hoping that this could the first step towards making OmniSharp to be able loading multiple projects in parallel. But for the sake of consistency I will go ahead and change this to Dictionary.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, loading projects in parallel is a big effort. The issue here is that MSBuild handles parallel builds by launching multiple processes to serve as additional nodes. However, doing that cross platform is challenging.

private readonly ProjectLoader _projectLoader;
private readonly OmniSharpWorkspace _workspace;

Expand All @@ -61,6 +63,7 @@ public ProjectManager(ILoggerFactory loggerFactory, IEventEmitter eventEmitter,
_metadataFileReferenceCache = metadataFileReferenceCache;
_packageDependencyChecker = packageDependencyChecker;
_projectFiles = new ProjectFileInfoCollection();
_failedToLoadProjectFiles = new ConcurrentDictionary<string, int>(StringComparer.OrdinalIgnoreCase);
_projectLoader = projectLoader;
_workspace = workspace;

Expand Down Expand Up @@ -148,14 +151,25 @@ private void ProcessQueue(CancellationToken cancellationToken)
projectList.Add(currentProject);

// update or add project
_failedToLoadProjectFiles.TryRemove(currentProject.FilePath, out int notUsed);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

out int nonUsed can just be out _ in C# 7.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

if (_projectFiles.TryGetValue(currentProject.FilePath, out var projectFileInfo))
{
projectFileInfo = ReloadProject(projectFileInfo);
if (projectFileInfo == null)
{
_failedToLoadProjectFiles.TryAdd(currentProject.FilePath, 0 /*not used*/);
continue;
}
_projectFiles[currentProject.FilePath] = projectFileInfo;
}
else
{
projectFileInfo = LoadProject(currentProject.FilePath);
if (projectFileInfo == null)
{
_failedToLoadProjectFiles.TryAdd(currentProject.FilePath, 0 /*not used*/);
continue;
}
AddProject(projectFileInfo);
}
}
Expand Down Expand Up @@ -193,31 +207,36 @@ private ProjectFileInfo LoadOrReloadProject(string projectFilePath, Func<(Projec
_logger.LogInformation($"Loading project: {projectFilePath}");

ProjectFileInfo projectFileInfo;
ImmutableArray<MSBuildDiagnostic> diagnostics;

try
{
ImmutableArray<MSBuildDiagnostic> diagnostics;
(projectFileInfo, diagnostics) = loadFunc();

if (projectFileInfo == null)
if (projectFileInfo != null)
{
_logger.LogWarning($"Successfully loaded project file '{projectFilePath}'.");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would this be a warning?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point - fixed.

}
else
{
_logger.LogWarning($"Failed to load project file '{projectFilePath}'.");
}

_eventEmitter.MSBuildProjectDiagnostics(projectFilePath, diagnostics);
}
catch (Exception ex)
{
_logger.LogWarning($"Failed to load project file '{projectFilePath}'.", ex);
_logger.LogWarning($"Failed to load project file '{projectFilePath}' : {ex.ToString()}.");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why switch over to ex.ToString()? Passing ex should still log the exception.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, it feels like this should probably be LogError.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my testing it didn't print the exception. But I will go ahead and revert this line to the original this is not related to the issue being fixed by these changes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm... we should check and see if the exception's not printing. I've definitely seen exceptions print in the past, but I wonder if it's not printing because this is a warning. I'll take a look.

_eventEmitter.Error(ex, fileName: projectFilePath);
projectFileInfo = null;
}

_eventEmitter.MSBuildProjectDiagnostics(projectFilePath, diagnostics);

return projectFileInfo;
}

private bool RemoveProject(string projectFilePath)
{
_failedToLoadProjectFiles.TryRemove(projectFilePath, out int notUsed);
if (!_projectFiles.TryGetValue(projectFilePath, out var projectFileInfo))
{
return false;
Expand Down Expand Up @@ -406,6 +425,12 @@ private void UpdateProjectReferences(Project project, ImmutableArray<string> pro

foreach (var projectReferencePath in projectReferencePaths)
{
if (_failedToLoadProjectFiles.ContainsKey(projectReferencePath))
{
_logger.LogWarning($"Ignoring previously failed to load project '{projectReferencePath}' referenced by '{project.Name}'.");
continue;
}

if (!_projectFiles.TryGetValue(projectReferencePath, out var referencedProject))
{
if (File.Exists(projectReferencePath) &&
Expand Down