Use dotnet from current process if possible #203

Forgind · 2023-03-16T18:58:45Z

If we are running in a dotnet executable, we should use that instead of trying to find it on the PATH.

ladipro

This change may break someone. If MSBuildLocator is running under a private runtime, it would now be enumerating only private SDKs in the corresponding directory. Is this really what we want?

ladipro · 2023-03-20T10:06:09Z

src/MSBuildLocator/DotNetSdkLocationHelper.cs

-            // https://github.com/dotnet/designs/blob/main/accepted/2021/install-location-per-architecture.md
-            // This can be done using the nethost library. We didn't do this previously, so I did not implement this extension.
-            foreach (string dir in Environment.GetEnvironmentVariable("PATH").Split(Path.PathSeparator))
+            if (Process.GetCurrentProcess().ProcessName.Equals(exeName))


On Windows the ProcessName property does not include the trailing .exe so I'm afraid this won't work.

Forgind · 2023-03-21T23:00:07Z

This change may break someone. If MSBuildLocator is running under a private runtime, it would now be enumerating only private SDKs in the corresponding directory. Is this really what we want?

What do you mean by a private runtime? I'd assume the only relevant runtime here is the .NET runtime. Since this is intended to be process-specific, I was thinking it would only affect things hosted in the dotnet process, which would mean first-party only. I think it's reasonable in that case to prioritize the dotnet they are actively using. @rainersigwald, do you have an opinion?

ladipro · 2023-03-22T07:11:05Z

.NET runtime is xcopy-able and may exist in many copies on the machine, including copies used only by a specific app. Consider the case where I use MSBuildLocator in an app which runs on its copy of the runtime, like for example on what's under .dotnet in our dotnet repos. It's not clear if MSBuildLocator is expected to locate SDK's in this private location (there may be none if it's just runtime without SDK) or in a more conventional location inferred from PATH, DOTNET_ROOT etc.

What is the motivation for this change?

Forgind · 2023-03-22T21:01:28Z

.NET runtime is xcopy-able and may exist in many copies on the machine, including copies used only by a specific app. Consider the case where I use MSBuildLocator in an app which runs on its copy of the runtime, like for example on what's under .dotnet in our dotnet repos. It's not clear if MSBuildLocator is expected to locate SDK's in this private location (there may be none if it's just runtime without SDK) or in a more conventional location inferred from PATH, DOTNET_ROOT etc.

What is the motivation for this change?

I guess I was mostly thinking of/motivated by the opposite case, where you unzip an SDK to some random spot on disk without adding it to PATH, then build. Ideally, we'd be able to detect that SDK and use it if there are no other SDKs available and offer it if possible.

I changed this to search both where it had been searching and next to the process it's running in if it's running in dotnet. It should only throw if it fails to find dotnet in either location. Does that sound like a more appropriate way to find both safely?

ladipro · 2023-03-23T12:56:19Z

src/MSBuildLocator/DotNetSdkLocationHelper.cs

+
+            if (!returnedSDKs)
+            {
+                throw new InvalidOperationException("Failed to find an appropriate version of .NET Core MSBuild. Call to hostfxr_resolve_sdk2 failed. There may be more details in stderr.");


nit: It doesn't look like getting here necessarily implies that hostfxr_resolve_sdk2 failed. We may have simply found no SDKs.

ladipro · 2023-03-23T13:00:48Z

I guess I was mostly thinking of/motivated by the opposite case, where you unzip an SDK to some random spot on disk without adding it to PATH, then build. Ideally, we'd be able to detect that SDK and use it if there are no other SDKs available and offer it if possible.

Makes perfect sense!

I changed this to search both where it had been searching and next to the process it's running in if it's running in dotnet. It should only throw if it fails to find dotnet in either location. Does that sound like a more appropriate way to find both safely?

I think so. It's hard for me to guess what all the use cases are. As long as the caller can then choose the SDK to use, I'd say returning both locations is a good approach. It just shouldn't return duplicates, which I believe would happen with the code as it is written now if we're running on a regularly installed runtime.

Forgind · 2023-03-23T14:26:21Z

It just shouldn't return duplicates, which I believe would happen with the code as it is written now if we're running on a regularly installed runtime.

It does, and I've been wrestling with this a bit. I'm not a huge fan of on-the-fly deduplication code in general, first because it sometimes erases subtle ordering decisions (like that the first SDK is potentially the one from the global.json, whereas the rest are "normal") and second because it tends to be less efficient. The first version of this only did the second check (current process) if the first version found no SDKs, but I decided that wasn't necessarily desirable. I'll see if I can implement some deduplication logic today.

rainersigwald · 2023-03-31T22:08:37Z

src/MSBuildLocator/DotNetSdkLocationHelper.cs

@@ -112,34 +115,110 @@ private static IEnumerable<string> GetDotNetBasePaths(string workingDirectory)
                dotnetPath = Path.GetDirectoryName(isWindows ? dotnetPath : realpath(dotnetPath) ?? dotnetPath);
            }

+            string pathOfPinnedSdk = string.Empty;
+
+            foreach (string path in GetDotNetBasePaths(workingDirectory, dotnetPath))


What's your thinking on preferring the PATH search to the current executable? I could see that going either way.

I agree it could go either way...in this case, since I'm adding them all to a HashSet rather than a List (for deduplication), I'm not actually preferring one above the other. I'm just going by version number and mixing them up. (I also see an argument for keeping them separate...but I'm not sure how valuable that is, and I honestly don't think the "from current process" code path will be important all that often.)

rainersigwald · 2023-03-31T22:15:51Z

src/MSBuildLocator/DotNetSdkLocationHelper.cs

            {
+                pinnedSdk = true;


What is a "pinned" sdk?

That would be like if you have a global.json. Is there a better name that would've made that clear?

YuliiaKovalova · 2023-08-21T15:41:35Z

@baronfel please take a look

YuliiaKovalova · 2023-08-23T13:40:14Z

@Forgind ,could you tell me if I understand it right, that my #230 covers the implemented functionality?

YuliiaKovalova · 2023-08-24T10:07:07Z

Implemented in scope of #230

Use dotnet from current process if possible

22dcca9

ladipro reviewed Mar 20, 2023

View reviewed changes

ProcessName is always dotnet

2d9c6ac

Forgind added 2 commits March 22, 2023 13:51

Combine results instead of if/else

26deca0

Only fail when SDK isn't found

1ff4d0c

ladipro reviewed Mar 23, 2023

View reviewed changes

Add deduplication logic

0bef5be

rainersigwald reviewed Mar 31, 2023

View reviewed changes

YuliiaKovalova requested review from rainersigwald and ladipro August 4, 2023 12:03

YuliiaKovalova closed this Aug 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use dotnet from current process if possible #203

Use dotnet from current process if possible #203

Forgind commented Mar 16, 2023

ladipro left a comment

ladipro Mar 20, 2023

Forgind commented Mar 21, 2023

ladipro commented Mar 22, 2023

Forgind commented Mar 22, 2023

ladipro Mar 23, 2023

ladipro commented Mar 23, 2023

Forgind commented Mar 23, 2023

rainersigwald Mar 31, 2023

Forgind Mar 31, 2023

rainersigwald Mar 31, 2023

Forgind Mar 31, 2023

YuliiaKovalova commented Aug 21, 2023

YuliiaKovalova commented Aug 23, 2023

YuliiaKovalova commented Aug 24, 2023

Use dotnet from current process if possible #203

Use dotnet from current process if possible #203

Conversation

Forgind commented Mar 16, 2023

ladipro left a comment

Choose a reason for hiding this comment

ladipro Mar 20, 2023

Choose a reason for hiding this comment

Forgind commented Mar 21, 2023

ladipro commented Mar 22, 2023

Forgind commented Mar 22, 2023

ladipro Mar 23, 2023

Choose a reason for hiding this comment

ladipro commented Mar 23, 2023

Forgind commented Mar 23, 2023

rainersigwald Mar 31, 2023

Choose a reason for hiding this comment

Forgind Mar 31, 2023

Choose a reason for hiding this comment

rainersigwald Mar 31, 2023

Choose a reason for hiding this comment

Forgind Mar 31, 2023

Choose a reason for hiding this comment

YuliiaKovalova commented Aug 21, 2023

YuliiaKovalova commented Aug 23, 2023

YuliiaKovalova commented Aug 24, 2023