search

microsoft / MSBuildLocator

An API to locate MSBuild assemblies from an installed Visual Studio location. Use this to ensure that calling the MSBuild API will use the same toolset that a build from Visual Studio or msbuild.exe would.
Other
212 stars 83 forks source link

Use dotnet from current process if possible #203

Closed Forgind closed 10 months ago

Forgind commented 1 year ago

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

Forgind commented 1 year ago

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 commented 1 year ago

.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 commented 1 year ago

.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 commented 1 year ago

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 commented 1 year ago

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.

YuliiaKovalova commented 10 months ago

@baronfel please take a look

YuliiaKovalova commented 10 months ago

@Forgind ,could you tell me if I understand it right, that my https://github.com/microsoft/MSBuildLocator/pull/230 covers the implemented functionality?

YuliiaKovalova commented 10 months ago

Implemented in scope of https://github.com/microsoft/MSBuildLocator/pull/230