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.

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

Move Omnisharp fix to the correct place. #12297

Merged
merged 3 commits into from
Oct 13, 2021

Conversation

crummel
Copy link

@crummel crummel commented Oct 11, 2021

We had some crossed PRs and this got added while the other patches were being moved. This moves it to the correct place.

@crummel crummel requested a review from a team as a code owner October 11, 2021 22:39
We had some crossed PRs and this got added while the other patches were being moved.  This moves it to the correct place.
@crummel crummel merged commit e15beac into dotnet:release/6.0.1xx Oct 13, 2021
@ViktorHofer
Copy link
Member

ViktorHofer commented Oct 19, 2021

Just saw this patch. Did someone on @danmoseley's team approve the change? We intentionally don't provide net46 assets in our projects, according to our announcement: dotnet/announcements#190.

@danmoseley
Copy link
Member

cc @eerhardt

@crummel
Copy link
Author

crummel commented Oct 19, 2021

This is a carry-forward of our 5.0 patch. I added a comment on the Omnisharp issue pointing them to the official announcement so hopefully they can move forward on switching to a .NET Core TFM. Do you think this will cause problems? Omnisharp being broken is a huge cause of customer complaints: dotnet/vscode-csharp#4638, dotnet/vscode-csharp#4610, dotnet/vscode-csharp#4583, dotnet/vscode-csharp#4545, dotnet/vscode-csharp#4446, dotnet/vscode-csharp#4360, dotnet/vscode-csharp#1744, dotnet/vscode-csharp#980 are all related issues, so we may want to keep the workaround until Omnisharp has a proper fix.

@ViktorHofer
Copy link
Member

cc @ericstj and @terrajobst for awareness.

If there's a strong customer case then I'm all in for raising attention and getting them unblocked. What is unclear to me is why we/I haven't heard of this before. Apparently this patch already made it into a 5.0 release without our approval.

@eerhardt
Copy link
Member

this patch already made it into a 5.0 release without our approval.

The patch was needed for 5.0 because Microsoft.NET.Sdk.Razor builds their tasks assembly for net46:

image

However, for 6.0, I believe this patch is no longer necessary because all the task assemblies that ship in box with the SDK (including Microsoft.NET.Sdk.Razor) have updated to net472:

image

I asked @crummel to test the Omnisharp scenarios without this patch, to confirm if it is still needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants