Skip to content

Fix forwarding DOTNET_ROOT #49634

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 3 additions & 5 deletions src/Cli/dotnet/Commands/Test/TestCommand.cs
Original file line number Diff line number Diff line change
Expand Up @@ -241,11 +241,9 @@ private static TestCommand FromParseResult(ParseResult result, string[] settings
}
}

// Set DOTNET_PATH if it isn't already set in the environment as it is required
// by the testhost which uses the apphost feature (Windows only).
(bool hasRootVariable, string rootVariableName, string rootValue) = VSTestForwardingApp.GetRootVariable();
if (!hasRootVariable)
{

Dictionary<string, string> variables = VSTestForwardingApp.GetVSTestRootVariables();
foreach (var (rootVariableName, rootValue) in variables) {
testCommand.EnvironmentVariable(rootVariableName, rootValue);
VSTestTrace.SafeWriteTrace(() => $"Root variable set {rootVariableName}:{rootValue}");
}
Expand Down
25 changes: 14 additions & 11 deletions src/Cli/dotnet/Commands/Test/VSTestForwardingApp.cs
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,13 @@ public class VSTestForwardingApp : ForwardingApp
public VSTestForwardingApp(IEnumerable<string> argsToForward)
: base(GetVSTestExePath(), argsToForward)
{
(bool hasRootVariable, string rootVariableName, string rootValue) = GetRootVariable();
if (!hasRootVariable)
Dictionary<string, string> variables = GetVSTestRootVariables();
foreach (var (rootVariableName, rootValue) in variables)
{
WithEnvironmentVariable(rootVariableName, rootValue);
VSTestTrace.SafeWriteTrace(() => $"Root variable set {rootVariableName}:{rootValue}");
}

VSTestTrace.SafeWriteTrace(() => $"Forwarding to '{GetVSTestExePath()}' with args \"{argsToForward?.Aggregate((a, b) => $"{a} | {b}")}\"");
}

Expand All @@ -38,14 +38,17 @@ private static string GetVSTestExePath()
return Path.Combine(AppContext.BaseDirectory, VstestAppName);
}

internal static (bool hasRootVariable, string rootVariableName, string rootValue) GetRootVariable()
internal static Dictionary<string, string> GetVSTestRootVariables()
{
string rootVariableName = Environment.Is64BitProcess ? "DOTNET_ROOT" : "DOTNET_ROOT(x86)";
bool hasRootVariable = Environment.GetEnvironmentVariable(rootVariableName) != null;
string rootValue = hasRootVariable ? null : Path.GetDirectoryName(new Muxer().MuxerPath);

// We rename env variable to support --arch switch that relies on DOTNET_ROOT/DOTNET_ROOT(x86)
// We provide VSTEST_WINAPPHOST_ only in case of testhost*.exe removing VSTEST_WINAPPHOST_ prefix and passing as env vars.
return (hasRootVariable, $"VSTEST_WINAPPHOST_{rootVariableName}", rootValue);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm double checking, this is fine because this logic isn't going to be used with old vstest.console.exe because it's guaranteed that we use vstest.console.exe that's part of SDK, and we will only merge this PR after inserting the fix on vstest side. Is that correct?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the old way of doing this can be removed. It also does not seem that people use that env variable on its own.

// Gather the current .NET SDK dotnet.exe location and forward it to vstest.console.dll so it can use it
// to setup DOTNET_ROOT for testhost.exe, to find the same installation of NET SDK that is running `dotnet test`.
// This way if we have private installation of .NET SDK, the testhost.exe will be able to use the same private installation.
// The way to set the environment is complicated and depends on the version of testhost, so we leave that implementation to vstest console,
// we just tell it where the current .net SDK is located, and what is the architecture of it. We don't have more information than that here.
return new()
{
["VSTEST_DOTNET_ROOT_PATH"] = Path.GetDirectoryName(new Muxer().MuxerPath),
["VSTEST_DOTNET_ROOT_ARCHITECTURE"] = RuntimeInformation.ProcessArchitecture.ToString()
};
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ public void TestForwardDotnetRootEnvironmentVariables()
// This project is compiled, and executed by the tests in "test/dotnet-test.Tests/GivenDotnetTestForwardDotnetRootEnvironmentVariables.cs"
foreach (DictionaryEntry env in Environment.GetEnvironmentVariables())
{
if (env.Key.ToString().Contains("VSTEST_WINAPPHOST_"))
if (env.Key.ToString().Contains("VSTEST_"))
{
Console.WriteLine($"{env.Key.ToString()}={env.Value.ToString()}");
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,8 @@ public void ShouldForwardDotnetRootEnvironmentVariablesIfNotProvided()
.Should().Contain("Total tests: 1")
.And.Contain("Passed: 1")
.And.Contain("Passed TestForwardDotnetRootEnvironmentVariables")
.And.Contain("VSTEST_WINAPPHOST_");
.And.Contain("VSTEST_DOTNET_ROOT_PATH")
.And.Contain("VSTEST_DOTNET_ROOT_ARCHITECTURE");
}

result.ExitCode.Should().Be(0);
Expand Down
Loading