Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix userlocal #41735

Open
wants to merge 1 commit into
base: release/8.0.4xx
Choose a base branch
from
Open

Fix userlocal #41735

wants to merge 1 commit into from

Conversation

Forgind
Copy link
Member

@Forgind Forgind commented Jun 21, 2024

Fixes #41420 and #41726

This attempts to find everywhere that we currently assume not-userlocal (where it's supported) and changes it to respect userlocal if specified.

As far as testing, I tried to do dotnet workload update with an SDK without this update, and it broke. I then tried it with these changes, and it succeeded.

I'm not very knowledgeable about what scenarios I should target. I'll keep testing things I can think of, but do either of you have a suggestion on where to focus my testing energy @baronfel @tmds?

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Workloads untriaged Request triage from a team member labels Jun 21, 2024
@baronfel
Copy link
Member

It looks like the two changes are in the main dotnet.dll and the resolver dll, right? So it could be a viable test to

  • get an Ubuntu or Red Hat WSL instance
  • install an 8.x sdk from distro sources
  • drop newly-built patched dlls onto that install
  • test workload install/update?

@baronfel
Copy link
Member

Conceptually, it worries me how many distinct places you had to touch to fix this. It feels like we need some abstraction/mediator that we can ask for what this path should be to ensure that no code has the wrong idea.

@Forgind
Copy link
Member Author

Forgind commented Jun 21, 2024

Conceptually, it worries me how many distinct places you had to touch to fix this. It feels like we need some abstraction/mediator that we can ask for what this path should be to ensure that no code has the wrong idea.

I think this is an excellent point. What makes this hard is that all the load-bearing changes are pretty clearly in one of three buckets: related to the install state, related to workload sets, or related to garbage collection, and for the first two cases, at least, even if something perfect existed, I wouldn't have known to reference it. There are a few rare cases in which we really do want the not-userlocal dotnet path regardless, notably as a fallback path in the resolver, so we can't just fully replace dotnetPath with dotnetOrUserLocalPath...

@kasperk81
Copy link
Contributor

It feels like we need some abstraction/mediator that we can ask for what this path should be to ensure that no code has the wrong idea.

simplification idea: remove dotnetPath argument from WorkloadInfoHelper and directly call CliFolderPathCalculatorCore.GetDotnetHomePath() for !IsUserLocal when initializing WorkloadInfoHelper.DotnetPath. it is still going through a wrapper but less fragmented in comparison.

@marcpopMSFT
Copy link
Member

Hold for the workload set PR but this looks good otherwise

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Workloads untriaged Request triage from a team member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants