-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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 appropriate parts of Microsoft.DotNet.PlatformAbstractions.RuntimeEnvironment into the shared framework #26780
Comments
FYI. @eerhardt |
@nguerrera, @KathleenDollard, @livarcocc, @dsplaisted. I know there has been a similar request on the SDK/CLI team before |
We already have other related issues like https://github.com/dotnet/corefx/issues/28620. |
Is just getting the current RID helpful? Or do you usually also need to find a best match from a list of RIDs you have resources available for, using the RID graph? |
Yes, getting the best match from a list of RIDs is even better. Not sure how closely tied that is to NuGet, however. |
@weshaggard, right. But I didn't see anything that looked like the "recommended" API proposal layout and that covered the |
See my comments here: https://github.com/dotnet/corefx/issues/28620#issuecomment-377421773, copied below for ease in reading:
The other public members of RuntimeEnvironment: public static Platform OperatingSystemPlatform { get; } = PlatformApis.GetOSPlatform();
public static string OperatingSystemVersion { get; } = PlatformApis.GetOSVersion();
public static string OperatingSystem { get; } = PlatformApis.GetOSName();
public static string RuntimeArchitecture { get; } = GetArch(); The following properties aren't already exposed in corefx:
The following properties are dupes of corefx APIs:
|
One question I have about the RID API: Today, we have an environment variable that allows someone to set the RID: private static readonly string OverrideEnvironmentVariableName = "DOTNET_RUNTIME_ID"; Do we plan on preserving this behavior with the proposed API? And if we do, would it make more sense for the API to be a method instead of a property? Since it would always need to check the env var. |
I think a method makes sense, if the behavior is preserved. Otherwise, a property seems the better choice. |
In general I don't like the idea of applications taking functionality dependency based on reported OS version at runtime: there is a history of broken features because the OS can fake its answers, or later starts to support some feature and the app then requires recompilation. If possible testing/trying the feature itself is the preferred way. That said decisions based on broad categories (e.g.: Windows vs Unix) or more fine grained for tests, not production, are reasonable. Of course using RID info is totally fine in the context of building/packaging/etc. Do we really need this as a public API? Can we instead bring it down to some place that provides the gains for building/packaging without exposing a public API? |
I don't think it is a great idea for code to take functionality dependency based on reported OS version at runtime either. But the fact of the matter is, it happens. The RID has the OS version inside of it. So anything you do with the RID will now mean you are dependent on the reported OS version. We've been down the "do we need the OS Version public API?" path before, and it wasn't added as a "corefx" API previously. And that decision is what led to the creation of the PlatformAbstractions library in the first place. So whether we like it or not, we already have this public API. The issue is the API lives outside of corefx, and outside the "typical" place where people look for this type of information - the Here are other places where people are asking for OS version: https://github.com/dotnet/corefx/issues/16629 (note this isn't an exhaustive list, I stopped looking as I think the point is made) The main usage of
I know that we already have Lastly, I think these methods/properties ( p.s.
|
Thanks for digging the history and showing that this was considered many times before 😄 you convinced me. |
I've added to the proposal to include public struct RuntimeIdentifier
{
public string Id { get; }
public string OperatingSystem { get; }
public string OperatingSystemVersion { get; }
} or potentially just a completely new public static class RuntimeIdentifier
{
public static string GetCurrent();
public static string OperatingSystem { get; }
public static string OperatingSystemVersion { get; }
} This would convey that the |
I think it might make some since to have a type for RuntimeIdentifier if we plan to support other scenarios like checking compatibility with other RIDs, if we only care about getting the current RID then I'd stick with a static class. I do think we need to be a little careful about making assumptions about the structure of a RID. While it is true it generally contains an OS and version it isn't always the case as a RID is just an string. It also commonly contains the architecture so if you are breaking it apart you may want to add that value as well.
We should consider actually fixing that if we can someone embed the RID graph we could use that information to check these different hierarchies. |
This is something that would be useful, especially when writing bindings for other native libraries. Has there been any update on this? |
Updated the api proposal and changed the label to |
Is there any API that returns the OSPlatform object representing current platform? For example an API that could expose the value of |
What do we expect this API to be used for? Is this API useful without the library that allows you to reason about the RID graph? The RIDs come with a lot of problems because of they form a graph. You cannot really do anything useful with the RID without having the library that is able to deal with the RID graph. |
@ViktorHofer Is that basically the proposed OSName API, or is OSName something else from OSPlatform? Could you please update the proposal with representative set of examples of values that each of the APIs return to make it clear what they do? |
The move of Microsoft.DotNet.PlatformAbstractions out of dotnet/core-setup, which partly motivates this API, is tracked in https://github.com/dotnet/core-setup/issues/5213 |
The |
No, that is not the proposed OSName. The The proposed OSName is something like
We have it today in
Here is ASP.NET's build system calling it. The CLI uses it to print it out during Today, you can reason about the RID graph by doing something similar to what the CLI does here. We can add more "RID-graph reasoning" APIs in the future, if necessary. But starting with "what is the current machine's RID?" seems like a good starting point. |
It has to return exactly what the ".NET runtime" thinks is the RID of the machine. This API and the
That "hack" has proven useful very often. Just recently, it was proven useful because we didn't have alpine 3.8 in the RID graph. |
This is old code in a private repo. I do not see it in live ASP.NET Core. Also, it is a great example that shows why the RID alone is useless. It has ad-hoc list of cases to compensate for when the RID is not what you want. Should the API to give you best guess about the current RID be rather part of the RID graph reasoning library? |
Sorry, that was a bad link to the ASP.NET build system. Here is the real link: https://github.com/aspnet/BuildTools/blob/b7b88d08d55abc8b71de9abf16e26fc713e332cd/src/ApiCheck.Console/NuGet/RuntimeGraph.cs#L93 Here is
No, I believe it needs to be sync'd with what the runtime thinks is the current RID. Again, if these are different values (what the runtime thinks, and what this API produces) we are going to have problems. |
Ok, so these APIs are really just tunnel to the host.
|
On
For use cases like the CLI showing it during |
|
How about Mono? I thought they used the libraries in Also note the breaking change here if we go through with https://github.com/dotnet/core-setup/issues/5213. Previously you could call this API on |
Once Mono chooses to implement .NET Core App 3.0 API set, they will implement this API too.
They will not get this API. |
The fallback really means "don't know". If you pass |
The issue in the API design meeting regarding this API being defined as: public static string? RuntimeIdentifier { get; } Is that it is an exceptional case when the AppContext variable isn't set. When an app is launched by our app host, the
I would think that depends on the API. Since However, the other API is probably defined as
And we couldn't say The current RuntimeEnvironment API returns |
That's not the only case. The broader .NET ecosystem includes e.g. Unity that has its own runtime and number of Unity target platforms do not even have well-defined RIDs by design. I am fine with this being implemented as `GetData("RUNTIME_IDENTIFIER") ?? "any"`` (or "unknown" if that works better). |
I think the primary conclusion is that we'd consider such hosts as having a bug. Frankly, I'd even go so far and say we should crash on startup when a RID isn't specified. Or said differently, for this API to be reliable, the host must set it meaningfully. That's why we should force the change there, rather than asking everyone to guard for |
If by native we also consider the control flow stemming from
By the time we get any version working on other OS, it is already too late in lifetime of that version, due to breaking changes in infra for the next version. IMO, keep RID story super simple; restrictions wise and otherwise so it can scale better in what it was originally designed to do. This is mitigate some pain points in porting work for vast range of targets. |
@terrajobst As @am11 said, the RIDs are ever-engineered, do not scale and major barrier for entry for non-Microsoft driven ports. It is why I believe that it is important that this API is allowed to return "don't know answer". If we dislike |
The host doesn't need to validate the string against the graph; it just needs to return a string that is unique and can be used to identity the runtime. However, for libraries with native dependencies it's important that "sensible" answers are returned. As you said, returning "any" is unlikely to be a good answer. |
I would like to see programs and libraries that try to do their own native library resolution based on RIDs to assume that the native library is supplied by other means for unknown RID. Nobody really needs to their own native library resolution in .NET Core, this is very niche case - often just a left over from .NET Framework. I know that there are a few libraries that do that for "reasons", and I would not feel bad that the potential null is in their face. I would like to avoid the situation we have with |
This value returns the Runtime Identifier (RID) of the current machine. Contributes to dotnet#26780
* Add RuntimeInformation.RuntimeIdentifier This value returns the Runtime Identifier (RID) of the current machine. Contributes to #26780 * Include XML doc comments for the new property. * Consistently check for AppContext strings.
@eerhardt the value returned for From a use-case perspective, it may be interesting to be able to query whether the platform is compatible with an rid. Both above are compatible with |
This API is not doing any filtering against any known lists. It is going to return |
@jkotas Ah, that's good! The processing is limited to some normalization of |
fwiw, since this returns an rid for the OS, |
Today, you can use the framework's If you need a full RID graph (ex.
Yes, it still does that normalization for rhel and alpine.
I believe that would be confusing since everywhere else in the system (CLI options, SDK properties, Documentation, etc) calls this RID or RuntimeIdentifier. |
Now that we have an updated test host, we can enable the RuntimeInformation.RuntimeIdentifier tests that are disabled. Fix dotnet#26780
* Enable RuntimeIdentifierTests Now that we have an updated test host, we can enable the RuntimeInformation.RuntimeIdentifier tests that are disabled. Fix #26780 * Trim quotes from ID in /etc/os-release
Rationale
It is often the case, when working with .NET Core that you want to do something based on the current platform/architecture. In many cases, this can include needing to interop with tools published using a particular "runtime identifier".
Today, the RuntimeIdentifier is only exposed through the
Microsoft.DotNet.PlatformAbstractions.RuntimeEnvironment.GetRuntimeIdentifier()
API and is not exposed as part of CoreFX. This requires referencing an additional package, and prevents it from being used in certain scenarios (such as evaluation time in MSBuild).As such, I propose that we expose a mechanism to get the
RID
for the current execution environment at runtime.Proposed API
The above API will return the value of a new AppContext variable
RUNTIME_IDENTIFIER
. This variable will be passed by thedotnet.exe
host, and can be passed by any other native host when the runtime is loaded.DISCUSSION: Should we still maintain a managed fallback code path for when this AppContext variable isn't present? For example if the app was loaded from a different native host that didn't set this variable.
Additional Notes
This would allow switching on the active
RID
from an MSBuild project/props/targets file, which would allow greater versatility in consuming native tools from a package during build time (or from a user program at runtime).It may be worth reviewing the other APIs exposed via
Microsoft.DotNet.PlatformAbstractions.RuntimeEnvironment
and determining which others, if any, should also be exposed: https://github.com/dotnet/core-setup/blob/master/src/managed/Microsoft.DotNet.PlatformAbstractions/RuntimeEnvironment.csUpdates
eerhardt: Add proposal for
OperatingSystem
andOperatingSystemVersion
soMicrosoft.DotNet.PlatformAbstractions.RuntimeEnvironment
can be completely removed/deprecated/etc.vihofer: Choosing OS over OperatingSystem as we already use OS in
IsOSPlatform
.OperatingSystemPlatform
is already accessible throughIsOSPlatform(Platform)
. RuntimeArchitecture is already exposed asProcessArchitecture
. TheRuntimeVersions
proposal is already covered by dotnet/coreclr#22664 and doesn't require an api review.eerhardt: Remove proposal for
OSName
andOSVersion
, we will drop support for these APIs. Callers can instead callNtdll.RtlGetVersionEx
on Windows, or read/etc/os-release
on Unix.Change
RuntimeIdentifier
from a property to a method call, since the proposed behavior will invoke a method:AppContext.GetData()
, which isn't appropriate for a property.The text was updated successfully, but these errors were encountered: