Cross-compile SignalR client libraries and enable platform compat ana… #26140

pranavkm · 2020-09-21T18:52:44Z

…lyzer

Contributes to #25917

src/SignalR/clients/csharp/Http.Connections.Client/src/HttpConnection.cs

halter73 · 2020-09-21T23:58:14Z

src/SignalR/clients/csharp/Http.Connections.Client/src/Internal/WebSocketsTransport.cs

                // Set this header so the server auth middleware will set an Unauthorized instead of Redirect status code
                // See: https://github.com/aspnet/Security/blob/ff9f145a8e89c9756ea12ff10c6d47f2f7eb345f/src/Microsoft.AspNetCore.Authentication.Cookies/Events/CookieAuthenticationEvents.cs#L42
+#pragma warning disable CA1416 // Analyzer bug


Is there an issue related to the analyzer bug that we can link to?

No, I was hoping this was the result of one of the SDK bugs that was already resolved.

@mavasani \ @buyaa-n any idea why this might be busted and how we could track this? I can't get a minimal app that reproduces the problem, but it is consistent in this branch of AspNetCore. I "updated" this branch to target 5.0.100-rc.2.20472.12 but that doesn't appear to help.

5.0.100-rc.2.20472.12

I am presuming this is the SDK version? Can you try the latest RC2 NuGet package: https://dev.azure.com/dnceng/public/_packaging?_a=package&feed=dotnet5&package=Microsoft.CodeAnalysis.NetAnalyzers&protocolType=NuGet&version=5.0.0-rc2.20471.13&view=overview?

Can you also check if this repros if you replace the boolean check above with explicit OperatingSystem.IsBrowser() check? If that removes the false positive, then it is likely that something in flow analysis is causing the state stored in the bool to get lost before this if block. I can try to repro this later today.

Surprsingly seems it is worked above on row 45 and 64, somehow working on off?

No good. As @buyaa-n pointed out, the same check works in the ifs above, just not after the if block on line 52

halter73 · 2020-09-22T00:15:06Z

Do we need to update Protocols.MessagePack and Protocols.NewtonsoftJson to also target DefaultNetCoreTargetFramework?

src/SignalR/clients/csharp/Http.Connections.Client/src/HttpConnectionFactory.cs

src/SignalR/clients/csharp/Http.Connections.Client/src/Internal/LongPollingTransport.cs

src/SignalR/clients/csharp/Http.Connections.Client/src/Internal/WebSocketsTransport.cs

src/SignalR/clients/csharp/Http.Connections.Client/src/HttpConnectionFactory.cs

mavasani · 2020-09-22T23:28:21Z

