Skip to content

Conversation

@mdh1418
Copy link
Member

@mdh1418 mdh1418 commented Jun 9, 2020

This PR looks to extend the PlatformNotSupportedException for OpenSslVersion from OSX to OSX, iOS, and tvOS, as none of these Apple OS support OpenSsl.

To test changes, reverted #36928 to get an OpenSslVersion exception.
Ran the tests and obtained similar errors.

Ran the tests with changes from this PR and obtained errors such as

        <failure exception-type="System.PlatformNotSupportedException">
          <message><![CDATA[System.PlatformNotSupportedException : Operation is not supported on this platform.]]></message>
          <stack-trace><![CDATA[   at System.PlatformDetection.get_OpenSslVersion()
   at System.PlatformDetection.get_SupportsAlpn()
   at System.Net.Test.Common.GenericLoopbackOptions..ctor()
   at System.Net.Test.Common.LoopbackServer.Options..ctor()
   at System.Net.Test.Common.LoopbackServer..ctor(Options options)
   at System.Net.Test.Common.LoopbackServer.CreateServerAsync(Func`2 funcAsync, Options options)
   at System.Net.Http.Json.Functional.Tests.HttpContentJsonExtensionsTests.TestValidMediaTypes(String mediaType)
--- End of stack trace from previous location ---]]></stack-trace>
        </failure>
      </test>```
[PNSE.txt](https://github.com/dotnet/runtime/files/4752719/PNSE.txt)

@Dotnet-GitSync-Bot
Copy link
Collaborator

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghost
Copy link

ghost commented Jun 9, 2020

Tagging subscribers to this area: @ViktorHofer
Notify danmosemsft if you want to be subscribed.

private readonly int _dummyPrimitive;
public static System.Runtime.InteropServices.OSPlatform Browser { get { throw null; } }
public static System.Runtime.InteropServices.OSPlatform FreeBSD { get { throw null; } }
public static System.Runtime.InteropServices.OSPlatform iOS { get { throw null; } }
Copy link
Member

Choose a reason for hiding this comment

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

instead of adding this to the ref (which will be done in #36704 and needs API review), please instead use RuntimeInformation.IsOSPlatform(OSPlatform.Create("IOS")) in the usages.

Copy link
Member Author

Choose a reason for hiding this comment

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

Should a follow up after #36704 be to change them back to RuntimeInformation.IsOSPlatform(OSPlatform.iOS) and RuntimeInformation.IsOSPlatform(OSPlatform.tvOS)?

Copy link
Member

Choose a reason for hiding this comment

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

ideally yes, but it's just a cosmetic cleanup

@akoeplinger akoeplinger merged commit 5862ede into dotnet:master Jun 9, 2020
@mdh1418 mdh1418 deleted the mdhwang/Add_apple_to_OpenSslVersion_PNSE branch June 9, 2020 17:47
@ghost ghost locked as resolved and limited conversation to collaborators Dec 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants