Add new apis proposed in #4137 to System.Runtime.InteropServices.RuntimeInformation #4334
Conversation
Priya91
changed the title
| { | ||
| struct utsname _utsname; | ||
| size_t length = _UTSNAME_LENGTH * 3 + 2; | ||
| char * version = new char[length]; |
There was a problem hiding this comment.
Where does this get freed? We should pass in a buffer from managed code with a single call to get the appropriate length once per process.
| #if defined(ARM) | ||
| return 2; | ||
| #elif defined(BIT64) | ||
| return 1; |
There was a problem hiding this comment.
This isn't the right define. This just means it's a 64-bit arch, which isn't necessarily x64.
There was a problem hiding this comment.
In our config files we are checking if CMAKE_SYSTEM_PROCESSOR is x86_64 string and sets the BIT64 define if true. The cmake_system_processor documentation says this is set using uname -p, which means checking for BIT64 define should mean it is actually x64. Let me know if i understood ur statement incorrectly.
There was a problem hiding this comment.
Yes, for now, but when we add another 64-bit arch we'd set BIT64 there too. See a hint of the future in coreclr repo.
There was a problem hiding this comment.
Removed the dependency on BIT64 and BIT32 defines, added a custom define (X64) for this API purposes, will add X86 and ARM64 in cmake config files, if we start building for those.
Priya91
force-pushed
the
ri
branch
2 times, most recently
from
c408d7c
to
9f891ce
Compare
|
|
||
| public static string GetRuntimeDebugName() | ||
| { | ||
| return RuntimeName; |
There was a problem hiding this comment.
This API should run on full framework as well and should not report CLR in that case.
There was a problem hiding this comment.
What do you expect it to report on full framework?
There was a problem hiding this comment.
"CLR" or maybe we can go all markety and report:
".NET Framework {version}"
".NET Core {version}"
It would be the version of mscorlib in both cases. .NET Native could report something similar.
There was a problem hiding this comment.
I think we are on the same page about having support for CLR. I thought your comment was about supporting full framework but not returning CLR as the name.
There was a problem hiding this comment.
I plan on having a different implementation for desktop and .net native, those changes will go internally. And adding the package through PackageManager will pick the right implementation.
There was a problem hiding this comment.
@Priya91 Just so we're on the same page. Those should be pure IL and not ship with .NET Framework itself right? We wouldn't type forward such a simple type.
Also I think it should be:
".NET Framework {version}"
".NET Core {version}"
".NET Native {version}"
There was a problem hiding this comment.
This is a OOB assembly, so it will not ship with Full Framework.
And on the format of the Runtime string, what is the {version} you are referring to here, the file version or the marketing version or the nuget version. And what value does knowing the version add, as we have already tried to expose the .NET Framework version in desktop, and it ended up being a constant hardcoded value.
I understand knowing if it's CORECLR or CLR or MRT100 lets you know what framework stack for display info, but I am not convinced why you need version. And I can change the runtime names to be the .NET brand names as mentioned in your comment, but without version.
There was a problem hiding this comment.
So that when people copy and paste exception stacks with data. The version number is captured. It's like seeing a watson report with .NET Framework and no version. The more info, the more useful.
|
@dotnet-bot test this please (coreclr config change) |
| { | ||
| x86, | ||
| x64, | ||
| ARM |
There was a problem hiding this comment.
This should be Arm (per our casing guidelines). Also, should we name it Arm32, so we can add Arm64 at some point?
There was a problem hiding this comment.
Arm is the brand name for Arm 32 bit, and when we add Arm64, it will be whatever it's branded as. Will correct the casing.
Priya91
force-pushed
the
ri
branch
2 times, most recently
from
e76f3fd
to
97d52c2
Compare
| "System.Runtime.Extensions": "4.0.0" | ||
| "System.Runtime.Extensions": "4.0.0", | ||
| "System.Runtime.InteropServices": "4.0.0", | ||
| "System.Diagnostics.Process": "4.1.0-beta-*", |
|
@Priya91 What's the state of this PR? |
Just to clarify, Priya's on vacation currently, so I think this is on hold for now. |
|
Can anyone else take this over? |
|
@davidfowl This PR will break internal build as some of the windows APIs used here are not available on onecore-uwp. This requires a simultaneous check-in internally with different implementations. I am not able to connect to TFS sources due to poor net connection. I'll be back on Tuesday, and if it's urgent then @joshfree can redirect this to someone else. |
Priya91
force-pushed
the
ri
branch
3 times, most recently
from
a4e2ea6
to
36888c9
Compare
|
@davidfowl @weshaggard @joshfree Can you review for merge. Rebased all commits, and made changes to prevent TFS build break on mirroring. Will do the rest of the work for UWP internally. |
| { | ||
| RTL_OSVERSIONINFOEX osvi = new RTL_OSVERSIONINFOEX(); | ||
| osvi.dwOSVersionInfoSize = (uint)Marshal.SizeOf(osvi); | ||
| string version = "Microsoft Windows"; |
|
LGTM |
|
All we need is OSVersion next 😄 |
|
PR unrelated failure in @dotnet-bot test this please |
System.Runtime.InteropServices.RuntimeInformation.
|
Test Innerloop Ubuntu Debug Build and Test |
|
Test Innerloop OSX Debug Build and Test |
|
You are probably still going to hit the OSX failure until #4842 is merged |
|
It failed previously due to a test timeout in Sockets. If this fails again, will check for the Vector test failure. Thanks! |
Add new apis proposed in #4137 to System.Runtime.InteropServices.RuntimeInformation
API Review: #4137
cc @joshfree @weshaggard @davidfowl @nguerrera @stephentoub