Closed blobor closed 4 years ago
This seems like a reasonable addition to me. I know we are nervous to add back version properties, but some way to figure out if you are on Linux/OSX/Windows seems reasonable.
Thank you :+1:
I've seen a similar request for ARM32 vs other platforms, checking pointer size is not good enough in that situation.
I would have upvoted this issue if it were possible =) I have a project that uses quite a few Environment.OSVersion.Platform
checks in the common code that's supposed to work on both Windows and Linux, so missing this API would really hurt. (Don't really care for the Version
, VersionString
and ServicePack
properties though.)
I think we do need to expose something like the Platform your are running on (Linux/Windows/Mac) and the processor.
For example, Kestrel does the following to figure out if they are running on windows or not:
public static bool IsWindows()
{
#if DNXCORE50
return true;
#else
var p = (int)Environment.OSVersion.Platform;
return (p != 4) && (p != 6) && (p != 128);
#endif
}
Which is completely busted when you want to run .NET Core on CoreCLR. If we don't provide an API I can imagine the next best thing being just trying to P/Invoke to random Windows or Unix APIs and determine the answer that way.
I have also seen cases where parts of ASP.NET vNext sniff PROCESSOR_ARCHIECTURE environment variable to detect x86 vs x64. I have been moving that code to use IntPtr.Size, but as David says, there's no way to figure out ARM or not with that.
Exposing the desktop types in cloud platform, is not desired, as we need to keep adding support back to desktop. A good way to go will be to create a new contract, with the following api-definition:
namespace System.XPlatform
{
public struct OSName
{
public OSName(string osName) { }
public static OSName Windows { get; }
public static OSName Linux { get; }
public static OSName OSX { get; }
}
public static class RuntimeInfo
{
public static bool IsOSPlatform(OSName osName) { return default(bool); }
}
}
cc @KrzysztofCwalina @ellismg @weshaggard @davkean @stephentoub : If this api design is agreeable i'll start implementing this.
Can you help me understand that conclusion? Adding enum values to .NET Framework ("desktop") is extremely easy, especially given NET Framework will never have to return them or plumb them through. In fact, it already contains the values (Unix/Linux, MacOS and Win32NT) that currently we're planning to port to.
Adding this into a new assembly is even more problematic, and we're now forced to introduce a new library (with one type!) that we'll need to build for each platform, even though we're already building mscorlib/CoreCLR for each one and it already contains the APIs.
I believe we should stick with the previous plan and expose the existing APIs.
cc @terrajobst @nguerrera @jaredpar @ericstj the rest of the Framework Design Core.
Can you help me understand that conclusion? Adding enum values to .NET Framework ("desktop") is extremely easy, especially given NET Framework will never have to return them or plumb them through. In fact, it already contains the values (Unix/Linux, MacOS and Win32NT) that currently we're planning to port to.
The BSD port of CoreCLR is underway by the community now and they are making good progress. I assume as soon as they are done, we will want to be answer the question "am I running on BSD". So we would have to version the contract anyway. With the above design we can say call RuntimeInfo.IsOSPlatform(new OSName("BSD")) even if we can't quickly rev the contract.
Adding this into a new assembly is even more problematic, and we're now forced to introduce a new library (with one type!) that we'll need to build for each platform, even though we're already building mscorlib/CoreCLR for each one and it already contains the APIs.
Not having a dependency on the runtime when possible feels like a win to me. I don't understand the pushback about having to build multiple times. We're already going to be in that boat for many pieces of CoreFX.
I also see no reason why long term the implementation of the contract could not be a façade over the runtime itself, if we find that that's the correct factoring.
Not having a dependency on the runtime when possible feels like a win to me. I don't understand the pushback about having to build multiple times.
For every port to a new OS, the runtime is going to have to rev, so I'm not understanding that concern. It's not just building multiple times; it's creating the contract, the multiple projects, NuGet packages, etc. It feels like we're adding complexity to an area that is not needed. The APIs already exist and given the large amount of code already in mscorlib that probably needs to do something similar, we'd probably need to keep them around anyway - or put these new APIs into mscorib.
With the above design we can say call RuntimeInfo.IsOSPlatform(new OSName("BSD")) even if we can't quickly rev the contract.
You can achieve the same thing with enums OSVersion.Platform == (PlatformID)10
. In the same way that '10' is a magic handshake, so is the hardcoded BSD.
@Priya91 I really like what you have here. My only piece of feedback is around the namespace. Could we use something like System.Runtime.InteropServices
instead? Do you have an idea for the contract name itself?
@ellismg : Yes, system.runtime.interopservices sounds good. Since these apis relate to runtime environment information how about System.Runtime.Environment..
I like that a lot!
(Side note that will clash with Environment class in the System namespace)
@davkean What clashes? If the contract is called System.Runtime.Environment?
Ah, I thought you were talking about the namespace.
Ok, sounds like we have reached a conclusion with the latest proposal. I'll mark the issue with accepting PRs. Going along with the API review process @blobor: will you be providing PR, or only contributing the issue?
The API looks good overall to me as well but I was curious about why you need the OSName struct? Is there a particular reason that is necessary instead of just using a string? We could still use OSName as a place holder for predefined sets.
@weshaggard : Any string parsing that has to be done, can be abstracted in OSName instead of IsOSPlatform, moreover, we expect a string that represents an OSName, instead of any string.
It also helps with discoverability. You know you need an OSName and that guides you to the static properties.
How is this supposed to work under the hood, i.e. will it end up calling uname
?
Linux also seems to be quite broad (there are many different flavors), are there plans to expose more detailed info?
Just to confirm I understand this correctly: this can't be put into Environment
because it'd mean it needs to be backported into Desktop .NET ?
How is this supposed to work under the hood, i.e. will it end up calling uname ? Linux also seems to be quite broad (there are many different flavors), are there plans to expose more detailed info?
The current thinking is we would provide a different implementation assembly for each platform, which would hard-code the result, so we wouldn't call uname. We don't have current plans to expose something like Distro information from this type, but that's more because we don't understand the scenarios where one might want to do that.
We are hesitant, however, to do something like expose a Version of the OS your are running on.
Just to confirm I understand this correctly: this can't be put into Environment because it'd mean it needs to be backported into Desktop .NET?
Yeah, that is the basic problem.
@Priya91 For now, only contributing the issue.
I'd vote for just exposing the existing Environment.OSVersion.Platform
API. This has been in .NET forever; introducing a new API that does the same thing is going to confuse and annoy developers.
@justinvp wanted to avoid the use of enum
for the platform portion which is how OperatingSystem
defines the platform.
@jaredpar For reasons I still don't understand. :)
I also don't fully grok why all of this is necessary.
@ellismg can you summarize the problems again?
Frankly, I don't see how the proposed API is better than the old one. From the readability point of view, OSName.Windows
is not that different from PlatformID.Windows
, and all these values are going to be exposed anyway, whether through static properties or through enum values. However, using enum simplifies discovery a bit: you immediately have Intellisense suggesting correct values. Compare this to the OSName
experience: the first intuitive thing to do when you need to provide an instance is to call a constructor... which needs a name for some reason. Is it "Windows"
? "windows"
? "Windows 8.1"
? "Microsoft Windows [Version 6.3.9600]"
? "Linux"
? "Unix"
? "Ubuntu"
? This is going to be confusing until you stumble upon those static properties one way or another.
And a separate obscure class somewhere in System.Runtime.InteropServices
isn't making this API more discoverable either. I wasn't googling Environment.OSVersion
when I needed it, I just followed an obvious hunch that if there's an API that exposes the current platform, it's most likely located in the Environment
class - and that's exactly where I found it.
@HellBrick :+1: Agree 100%
Can we have some of the fxdc members who were in the room when the decision was made (@weshaggard @KrzysztofCwalina @ellismg) please respond back to the thread around how this conclusion was reached?
@terrajobst for context.
@ellismg can you summarize the problems again?
I can try.
There are a few problems here. Some are pure design issues, some are reflections of our goals with the project and where we are in the engineering cycle with other platforms.
The basic problem is we expect that we will need to continue to add to the list of supported platforms at a quick cadence. For example, some folks are making good progress on porting CoreCLR to BSD, what is going to happen when we want to start being able to ask the question "am I running on BSD?"
With the enum based approach, we have a few options. We can either:
OperatingSystem bsd = (OperatingSystem)7;
In their code.
With the new type, we completely decouple ourselves from the versioning concerns of the full .NET Framework. This will give us more agility to update the contract, while still building something that can run on the full .NET Framework. In the cases where there may be a lag in updating the predefined list, we can always say do something like new OSName("bsd")
until we are able to add a BSD Property to OSName. We felt this expresses intent better than (OperatingSystem)7.
FWIW, the enum as it exists today is not great. What does "Unix" mean? Is it a synonymy for "Linux"? Do we introduce a new "Linux" value to compliment "OSX"?
In addition, we've generally tried to move away from Enums for cases where we have an unbounded set of options that we foresee growing regularly. We use a similar pattern (a struct which wraps a string) in some new Cryptography APIs we have been designing to be used as names for algorithms.
In addition to the above, the are some additional issues that make it hard to "just expose OperatingSystem" for .NET Core 1.0. It's not clear that we could actually get this change pulled back into the full .NET Framework in time for 4.6 (again, due to forces outside our control the .NET Framework ships facades for the contracts we'd want to update as part of this change and given where the product is in its release cycle pushing the updates through is very, very, difficult).
I totally understand the downsides of this approach. Introducing a new type to solve a problem that an existing type solves is something we try not to do. There was a lot of discussion internally around if that was the right thing and there's strong views on each side. If we didn't envision having to add a bunch of OperatingSystems in the future, we would probably fight much harder to just expose what we have an move on. But the difficultly of simply exposing what we already have coupled with the fact that the versioning is less than ideal at a time when we expect we will need to update the list of supported systems quickly pushed us down this path.
Thanks for the explanation @ellismg I think that covers most of the concerns we discussed. One other aspect we talked about is we feel it is better to ask the question of "am I running on this OS" rather than to do an exact comparison via Environment.OSVersion.PlatformId == PlatformId.Unix. This gives us the freedom to answer true for both IsOS("unix") and IsOS("osx") when running on OSX. That allows for people to be able to ask more general or more specific questions without having to come up with various different PlatformId check combinations that will not work in the future.
@weshaggard The code submitted to implement this feature doesn't have a Unix entry, so that would be an oversight that it's missing? If not, not sure I agree with throwing out existing API for speculate support for something that might happen in the future.
I agree with the point about choosing IsOS( Whatever )
over OS == Whatever
. However, I still fail to see the benefit of using string over enum. As far as I understand, the classic .NET Framework is supposed to lag behind .NET Core, which means if a programmer wants his code to run on both .NET Framework and .NET Core he or she won't be able to use some of the newest stuff anyway and will have to resort to some way of moving that newest stuff into his own code. So if we want to write some BSD-specific code that works on .NET Framework as well, we'll most likely just move the constant value for a platform .NET Framework doesn't know of yet to a helper field to avoid hardcoding it throughout the source code, whether its type is enum or string or some special struct:
public static class OS
{
public const OperatingSystem BSD = (OperatingSystem) 7;
}
or
public static class OS
{
public static readonly OSName BSD = new OSName( "bsd" );
}
I don't think the value of the constant (7
vs "bsd"
) really matters in this case, since it's gonna be written once in a field declaration and then forgotten. However, I do think that the enum advantages I've mentioned in my previous comment matter, as they make the majority of the use cases easier. So I think something like this would be better:
public enum OSKind
{
Windows = 1, Linux = 2, OSX = 3, Unix = 4, BSD = 7 //, etc.
}
public static class CurrentOperatingSystem
{
public static bool Is( OSKind kind )
{
// platform-specific implementation
}
}
@ellismg,
The basic problem is we expect that we will need to continue to add to the list of supported platforms at a quick cadence.
I'm curious, besides BSD, what additional platforms do you realistically expect to be adding at a quick cadence? I could see the possibility of adding iOS and Android at some point, but what other platforms are on the immediate horizon?
In addition to the above, the are some additional issues that make it hard to "just expose OperatingSystem" for .NET Core 1.0
What are the additional issues?
We use a similar pattern (a struct which wraps a string) in some new Cryptography APIs we have been designing to be used as names for algorithms.
Off-topic, but Is there an issue (with speclet) for the new Crypto APIs, or is this feature being discussed/designed behind closed doors?
Also, why wasn't there a speclet for this feature? Is the preference now to just open a pull request and do the API naming/design discussion there, instead of opening an issue w/ speclet first?
The basic problem is we expect that we will need to continue to add to the list of supported platforms at a quick cadence.
I'm curious, besides BSD, what additional platforms do you realistically expect to be adding at a quick cadence? I could see the possibility of adding iOS and Android at some point, but what other platforms are on the immediate horizon?
This is what we discussed in the meeting as possibilities. New platforms for .NET core are very much a "wherever the community wants to take it sort of thing" right now.
In addition to the above, the are some additional issues that make it hard to "just expose OperatingSystem" for .NET Core 1.0
What are the additional issues?
It would require us to rev the facades for the contracts we ship on the full .NET Framework which is hard to do right now given where the full .NET Framework release is.
Off-topic, but Is there an issue (with speclet) for the new Crypto APIs, or is this feature being discussed/designed behind closed doors?
It's happening "behind closed doors" because we aren't sure exactly the scope of work we want to do yet or where it will start.
Also, why wasn't there a speclet for this feature? Is the preference now to just open a pull request and do the API naming/design discussion there, instead of opening an issue w/ speclet first?
No, we more or less did the wrong thing and that's my fault. Mea Culpa. Basically, this started by thinking we would just expose the existing Desktop APIs. We went in to the API review and started discussing the versioning concerns and the interactions with the desktop timeline and decided we should go towards a new API. I asked @Priya91 to sketch out a design (which she did as a comment in this issue), there was some discussion and when it looked like we reached rough consensus a few days later she opened the PR. Then the discussion really kicked off.
It probably would have been better if we had closed this issue and opened a new one with a clear title about a new API. I was pushing hard for this to get added to the platform for my selfish reasons of wanting the API to use as I bring up more of the stack cross platform and I can see how we could have done better here.
I'm really sorry.
We went in to the API review and started discussing the versioning concerns and the interactions with the desktop timeline and decided we should go towards a new API.
Sorry for another off-topic follow-up question: Are you open to the possibility of being a little more transparent with the internal API review/design discussions? The last set of notes on https://github.com/dotnet/apireviews are from January. In contrast, the language team is regularly posting language design notes. Would it be useful to do the same for API design/review notes?
Are you open to the possibility of being a little more transparent with the internal API review/design discussions?
There isn't really much to share at this point. We're focusing on preparing the bits for Build. Since not everything is in the open yet, a lot of that activity is still happening behind closed doors. As far as API reviews go: our internal meeting is also used for tactics and bug fix level discussion which is mostly focused on getting the release prepped. So I'm afraid what you're seeing publicly (not much) is an accurate representation on what went on in this space :-(
As input to this conversation, DNX has the IRuntimeEnvironment "service" that can be injected into your application by the runtime. It looks something like this:
interface IRuntimeEnvironment
{
string OperatingSystem { get; }
string OperatingSystemVersion { get; }
string RuntimeArchitecture { get; } // x86, x64, etc.
}
Current shape of the APIs:
namespace System.Runtime.InteropServices
{
public struct OSName : IEquatable<OSName>
{
public OSName(string OSName) { }
public static readonly OSName Windows;
public static readonly OSName Linux;
public static readonly OSName OSX;
public bool Equals(OSName other) { }
public override bool Equals(object obj) { }
public override int GetHashCode() { }
public override string ToString() { }
public static bool operator ==(OSName left, OSName right) { }
public static bool operator !=(OSName left, OSName right) { }
}
public static class RuntimeInformation
{
public static bool IsOperatingSystem(OSName osName) { }
}
}
As @bricelam we made an interface mostly because we like the idea that you can also write tests against code that checks what the current OS is.
We also took a look at some other platforms an how they surface it:
How would this API work on full CLR?
@Priya91 what about adding an implicit conversion from string?
The implicit conversion feels bad to me. If that existed, folks might try:
RuntimeInformation.IsOperatingSystem("Linux");
or RuntimeInformation.IsOperatingSystem("linux");
and be confused when one works and the other doesn't. We want to push folks towards the static members and only have the construct from string sample be used in exceptional cases where the new static member has not yet been added to the contract.
We want to push folks towards the static members and only have the construct from string sample be used in exceptional cases where the new static member has not yet been added to the contract.
That makes me kinda sad :cry:
We want to push folks towards the static members and only have the construct from string sample be used in exceptional cases where the new static member has not yet been added to the contract.
If the goal is to encourage the use of the static members, there is a minor API design issue with the current shape of the API. When developers go to use RuntimeInformation.IsOperatingSystem
, they'll see that it takes an OSName
parameter. Some developers (perhaps many) will then "new-up" an OSName
by calling the OSName
constructor, passing in a string name, unaware of any static properties.
To make the static properties more discoverable and encourage their use, the OSName
constructor should be private, replaced with a public static factory method. This same approach is used by System.Drawing.Color
, as an example.
Something like:
public struct OSName : IEquatable<OSName>
{
public static readonly OSName Windows;
public static readonly OSName Linux;
public static readonly OSName OSX;
public static OSName Create(string name);
...
}
Create
might be too attractive for the name of the factory method. A less attractive name like FromName
(or something else) might be better to discourage its use (it's only there for the rare exceptional circumstance).
Caveat: since this is a struct, developers will still be able to create an OSName
by calling new OSName()
(or default(OSName)
). But when they attempt this, they won't get any help from intellisense (no documentation and no parameters to pass), and it will be a useless OSName
, which will help lead them to the static members.
Is everyone happy with the naming of this? Personally, I think we can do better.
Usage with the current naming:
if (RuntimeInformation.IsOperatingSystem(OSName.Linux)) { ... }
Questions:
OperatingSystem
spelled out for IsOperatingSystem
, but abbreviated as OS
for OSName
?OSName
have a superfluous Name
suffix?Here are some alternatives:
if (RuntimeEnvironment.IsPlatform(Platform.Linux)) { ... }
if (RuntimeEnvironment.IsOS(OS.Linux)) { ... }
if (RuntimeEnvironment.IsOS(OSName.Linux)) { ... }
if (RuntimeEnvironment.IsOperatingSystem(OperatingSystem.Linux)) { ... }
if (RuntimeEnvironment.IsOperatingSystem(OperatingSystemName.Linux)) { ... }
if (RuntimeInformation.IsPlatform(Platform.Linux)) { ... }
if (RuntimeInformation.IsOS(OS.Linux)) { ... }
if (RuntimeInformation.IsOS(OSName.Linux)) { ... }
if (RuntimeInformation.IsOperatingSystem(OperatingSystem.Linux)) { ... }
if (RuntimeInformation.IsOperatingSystem(OperatingSystemName.Linux)) { ... }
Any others?
How I can get current OS name? I need to call IsOperatingSystem method with different os names until I get true? Or I should use RuntimeEnvironment class?
@justinvp I like the idea of a private constructor and a static factory method to build them to nudge folks towards the fields.
@blobor Why do you want to get the OS name?
@ellismg Run specific code depending on OS.
@blobor The proposed API let's you answer the question: "Am I running on this OS", which can also let you do this, is there a reason it's not sufficient?
@ellismg It's ok. But in case we to do specific action for each supported OS. With this approach we should write bunch of code with "if" and "else if". But if we have just OS name, we could use switch for it.
I want to write cross-platform class library based on .NET Core. I need to invoke native libraries in Windows (kernel32) and in UNIX (libc).
Currently Environment class is missing OSversion property. Is there some way to determine in what OS currently my assembly is running?