Handle CSProj files that are not C# projects that #1181
Conversation
…extension encounters a file with CSProj extension that is not a C# project.
| @@ -148,14 +151,25 @@ private void ProcessQueue(CancellationToken cancellationToken) | |||
| projectList.Add(currentProject); | |||
|
|
|||
| // update or add project | |||
| _failedToLoadProjectFiles.TryRemove(currentProject.FilePath, out int notUsed); | |||
There was a problem hiding this comment.
out int nonUsed can just be out _ in C# 7.
| @@ -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; | |||
There was a problem hiding this comment.
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. 😄
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| if (projectFileInfo == null) | ||
| if (projectFileInfo != null) | ||
| { | ||
| _logger.LogWarning($"Successfully loaded project file '{projectFilePath}'."); |
There was a problem hiding this comment.
Why would this be a warning?
| } | ||
| catch (Exception ex) | ||
| { | ||
| _logger.LogWarning($"Failed to load project file '{projectFilePath}'.", ex); | ||
| _logger.LogWarning($"Failed to load project file '{projectFilePath}' : {ex.ToString()}."); |
There was a problem hiding this comment.
Why switch over to ex.ToString()? Passing ex should still log the exception.
There was a problem hiding this comment.
Also, it feels like this should probably be LogError.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
The proposed changes allow OmniSharp not to stop processing projects when it encounters a file with CSProj extension that is not a C# project. At the moment I'm working with a repo where out of 736 CSProj files 229 are of this sort. They typically are TypeScript projects, projects that copy files into final release folder and project files used only as data inputs for test cases. With my changes, OmniSharp invoked from VSCode was able to successfully process all valid C# projects while ignoring the rest.