dotnet / dotnet/sdk

`dotnetup` system vs admin install detection may incorrectly classify install roots

Open
#55,350 1 comment 0 reactions 1 assignee Claimed by @marcpopMSFT View on GitHub
Area-Install cookie
Dominant language
C#
Stars
3.2k
Forks
1.3k
PR merge metrics
PR metrics pending

Description

As mentioned in https://github.com/dotnet/sdk/pull/54545#discussion_r3599474868:

[DotnetEnvironmentManager.cs](https://github.com/dotnet/sdk/pull/54545/files/a2590adbd581671f164f5958b48bb681a3f0320e#diff-5a33ee7e689222033c298efb31b6dc2778e696c8a9d140b8bbfe09f32ffcf4f2) mixes up two different concepts.

GetPathWinningDotnet VS IsAdminInstallPath is not the same.

The problem that got added, then fixed in https://github.com/dotnet/sdk/pull/54545 was that the shell made dotnetup's dotnet win on the PATH so it would be labeled as System.

But now I think we have the opposite potential problem. If the admin path / reg key value is set to something outside of the expected program files location, then it would be classified as a user install path. I believe this makes dotnetup dotnet use the system PATH and also makes uninstall workflow try to uninstall a system PATH.

https://github.com/dotnet/sdk/pull/55349 Already exists and is being worked on to address this problem.

This was already mentioned in https://github.com/dotnet/sdk/issues/53964 and is now an expanded problem and code patterns have changed.

```
I think there's some conflicting design patterns I've introduced with the walkthrough PR and I'm getting confused in Copilot's terminology.. working this through:

My intention with this function was that ShouldReplaceSystemConfiguration implies system level changes, where the profile change is at a user level basis.

// Save config and apply configuration(s) - NOTE: Terminal Profile not yet implemented. ^ this line was meant to indicate that profile modification would occur in this chunk of code, and should probably be edited as it is incorrect once this PR is merged

With this current change:
ShouldReplaceSystemConfiguration -> causes ApplyEnvironmentModifications to run.

I think the naming conventions have blurred uses cases in the DotnetEnvironmentManager, and we should completely isolate:
A. Environment/PATH Modification
B. Profile File Modiciation

Right now, ApplyEnvironmentModifications calls ConfigureInstallTypeUnix

ConfigureInstallTypeUnix takes in whether we are installing a InstallType of system install or a user install. Now, I might think this means we'd want to:
A. if user install, modify user profiles
B. if system install, modify system path ( no op on unix)

Instead, what the code does today:

If it is a system install, it decides to only set dotnetup on the users profile, maybe because it assumes dotnetup installed using the msi/apt-get system level installer rather than a local install?

Further confusion on Windows for me:
ApplyEnvironmentModifications with this implementation sets the
AdminPathContainsProgramFilesDotnet even if the installType is user. It calls get user root configurations but user root configurations still decide to override the system level configuration by removing the path. I think installType here was meant to designate when/if dotnetup supported running the msi/pkg/apt-get installer but now copilot doesn't understand and there is a bifurcation of the term meaning different things.

Suggestions:
ApplyEnvironmentModifications - should remain methods that only directly modify the system environment
ApplyEnvironmentModifications installType - should clarify installType here means a zip/tar install vs a msi/pkg/apt-get install and not how we want to configure the system
Add: ApplyProfileModifications - used if PathPreference is PathPreference.ShellProfile
ShouldReplaceSystemConfiguration -> returns to the original implementation and is checked separately, so we can both set the system install or not, and modify the profile (or not)
Maybe we need to yoink out the code entirely for using the system installer because we aren't planning to use those right now. this means the install root manager would become simplified and also installType might not even matter as a param to the environment manager.
```

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.