Fix bug when multi-TFM project reference is outside OmniSharp's target path #1195
Conversation
…t path When OmniSharp loads a project reference outside the OmniSharp path, it does not attempt to do a full design-time build of the project. Instead, it just uses the MSBuild project evaluation. However, an important code path was being skipped where a target framework is selected in the case that the project has multiple target frameworks. Later, this causes an exception to be thrown because several properties did not evaluate properly. This change ensures that a target framework is selected in this case.
|
@david-driscoll / @filipw : @rchande is currently on vacation. Do either of you have confidence to sign off on this change? |
| return projectCollection.LoadProject(filePath, toolsVersion); | ||
| var project = projectCollection.LoadProject(filePath, toolsVersion); | ||
|
|
||
| SetTargetFrameworkIfNeeded(project); |
There was a problem hiding this comment.
Every project goes through this code path, where we first evaluate the project with MSBuild. At this point, we need to set the <TargetFramework/> property if there are multiple <TargetFrameworks/> to ensure that the project evaluates with a TFM. Otherwise, several MSBuild properties (like TargetFrameworkMoniker) are missing.
| @@ -127,10 +131,7 @@ private static void SetTargetFrameworkIfNeeded(MSB.Evaluation.Project evaluatedP | |||
| // do better and potentially allow OmniSharp hosts to select a target framework. | |||
| targetFramework = targetFrameworks[0]; | |||
| evaluatedProject.SetProperty(PropertyNames.TargetFramework, targetFramework); | |||
| } | |||
| else if (!string.IsNullOrWhiteSpace(targetFramework) && targetFrameworks.Length == 0) | |||
There was a problem hiding this comment.
This check is not needed here. We used to pass targetFrameworks as an out parameter but that was removed long ago. So, this is essentially unnecessary code.
| else if (!string.IsNullOrWhiteSpace(targetFramework) && targetFrameworks.Length == 0) | ||
| { | ||
| targetFrameworks = ImmutableArray.Create(targetFramework); | ||
| evaluatedProject.ReevaluateIfNecessary(); |
There was a problem hiding this comment.
Here, we tell the project to re-evaluate itself if it's dirty, which will be the case because we set the <TargetFramework> property.
|
I added some comments to describe what the code changes do. The rest is just test code. |
| @@ -127,10 +131,7 @@ private static void SetTargetFrameworkIfNeeded(MSB.Evaluation.Project evaluatedP | |||
| // do better and potentially allow OmniSharp hosts to select a target framework. | |||
| targetFramework = targetFrameworks[0]; | |||
There was a problem hiding this comment.
how could we allow this to be overrideable in the future?
allow the user to set preferred TFM via MSBuildOptions in the config?
There was a problem hiding this comment.
We need create a singleton service or something that we can toggle the tfm for a given project, and then also enhance services to also understand that...
There was a problem hiding this comment.
Yes, we need the same thing for setting the active solution/project configuration as well.
Fixes dotnet/vscode-csharp#2295
When OmniSharp loads a project reference outside the OmniSharp path, it does not attempt to do a full design-time build of the project. Instead, it just uses the MSBuild project evaluation. However, an important code path was being skipped where a target framework is selected in the case that the project has multiple target frameworks. Later, this causes an exception to be thrown because several properties did not evaluate properly. This change ensures that a target framework is selected in this case.