-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
EnvironmentVariable operations with EnvironmentVariableTarget.User fails on Windows Nano Server #30566
Comments
Setting to 3.0 to make sure that the issue is triaged asap. |
Disabled the test in dotnet/corefx@a969488. |
@ViktorHofer is any exception being thrown here ? do you have the failure\exception message ? |
No exception being thrown in source code. Just assert errors in the different tests that test the behavior. |
@safern and I looked at this previously and were seeing that it's just storing stuff in the registry. I wonder if Nano removed the root key in the registry for user environment variables: https://github.com/dotnet/corefx/blob/954b1fdab8e98060cce3d0d88a34c3b0587a6acb/src/Common/src/CoreLib/System/Environment.Win32.cs#L51 @maryamariyan do you think you can take a look? |
This may have to move out of 3.0 now given there's no customer reports (?) @ViktorHofer was it previously passing on a different version of Nano, and the upgrade broke it? |
Yes, the upgrade to Windows Nano Server 1809 broke this. I assume the number of customers that use .NET Core on Nano is limited. |
We should look at the api changes in Nano 1809 and/or talk with the Windows team to understand the issue better. This isn't 3.0 specific so I'm fine moving it out but it probably affects all .NET Core versions that support Nano and use that API. |
I ran the test on Windows 10, Windows Nano Server 1809 and 1803. The repro (failure for SetEnvironmentVariable) only happened in 1809. After manually adding that in 1809, the test passes for Nano Server 1809 as well. |
Sounds like we could reasonably work around that in code then (eg create hte key or skip the operation or whatever) |
Skipping the operation is what happens now: that's the bug. We should probably let the Windows folks know about this regression and confirm that removal of the key was intentional. We could create the key, assuming we can get the ACLs right. On my machine it looks like this key has an explicit ACL (not inherited). We'd want to test what would happen when we create the key from elevated and non-elevated process (if such a thing exists in nano) to ensure we create the key with appropriate ACL. We should test if we add the key, and create a new process, that the variable will be set in that process. Given Nano removed creation of the key, we need to confirm they didn't remove consumption of the key too. |
Creating the key ourselves is a bit scary, let's first talk with Windows. |
Not blocking release of 5.0 product |
I'm trying to bring up checked legs for the NativeAOT runtime and running into the same assert this is tracking (#79847). I don't believe it's NativeAOT specific because it only happens on Nano server. My theory is that we just don't have any test legs that would detect this with CoreCLR anymore. I'm going to disable the entire System.Runtime.Extensions suite on the checked legs because I've already spent too much time on this 3.5 year old known bug. It would be nice if this got attention from the maintainers in 8.0. |
We do not have a leg that runs libraries tests on Windows Nano Server as admin with checked CoreLib. NativeAOT happens to have some coverage for this specific config by accident. |
* Ignore SendMessageTimeout failures on Windows Nano Server * Fix tests Fixes #30566
This seems to be a product bug because of an underlying windows api change.
cc @Anipik @stephentoub @danmosemsft
The text was updated successfully, but these errors were encountered: