Skip to content
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

Can we avoid resetting all env vars for some remotely executed tests? #100505

Open
AndyAyersMS opened this issue Apr 2, 2024 · 7 comments
Open
Assignees
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Milestone

Comments

@AndyAyersMS
Copy link
Member

The environment clearing done here:

private static void RunUsingInvariantCulture(Action action)
{
Assert.True(CanTestInvariantCulture);
var psi = new ProcessStartInfo();
psi.Environment.Clear();
psi.Environment.Add("DOTNET_SYSTEM_GLOBALIZATION_INVARIANT", "true");
RemoteExecutor.Invoke(action, new RemoteInvokeOptions { StartInfo = psi, TimeOut = 10 * 60 * 1000 }).Dispose();
}

(and likely elsewhere) makes impossible to tunnel DOTNET options through the test layers to enable various JIT diagnostics and debugging techniques.

cc @MihaZupan @dotnet/jit-contrib

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Apr 2, 2024
@AndyAyersMS
Copy link
Member Author

Also blocks various testing modes: gcstress, jitstress, SPMI collection, etc.

@MihaZupan
Copy link
Member

For the SearchValues tests I picked this up from CultureInfo tests. The clearing likely isn't needed here

@AndyAyersMS
Copy link
Member Author

@BruceForstall pointed me at this, which seems like a good pattern to emulate:

private static void CopyEssentialTestEnvironment(IDictionary<string, string> environment)
{
string[] essentialVariables = { "HOME", "LD_LIBRARY_PATH", "ICU_DATA" };
string[] prefixedVariables = { "DOTNET_", "COMPlus_", "SuperPMIShim" };
foreach (DictionaryEntry de in Environment.GetEnvironmentVariables())
{
if (Array.FindIndex(essentialVariables, x => x.Equals(de.Key)) >= 0 ||
Array.FindIndex(prefixedVariables, x => ((string)de.Key).StartsWith(x, StringComparison.OrdinalIgnoreCase)) >= 0)
{
environment[(string)de.Key] = (string)de.Value;
}
}
}

There are only 6 hits for Environment.Clear(); so perhaps I can go audit them all and change them over to this pattern.

@jkotas
Copy link
Member

jkotas commented Apr 2, 2024

I think both these patterns are questionable.

In both of these cases, the code wants to clear or preserve environment variables that affect process globalization settings. The code should clear the specific environment variables - it is a small finite number - and leave everything else intact.

It would be nice to factor this out into a helpers under src\libraries\Common so that we have just one place to update if we discover new environment variables that affect globalization.

@JulieLeeMSFT JulieLeeMSFT added this to the 9.0.0 milestone Apr 9, 2024
@AndyAyersMS
Copy link
Member Author

The code should clear the specific environment variables - it is a small finite number - and leave everything else intact.

Can somebody help me figure out what needs to be cleared?

@jkotas
Copy link
Member

jkotas commented Apr 16, 2024

I think clearing LANG and all env variables that start with DOTNET_SYSTEM_GLOBALIZATION prefix should do it.

@AndyAyersMS AndyAyersMS added the Priority:2 Work that is important, but not critical for the release label May 8, 2024
@AndyAyersMS AndyAyersMS removed the Priority:2 Work that is important, but not critical for the release label Jul 25, 2024
@AndyAyersMS AndyAyersMS modified the milestones: 9.0.0, 10.0.0 Aug 1, 2024
@AndyAyersMS
Copy link
Member Author

Still want to do this, but not a must-have for 9.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

No branches or pull requests

4 participants