questionable file name assumptions baked into SDK

[RobertHenry6bev]

Describe the bug

There are assumptions baked into Get:DotnetExeDirectory, especially for linux arm64 hosts.

SDK function GetDotnetExeDirectory
contains the line:
dotnetRootPath = Path.GetFileName(dotnetRootPath).Contains("dotnet") || Path.GetFileName(dotnetRootPath).Contains("x64") || Path.GetFileName(dotnetRootPath).Equals("d") ? dotnetRootPath : Path.Combine(dotnetRootPath, "dotnet");

There are lots of things wrong here:

• The line is too long (too many characters; source code style)
• There's an assumption that the directory holding the executable (we're given the name of the executable) contains "dotnet". For the latest SDK+runtime, directory names seem to be of the form "net7.0-linux" or "net7.0-Linux-Release-arm64". So why are you looking for "dotnet" rather than just "net"?
• There's an assumption that x64 host is handled differently than arm64. Why aren't these handled identically, at least for the *nix hosts? I'm told this is a windows architecture issue)
• You're fishing for substrings in the directory holding the executable. Perhaps fish for all substrings that are plausibly part of a RID.
• Why the special case assumption for "d" ? If this is a test harness issue, fix it over there, not in this general code.
• You aren't testing to ensure that the directory name you return is indeed a directory
• You aren't testing to ensure that every intermediate directory on the proposed path is a directory. So you'll return a path like "..../dotnet/sdk" where "..../dotnet" exists and is an executable file, but is not a directory like you assume in what is returned.
• Catch the thrown error and make an error message that is a little more appropriate, and perhaps rethrow the error.
• You implement a naming protocol. Is this protocol documented in "official" docs on the SDK? If so, please add to the comment a citation of the official filenaming docs for the SDK.

To Reproduce

Exceptions (if any)

Further technical details

• I build dotnet runtime in a variety of ways using a variety of gcc versions and a variety of gcc flags. I compare results on x64 host vs arm64 hosts.

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[elinor-fung]

CommonOptions.GetDotnetExeDirectory is the function in question (not EnvironmentProvider.GetDotnetExeDirectory):

sdk/src/Cli/dotnet/CommonOptions.cs

Lines 234 to 245 in aef5b12

	public static string GetDotnetExeDirectory()
	{
	// Alternatively we could use Microsoft.DotNet.NativeWrapper.EnvironmentProvider.GetDotnetExeDirectory here
	// (while injecting env resolver so that DOTNET_MSBUILD_SDK_RESOLVER_CLI_DIR is being returned as null)
	// However - it first looks on PATH - which can be problematic in environment (e.g. dev) where we have installed and xcopy dotnet versions
	var dotnetRootPath = Path.GetDirectoryName(Environment.ProcessPath);
	// When running under test the path does not always contain "dotnet".
	// The sdk folder is /d/ when run on helix because of space issues
	dotnetRootPath = Path.GetFileName(dotnetRootPath).Contains("dotnet") || Path.GetFileName(dotnetRootPath).Contains("x64") || Path.GetFileName(dotnetRootPath).Equals("d") ? dotnetRootPath : Path.Combine(dotnetRootPath, "dotnet");
	return dotnetRootPath;
	}

I'll be honest - I have no idea why this function is doing the checks for directory name substrings.

@JanKrivanek @marcpopMSFT who last updated this - can you explain the logic in this function? If this is for the dotnet SDK commands, shouldn't Environment.ProcessPath always be the dotnet executable and the dotnet root always be the containing directory? What is the scenario where the root is actually <directoryOfProcessPath>\dotnet?

[marcpopMSFT]

That's a question for @JanKrivanek as prior to #25505, this code was only used in GetCurrentRuntimeId and now it's used more broadly in dotnet new calls. It was originally added as part of our code to get the RID when specifying --os and --arch: #18889.

My change was just to add the /d/ check since that's used in in helix paths and there was not a better way to solve this within the tests easily.

[JanKrivanek]

I came accross this recently, and moved our functionality to use Microsoft.DotNet.NativeWrapper.EnvironmentProvider.GetDotnetExeDirectory
As the previous approach is not reliable enough.
This is not yet merged - it's part of our templating merge PR: #26521, after we merge the PR - I'll make the logic in CommonOptions private to prevent it being pulled elsewhere (as we are not using it any more).

I'd suggest the same utility to be used in CommonOptions as well - I created a draft PR for that: #26851
It just can get tricky in few cases:

• custom msbuild resolver defined (via DOTNET_MSBUILD_SDK_RESOLVER_CLI_DIR env var) - do we want to get RID of the msbuild override or ignore it? I choosed the latter in the proposed draft PR - @marcpopMSFT could you suggest if that's desirable?
• sdk test environment scenarios - some logic is being executed from test host and there is mismatched version of dotnet registered on path (the one used for building the code - in <root>\.dotnet). This I believe can be addressed by making test properly aware of their env (and register artifacts dotnet in env if needed. I'm e.g. doing this in our templating merge PR)
• xcopy deploy without registering in path - I believe the Microsoft.DotNet.NativeWrapper.EnvironmentProvider.GetDotnetExeDirectory should first look on current executable and then on path as a fallback (not vice versa which happens now). @sfoslund (author of the code: 2d5a8e6#diff-8575e38e0b5dc36efede66ab45a93a6b6995b10cea19168a6f83b2af0048ec43) might have better perspective about this