-
Notifications
You must be signed in to change notification settings - Fork 4.9k
Conversation
|
||
private const string WindowsString = "WINDOWS"; | ||
private const string LinuxString = "LINUX"; | ||
private const string OSXString = "OSX"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also add private const string FreeBSDString = "FreeBSD";
CoreCLR is recently configured to build on FreeBSD with CI build setup: https://github.com/dotnet/coreclr/blob/master/README.md#build-status.
Some folks are already working on corefx to build against FreeBSD (search "freebsd" on GitHub in corefx and coreclr repos). With that said, I think FreeBSD deserves some love here. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See also https://github.com/dotnet/corefx/issues/1576 where naming was discussed within the test suites and #1625 where naming was discussed within the public API.
Important to note that this is the public API contract, if anyone in MSFT wants to choose a different identifier now is the time to speak up. The portteam has pretty much settled on and is happy with using FreeBSD.
private const string FreeBSDString = "FREEBSD"
Additionally you will need to add
public static OSPlatform FreeBSD
{
get
{
return new OSPlatform(FreeBSDString);
}
}
also /cc: @janhenke
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we need FreeBSD to be added to the list of OS platforms. As @ghuntley expressed, our preference is using the "FreeBSD" identifier throughout the code.
The contract System.Runtime.InteropServices.RuntimeInformation provides RuntimeInformation type that has methods to query for underlying OS information at runtime. The OS information is expressed by the OSPlatform type. Reference issue: dotnet\corefx#1017, API Review done on 2015\06\09.
c27acee
to
e84ac58
Compare
(Commenting at the top level instead of on the outdated diff, so the comment is easier to find.) I do not believe that we should add FreeBSD as a predefined property as part of this PR. As we discussed in the framework design review:
Instead, I would like to see the following happen, after merging the PR as is:
I think it's completely reasonable to use #1625 to claim the "FreeBSD" identifier (I would suggest that we use "FREEBSD" since we put the other monikers in all caps). Folks that want to do platform detection can use /cc @stephentoub @richlander. What do the two of you think about this approach? |
Thanks, @ellismg. Well stated, and I fully agree. As part of (2), as you suggest, one of the first libraries implemented on FreeBSD could be this one, to recognize "FREEBSD" as the OS name string, but as you've stated that doesn't require any new public surface area. |
@ellismg That seems like a good approach to me |
I also think that this is a good plan. The initial plan with the port team around CI was similar to this. It isn't about setting arbitrary goals for the port team (in a schedule/milestone sense), but a statement to the community on where a given port is at. We want to make sure that people have a good experience with a port and any sort of official statements from the MS team will lead people to believe that they will. Certainly, we could add this property w/o harm. It's about being consistent. Fair? |
I'm OK with the approach outlined by @ellismg. As long as we have a clean way to do platform-detection "in the meantime", I don't see the need to commit anyone to a permanent API. And like @richlander says, we then avoid having MS send messages about readiness until we're comfortable doing so. |
@KrzysztofCwalina : Can you please review this.. Thanks! |
Any progress on getting this issue merged? The FreeBSD port team is eager to add |
looks good. |
Add OSVersion APIs to .NET Core
private OSPlatform(string osPlatform) | ||
{ | ||
if (osPlatform == null) throw new ArgumentNullException("name"); | ||
if (osPlatform.Length == 0) throw new ArgumentException(SR.Argument_EmptyValue, "name"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Priya91 sorry for not catching this earlier, but the parameter name for the exceptions is wrong here: name osPlatform
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed by commit 2f31aa9
- Adds a new dependency to System.Runtime.InteropServices.RuntimeInformation. More info can be found - On this issue: https://github.com/dotnet/corefx/issues/1017 - On this PR: dotnet/corefx#1999
Add OSVersion APIs to .NET Core Commit migrated from dotnet/corefx@e3296d3
The contract System.Runtime.InteropServices.RuntimeInformation provides
RuntimeInformation type that has methods to query for underlying OS
information at runtime. The OS information is expressed by the OSPlatform type.
Reference issue: dotnet\corefx#1017, API Review done on 2015\06\09.
cc @KrzysztofCwalina @weshaggard @terrajobst @nguerrera @justinvp @ellismg @stephentoub
closes issue #1017