Check for dotnet.dll when enumerating SDK paths

[elinor-fung]

The host treats the existence of a versioned folder under sdk as a valid SDK path. We've seen this cause confusion as users can end up in this state without their knowledge (for example, uninstall leaves an empty folder - which would normally be harmless, except when we try to use it as a valid SDK).

This adds a check for dotnet.dll instead of just the existence of the directory. In the worst case (order of versions we look at happens to be where every version is a better match than the one before), this ends up with extra file existence checks equal to the number of installed SDK versions minus one (since we were always doing one check before).

Fixes dotnet/sdk#22322

[ghost]

Tagging subscribers to this area: @vitek-karas, @agocke, @VSadov
See info in area-owners.md if you want to be subscribed.

Issue Details

---

The host treats the existence of a versioned folder under sdk as a valid SDK path. We've seen this cause confusion as users can end up in this state without their knowledge (for example, uninstall leaves an empty folder - which would normally be harmless, except when we try to use it as a valid SDK).

This adds a check for dotnet.dll instead of just the existence of the directory. In the worst case (order of versions we look at happens to be where every version is a better match than the one before), this ends up with extra file existence checks equal to the number of installed SDK versions minus one (since we were always doing one check before).

Fixes dotnet/sdk#22322

Author:	elinor-fung
Assignees:	-
Labels:	area-Host
Milestone:	-

[vitek-karas]

The code changes look good.

Personally I'm not convinced that the host should do this. In my opinion this is just hiding a bug in the installer which should never leave an empty SDK folder behind.
But if others think that we should add this functionality, then I'm OK with it.

The only worry I have is if there are other pieces of code somewhere else which also treat any version directory as "valid" and those would still accept the old behavior. Especially installers, if they do something like this.

[elinor-fung]

I definitely get the argument that installers should ensure the install is in a reasonable state - and in most cases I would probably agree that other things should not try to compensate.

For the empty folder case though, at least for Windows installers (wix), that is a non-error. They operate on the files in those folders, so uninstall is successful as long as those files are removed - if the folder is in use, that empty folder is left. I think that does make sense from an installer perspective. So for this specific case, I think it is reasonable for the host to ignore empty folders rather than try to make installers never leave an empty folder (likely requiring non-natural things in the authoring).

[vitek-karas]

I don't know installers, so if this is something which is considered "standard" for the installers, then I agree there's value in adding this functionality. In that case I would argue we should consider doing the same for framework lookup - same problem, same solution and we did see some customer issues reported around this.

[elinor-fung]

I do agree we should do the same for framework lookup - as you said, it is the same problem, same solution. I started with sdk since more questions/issues have come my way about that one (also, it was simpler to do).