src/SignalR/clients/csharp/Http.Connections.Client/src/Internal/WebSocketsTransport.cs

                    {
                        var accessTokenEncoded = UrlEncoder.Default.Encode(accessToken);
                        accessTokenEncoded = "access_token=" + accessTokenEncoded;
                        resolvedUrl = Utils.AppendQueryString(resolvedUrl, accessTokenEncoded);
                    }
                    else
                    {
+#pragma warning disable CA1416 // Analyzer bug


This feels surprising given you have an explicit OperatingSystem.IsBrowser() check in the if part. What are the platform attributes applied to Options and SetRequestHeader symbols?

Seems [System.Runtime.Versioning.UnsupportedOSPlatformAttribute("browser")]
https://github.com/dotnet/runtime/pull/41963/files#diff-0f06b592b2ee5b30d95ae38efae16a85R49

BrennanConroy · 2020-09-23T16:53:33Z

Can we add supported platform browser to https://github.com/dotnet/aspnetcore/blob/master/src/SignalR/common/Protocols.Json/src/Microsoft.AspNetCore.SignalR.Protocols.Json.csproj?

…lyzer

...R/common/Protocols.MessagePack/src/Microsoft.AspNetCore.SignalR.Protocols.MessagePack.csproj

pranavkm

Next up add nullability to these types

src/SignalR/clients/csharp/Http.Connections.Client/src/HttpConnectionFactory.cs

ghost · 2020-09-23T18:57:32Z

Hello @pranavkm!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

davidfowl reviewed Sep 21, 2020

View reviewed changes

src/SignalR/clients/csharp/Http.Connections.Client/src/HttpConnection.cs Outdated Show resolved Hide resolved

pranavkm requested a review from BrennanConroy September 21, 2020 18:58

pranavkm commented Sep 21, 2020

View reviewed changes

src/SignalR/clients/csharp/Http.Connections.Client/src/HttpConnection.cs Show resolved Hide resolved

src/SignalR/clients/csharp/Http.Connections.Client/src/HttpConnection.cs Outdated Show resolved Hide resolved

pranavkm force-pushed the prkrishn/netcoreapp-client branch 2 times, most recently from df9b38e to 1438f75 Compare September 21, 2020 19:45

pranavkm marked this pull request as ready for review September 21, 2020 19:45

pranavkm requested a review from halter73 as a code owner September 21, 2020 19:45

pranavkm force-pushed the prkrishn/netcoreapp-client branch from 1438f75 to 876befa Compare September 21, 2020 21:27

halter73 reviewed Sep 21, 2020

View reviewed changes

BrennanConroy reviewed Sep 22, 2020

View reviewed changes

src/SignalR/clients/csharp/Http.Connections.Client/src/HttpConnectionFactory.cs Show resolved Hide resolved

BrennanConroy reviewed Sep 22, 2020

View reviewed changes

src/SignalR/clients/csharp/Http.Connections.Client/src/Internal/LongPollingTransport.cs Show resolved Hide resolved

BrennanConroy reviewed Sep 22, 2020

View reviewed changes

src/SignalR/clients/csharp/Http.Connections.Client/src/Internal/WebSocketsTransport.cs Outdated Show resolved Hide resolved

BrennanConroy reviewed Sep 22, 2020

View reviewed changes

src/SignalR/clients/csharp/Http.Connections.Client/src/Internal/WebSocketsTransport.cs Outdated Show resolved Hide resolved

halter73 reviewed Sep 22, 2020

View reviewed changes

src/SignalR/clients/csharp/Http.Connections.Client/src/HttpConnectionFactory.cs Show resolved Hide resolved

Pilchie added the area-signalr Includes: SignalR clients and servers label Sep 22, 2020

pranavkm force-pushed the prkrishn/netcoreapp-client branch from 876befa to 998cc42 Compare September 22, 2020 23:01

pranavkm added this to the 6.0.0-alpha1 milestone Sep 22, 2020

mavasani reviewed Sep 22, 2020

View reviewed changes

pranavkm added 5 commits September 23, 2020 10:05

Cross-compile SignalR client libraries and enable platform compat ana…

9414ee7

…lyzer

Make our own OperatingSystem for downlevel tfms

38660f2

Changes per PR feedback

886d36e

Eliminate one of the analyzer bugs

84fc71c

Add browser

f715aa5

pranavkm force-pushed the prkrishn/netcoreapp-client branch from 44739f9 to f715aa5 Compare September 23, 2020 17:14

BrennanConroy reviewed Sep 23, 2020

View reviewed changes

...R/common/Protocols.MessagePack/src/Microsoft.AspNetCore.SignalR.Protocols.MessagePack.csproj Show resolved Hide resolved

halter73 approved these changes Sep 23, 2020

View reviewed changes

pranavkm commented Sep 23, 2020

View reviewed changes

src/SignalR/clients/csharp/Http.Connections.Client/src/HttpConnectionFactory.cs Show resolved Hide resolved

pranavkm added the auto-merge label Sep 23, 2020

This was referenced Sep 23, 2020

Test failures via timeline in Roslyn jaredpar/runfo#37

Closed

Test failures via timeline in Roslyn jaredpar/runfo#38

Closed

ghost merged commit eb3a719 into master Sep 23, 2020

ghost deleted the prkrishn/netcoreapp-client branch September 23, 2020 19:19

wtgodbe mentioned this pull request Dec 23, 2020

Consider cross-compiling projects to target DefaultNetCoreTargetFramework #25917

Closed

This was referenced Sep 22, 2023

Identifying whether the page is server or client for RenderMode.Auto #50859

Closed

Add OperatingSystem.IsBrowser to the overview article dotnet/AspNetCore.Docs#30437

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cross-compile SignalR client libraries and enable platform compat ana… #26140

Cross-compile SignalR client libraries and enable platform compat ana… #26140

pranavkm commented Sep 21, 2020 •

edited

Loading

halter73 Sep 21, 2020

pranavkm Sep 22, 2020

mavasani Sep 22, 2020

buyaa-n Sep 22, 2020

pranavkm Sep 23, 2020

halter73 commented Sep 22, 2020

mavasani Sep 22, 2020 •

edited

Loading

buyaa-n Sep 22, 2020

BrennanConroy commented Sep 23, 2020

pranavkm left a comment

ghost commented Sep 23, 2020

Cross-compile SignalR client libraries and enable platform compat ana… #26140

Cross-compile SignalR client libraries and enable platform compat ana… #26140

Conversation

pranavkm commented Sep 21, 2020 • edited Loading

halter73 Sep 21, 2020

Choose a reason for hiding this comment

pranavkm Sep 22, 2020

Choose a reason for hiding this comment

mavasani Sep 22, 2020

Choose a reason for hiding this comment

buyaa-n Sep 22, 2020

Choose a reason for hiding this comment

pranavkm Sep 23, 2020

Choose a reason for hiding this comment

halter73 commented Sep 22, 2020

mavasani Sep 22, 2020 • edited Loading

Choose a reason for hiding this comment

buyaa-n Sep 22, 2020

Choose a reason for hiding this comment

BrennanConroy commented Sep 23, 2020

pranavkm left a comment

Choose a reason for hiding this comment

ghost commented Sep 23, 2020

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

pranavkm commented Sep 21, 2020 •

edited

Loading

mavasani Sep 22, 2020 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.