Move Environment and friends to corefx #9851
Conversation
|
Yay for OSVersion! resolves https://github.com/dotnet/corefx/issues/4741 |
| public int dwMinorVersion; | ||
| public int dwBuildNumber; | ||
| public int dwPlatformId; | ||
| [MarshalAs(UnmanagedType.ByValTStr, SizeConst = 128)] |
There was a problem hiding this comment.
public fixed char szCSDVersion[128]; would be better - faster, and you can also use sizeof(OSVERSIONINFOEX) instead of the slow Marshal.SizeOf to initialize the size.
Currently System.Runtime.Extensions type forwards to the E 10000 nvironment in the runtime, which means we need to implement functionality in both coreclr and corert. It's also missing functionality, which means to add that functionality back we'd need to do so in both runtimes, and we'd miss out on sharing code with elsewhere in corefx. This commit: 1) Adds Environment, EnvironmentVariableTarget, OperatingSystem, and PlatformID to corefx so that System.Runtime.Extensions contains the type rather than forwarding to it. 2) Adds implementations of all of the missing members: CommandLine, CurrentDirectory, ExitCode, Is64BitProcess, Is64BitOperatingSystem, OSVersion, SystemPageSize, UserInteractive, UserName, UserDomainName, Version, WorkingSet, GetLogicalDrives, GetFolderPath, and the overloads of Get/SetEnvironmentVariable(s) that take an EnvironmentVariableTarget. 3) Adds the additional members to the contract. 4) Adds some basic tests to ensure basic functionality works. Work that still needs to be done after this: 1) Add an EnvironmentAugments type to System.Private.Corelib in both corefx and corert, use that to replace the reflection-based hack in this commit, then remove the current Environment from Corelib's model.xml so that the rewriter will make it internal. As part of this, we need to determine how to handle "IsAppXmodel/DesignMode" from corefx, whether we need two different builds for Win32/WinRT, etc. 2) Fix Get/SetEnvironmentVariable(s) to not ignore the EnvironmentVariableTarget. This should be doable once (1) is done. 3) Add more support to the Unix implementation of GetFolderPath for more special folders 4) Add a GetTickCount method to the System.Native native lib, ala the implementation in libcoreclr; the current implementation is functional but slower than it could be. 5) Implement more Windows functionality if/when missing functions become available: SHGetFolderPath and GetVersionExW. 6) Use GetLogicalProcessInformationEx in ProcessorCount (or delegate to the implementation in coreclr). I will open issues for each of these prior to merging this commit.
| { | ||
| // First try with a buffer that should suffice for 99% of cases. | ||
| string username; | ||
| const int BufLen = 1024; |
There was a problem hiding this comment.
How do you guys test the fallback path for these things? In the past, I've used #if DEBUG to set the initial buffer to zero in debug only.
There was a problem hiding this comment.
This was a good idea, and actually helped find a bug in our native implementation (which I fixed), but I'm going to undo the ifdef as it's bumping up against what appears to be a known issue on CentOS:
https://bugs.centos.org/view.php?id=7324
and the associated test is failing.
ca9e711 to
043632f
Compare
|
Thanks for the review. I pushed another commit that addresses the feedback and fixes up additional things. Main changes:
plus a smattering of others. |
PR feedback: - Remove AppX checks and split Environment.Windows into Environment.Windows, Environment.CoreCLR, and Environment.NETNative - Change marshaling of service pack version field in OSVERSIONINFOEX - Forward TickCount to EnvironmentAugments rather than implementing it via a P/Invoke - Move environment variables support from coreclr to corefx Plus: - Use GetLogicalProcessorInformationEx to get the processor count, and cache it - Use GetUserNameExW instead of GetUserNameW, as it's available on more platforms - Fix compilation for .NET Native - Fixed a test
043632f to
282ad11
Compare
| @@ -106,16 +143,42 @@ | |||
| </ItemGroup> | |||
| <!-- WINDOWS: CoreCLR --> | |||
| <ItemGroup Condition=" '$(TargetsWindows)' == 'true' And '$(CoreClrOrCorRt)' == 'true'"> | |||
| <Compile Include="System\Environment.CoreCLR.cs" /> | |||
There was a problem hiding this comment.
This is not CoreCLR specific. The conventions used elsewhere are:
- .Windows.cs: Windows-specific implementation, usable for both classic and UWP apps
- .Win32.cs: Implementation specific for classic apps
- .WinRT.cs: Implementation specific for UWP apps
There was a problem hiding this comment.
In other words, we want this implementation to be usable for CoreRT console apps - replacing what's there today (https://github.com/dotnet/corert/blob/master/src/System.Private.CoreLib/src/System/Environment.Win32.cs).
There was a problem hiding this comment.
I guess I'm confused when we're choosing to use .NETNative.cs/.CoreCLR.cs vs .Win32.cs/.WinRT.cs (we have a bunch of files in the corefx repo that use the former convention, and I was just following that). But I'll switch these.
There was a problem hiding this comment.
There are several different axes:
- Classic win32 vs. UWP apps (vs. non-Windows OSes). The two Windows appmodels are very much like different OSes because of there are different APIs available and things have to be generally done differently in number of places. I believe that the most common pattern has been to use
.Win32.csvs..WinRT.csto provide different implementations.System.IO.FileSystemorSystem.Consoleare good examples. - JIT vs. full AOT. I do not actually see any good examples.
System.Linq.Expressionsor data contract serialization where this difference is most prelevant are configured via ifdefs, not via alternative file includes. - Different surface of System.Private.CoreLib between CoreCLR and CoreRT/.NET Native. The common pattern for this is
.CoreCLR.csvs..netcore50.csor.netcore50aot.cs. Hopefully, most of these will go away over time as we work on filling in the missing APIs and converging the implementations.
I know there have been a ton of confusion and inconsistencies about this. We have been getting by so far because of we support just a small subset of combinations (CoreCLR/Classic, CoreCLR/UWP, CoreRT/UWP). We run into it during OSS CoreRT project bring up because it needs a new combination (CoreRT/Classic). We ended up hacking around it because of getting this cleaned up (especially wrt packaging) seemed to be a lot of work.
I am going to open an issue on fixing consistency of the .cs file names at least to make some incremental progress on this.
|
LGTM so far, modulo comments. |
abfe734 to
a71bae1
Compare
|
Thanks for all the good feedback, @jkotas. Addressed. @ianhays, FYI, this also fixes https://github.com/dotnet/corefx/issues/3568. |
Main changes: - Rename Environment Windows files to .Win32.cs and .WinRT.cs - Refactor environment variable functions to help with tree shaking - Fix environ access on macOS - Remove direct Environment dependency from System.Diagnostics.Debug causing ambiguity
a71bae1 to
8ea801c
Compare
|
|
||
| object IDictionary.this[object key] | ||
| { | ||
| get { return this[(TKey)key]; } |
There was a problem hiding this comment.
Instead of throwing KeyNotFoundException, this should return null if the key isn't found.
UserName test is failing on debug (with the initial buffer size set to 1) due to what appears to be a CentOS bug: https://bugs.centos.org/view.php?id=7324
|
Test Innerloop CentOS7.1 Release Build and Test please |
|
Test Innerloop CentOS7.1 Release Build and Test please ("failed to mkdirs") |
| WinCE = 3, | ||
| Unix = 4, | ||
| Xbox = 5, | ||
| MacOSX = 6 |
There was a problem hiding this comment.
This is the existing name. We can't change it, as part of the goal here is binary compatibility. In the future we could potentially add another name with the same value.
|
@stephentoub any ideas when the next release will go out with this? |
I'm not sure. @richlander or @joshfree may be able to comment. At the moment it's also in a separate branch, where lots of APIs are being added back, but I expect/hope we'll see that moved over to master soon and then have additional API work happen there. @danmosemsft may be able to comment on that. |
@niemyjski https://github.com/dotnet/corefx/milestones has our most current info |
…ment_corefx Move Environment and friends to corefx Commit migrated from dotnet/corefx@c4af76e
Currently System.Runtime.Extensions type forwards to the Environment in the runtime, which means we need to implement functionality in both coreclr and corert. It's also missing functionality, which means to add that functionality back we'd need to do so in both runtimes, and we'd miss out on sharing code with elsewhere in corefx.
This commit:
Work that still needs to be done after this:
I will open issues for each of these prior to merging this commit.
cc: @jkotas, @weshaggard, @danmosemsft, @KrzysztofCwalina, @ianhays
Fixes #9826
Fixes #5407
Fixes #4741
Fixes #3568