Remove references to PlatformAbstractions.

[eerhardt]

This will allow us to remove the PlatformAbstractions library from dotnet/runtime.

Contributes to dotnet/runtime#3470

[eerhardt]

@dsplaisted @wli3 - thoughts on this?

[dsplaisted]

@eerhardt Is the RuntimeEnvironment class here a bunch of code copied from the PlatformAbstractions library? Is there a way to avoid adding it here?

[eerhardt]

Is the RuntimeEnvironment class here a bunch of code copied from the PlatformAbstractions library?

You can think of it as "moved" code from the PlatformAbstractions library. The main things that dotnet/sdk uses from PlatformAbstractions that we don't have APIs for in .NET 5 are:

1. OperatingSystemPlatform
2. OperatingSystemVersion
3. OperatingSystem

You can see the discussion about these APIs at dotnet/runtime#26780.

Is there a way to avoid adding it here?

The only way of avoiding it that I can think of is to change the callsites in dotnet/sdk to no longer use the above 3 APIs.

As discussed in dotnet/runtime#26780 (comment), dotnet/sdk uses these APIs for logging/telemetry purposes, ex. dotnet --info. However, the dotnet/runtime team doesn't think these APIs are generally useful to developers and actually causes developers to use bad habits by checking if you are running on a specific version of an OS. Thus, the APIs were not approved.

Since dotnet/runtime doesn't want to expose these as APIs, the only options are:

1. Put the code here, where it is needed
2. Don't do it at all

So unless we want to change the telemetry information we are sending and change dotnet --info, moving the code here was my only option.

Another potential option would be to use RuntimeInformation.OSDescription instead, but that would still change the format of the telemetry information and the dotnet --info information.

[dsplaisted]

@eerhardt How likely is it that this logic will need to be updated moving forward? Who will be responsible for maintaining it?

Also it looks like it still has its own implementation of getting the RuntimeIdentifier, can we remove that code and use the one that's being added to the BCL?

[wli3]

I am very concerned about this change considering the left over part - RuntimeEnvironment.cs is still so big.

Also there is mostly no test coverage for the entire file -- and it would be hard to test since it varies and requires testing on different machine config -- and that's why it is bad to be in SDK. SDK repo is not setup to test different combination of OSs. If I need to fix a bug in this code, it will be an edit and pray - and find a regression - and fix and pray again situation.

Some ideas:

1. Keep RuntimeEnvironment.cs in runtime repo and produce a library out of it, since it can be better tested there.
2. Add full test coverage to RuntimeEnvironment.cs somehow in SDK
3. RuntimeEnvironment.cs file will still be owned by runtime and log it by this. Considering the repo merge, we no longer have a hard boundary between team and repos, this is normal now.

[eerhardt]

How likely is it that this logic will need to be updated moving forward?

I can't really predict the future, but in the past 2.5 years, this has been the only substantive change: dotnet/core-setup#4085

Who will be responsible for maintaining it?

I'm fine with me or someone from the libraries/runtime team maintaining it. But I don't see it as different than the current get libc version telemetry code we have today.

Also it looks like it still has its own implementation of getting the RuntimeIdentifier, can we remove that code and use the one that's being added to the BCL?

That implementation is used by the GetCurrentRuntimeInformation task in toolset-tasks.csproj, which builds for both netcoreapp and net472. The new API in the BCL is only in netcoreapp.

I am very concerned about this change considering the left over part - RuntimeEnvironment.cs is still so big.

If we can remove the telemetry and dotnet --info functionality that uses this code, we can remove this left over part. For example, do we really need both OS Name and OS Platform in dotnet --info and telemetry? The advantage of putting this code in dotnet/sdk where it is used is if/when we change the telemetry to no longer need this, we can just delete the code. Where if we have this functionality as an official API, we can never remove it.

It looks like some of the version code in RuntimeEnvironment.cs can be refactored to instead use System.Environment.OSVersion. That will make for less code here.

Also there is mostly no test coverage for the entire file

I can add some tests for the functions. For the most part, there are only 3 main code paths: windows, osx, and linux.

Keep RuntimeEnvironment.cs in runtime repo and produce a library out of it, since it can be better tested there.

This is not a viable solution. It is what we have today and what I'm trying to fix. The PlatformAbstractions library is in a weird "limbo" state, where it hasn't been officially approved, but it is in the same repo as the official BCL APIs.

[eerhardt]

@dsplaisted @wli3 - I believe I've addressed your feedback. Please let me know what you think.

[wli3]

I don't see it as different than the current get libc version

I actually made the same comment then that code was checked-in. And it is also maintained by runtime team

[dsplaisted]

I think I am OK with this. The use of this new code is restricted to telemetry and informational output.

As I commented, I think we can get rid of the duplicate GetRuntimeIdentifier implementation altogether.