Skip to content

Remove new platform members from OSPlatform #40373

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.

Sign up for GitHub

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

Jump to bottom
Merged
adamsitnik merged 9 commits into from
Aug 7, 2020
Merged

Remove new platform members from OSPlatform #40373

adamsitnik merged 9 commits into from
Aug 7, 2020

Conversation

@adamsitnik
Copy link
Member

@adamsitnik adamsitnik commented Aug 5, 2020

The spec mentions removing also the "Browser": #40111 (comment)

I am not sure about this since it was added 4 months ago together with Architecture.Wasm in #34781

cc @terrajobst

@Dotnet-GitSync-Bot
Copy link
Collaborator

Dotnet-GitSync-Bot commented Aug 5, 2020

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@adamsitnik adamsitnik added this to the 5.0.0 milestone Aug 5, 2020
@adamsitnik adamsitnik added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Aug 5, 2020
@adamsitnik
Copy link
Member Author

adamsitnik commented Aug 5, 2020

I've added the no merge label as I simply need to revert more changes (new methods added to RuntimeInformation && OrdinalIgnoreCaseChange)

@jeffhandley
Copy link
Member

jeffhandley commented Aug 5, 2020

@terrajobst / @adamsitnik -- I propose that we leave OSPlatform.Browser in place for this PR since there are already some references to it in some of our tests. We'd then keep this PR to reverting @adamsitnik's PR that had added the other members and changed the string comparison.

Here are a couple of PRs that had added references to OSPlatform.Browser:

  1. PR Fix creating OperatingSystem with PlatformID.Other #39130
  2. PR WASM: Fix System.IO.MemoryMappedFiles tests #39355

I was considering a suggestion that we change the reference to OSPlatform.Browser to use OperatingSystem.IsBrowser(), but the references to OSPlatform.Browser in those files is alongside other OSPlatform references, which would make the work snowball.

@steveisok
Copy link
Member

steveisok commented Aug 5, 2020

I would definitely feel more comfortable if we left OSPlatform.Browser in place. Snowballs at this point in time would be pretty bad.

@akoeplinger
Copy link
Member

akoeplinger commented Aug 5, 2020

I don't think it's that bad, there are only two usages of OSPlatform.Browser outside of the RuntimeInformation tests. We can trivially change those.

@campersau
Copy link
Contributor

campersau commented Aug 6, 2020

Note that aspnetcore is also already using OSPlatform.Browser: https://github.com/search?l=C%23&p=1&q=%22osplatform.browser%22&type=Code

@akoeplinger
Copy link
Member

akoeplinger commented Aug 6, 2020

Yeah we can change them back to using RuntimeInformation.IsOSPlatform(OSPlatform.Create("BROWSER")). I'd still prefer that to releasing with a new property that we immediately obsolete.

@adamsitnik adamsitnik removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Aug 6, 2020
@adamsitnik
Copy link
Member Author

adamsitnik commented Aug 6, 2020

@buyaa-n @jeffhandley PTAL. I've removed the methods and properties that I've recently added.

Once #40457 gets merged I am going to send another PR that will make RuntimeInfromation.IsOSPlatform just call OperatingSystem.IsOSPlatform, remove Browser and use new OperatingSystem methods everywhere in runtime and aspnet

buyaa-n
buyaa-n approved these changes Aug 6, 2020
View reviewed changes
Copy link
Contributor

@buyaa-n buyaa-n left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@lewing
Copy link
Member

lewing commented Aug 6, 2020 •
edited
Loading

We'll need to fix dotnet/aspnetcore as well https://github.com/dotnet/aspnetcore/search?q=OSPlatform.Browser&unscoped_q=OSPlatform.Browser

@adamsitnik
Copy link
Member Author

adamsitnik commented Aug 7, 2020

The CI failure is unrelated (Utf8String long running test timeout), merging

@adamsitnik adamsitnik merged commit 3ea278d into dotnet:master Aug 7, 2020
@adamsitnik adamsitnik deleted the removeNewOsPlatforms branch August 7, 2020 10:18
@adamsitnik adamsitnik mentioned this pull request Aug 7, 2020
Merged
@adamsitnik
Copy link
Member Author

adamsitnik commented Aug 7, 2020

I've sent a PR to ASP.NET: dotnet/aspnetcore#24652

/cc @jeffhandley @eerhardt

@adamsitnik adamsitnik mentioned this pull request Aug 7, 2020
Closed
Jacksondr5 pushed a commit to Jacksondr5/runtime that referenced this pull request Aug 10, 2020
@adamsitnik @Jacksondr5
Remove new platform members from OSPlatform (dotnet#40373)
b60dbbf 
* Remove new platform members from OSPlatform

* remove IsOSPlatformEarlierThan and IsOSPlatformOrLater methods

* remove unused resource and dependency

* delegate RuntimeInformation.IsOSPlatform to call OperatingSystem.IsOSPlatform. it won't compile now as the new guards have not been merged yet (dotnet#40457)

* remove OSPlatform.Browser and it's usage
@adamsitnik adamsitnik mentioned this pull request Aug 11, 2020
Closed
@dfederm dfederm mentioned this pull request Sep 16, 2020
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jeffhandley jeffhandley jeffhandley approved these changes

@eerhardt eerhardt eerhardt left review comments

+1 more reviewer

@buyaa-n buyaa-n buyaa-n approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-System.Runtime.InteropServices

Projects

None yet

Milestone

5.0.0

Development

Successfully merging this pull request may close these issues.

9 participants

@adamsitnik @Dotnet-GitSync-Bot @jeffhandley @steveisok @akoeplinger @campersau @lewing @eerhardt @buyaa-n