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

Add option to install dotnet at .dotnet/$rid #14913

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

am11
Copy link
Member

@am11 am11 commented Jul 2, 2024

It is enabled by setting DOTNET_USE_ARCH_IN_INSTALL_PATH=1.

Fix #14751

cc @akoeplinger

@am11 am11 force-pushed the feature/rid-aware-dotnet-install branch 3 times, most recently from 613df43 to de12fab Compare July 2, 2024 10:02
@ViktorHofer
Copy link
Member

Honestly I'm not in favor of this change. While the current layout might not be correct in all cases, it existed for years and works. I fear that many repositories (150+ repos depend on Arcade) expect the dotnet installation to exist in the current location.

Another example is

<DotNetRoot Condition="'$(DotNetRoot)' == ''">$([MSBuild]::NormalizeDirectory('$(RepoRoot)', '.dotnet'))</DotNetRoot>
which would now be wrong.

@am11
Copy link
Member Author

am11 commented Jul 2, 2024

This is enabled with repo sets DOTNET_USE_ARCH_IN_INSTALL_PATH, as discussed in the issue.

We also have DOTNET_INSTALL_DIR support, so it's not the only different path in these scripts.

@ViktorHofer
Copy link
Member

Right, but I question the benefit of adding yet another path. I'm not sold that this is needed. Which repositories would set this?

@am11
Copy link
Member Author

am11 commented Jul 2, 2024

This enabler is mainly for runtime where we do a lot of platform-specific builds, I'm testing it dotnet/runtime#104295 to see how CI reacts.

BTW, we don't have to set it repo-wide either. Just having this facility enables the consumer (either the repo itself, or someone building a repo) to use the derived RID in installation path that works with dotnet.sh/cmd.

@akoeplinger
Copy link
Member

akoeplinger commented Jul 3, 2024

After thinking through this some more I'm also not sure anymore this is the best approach since it's only really useful if we enable it by default and we probably don't have the resources to fix all the cases where we assume the current path.

I had another simpler idea though: we should be able to detect whether we have the right dotnet by running e.g. .dotnet/dotnet --version in InitializeDotNetCli and if it doesn't work we could delete and redownload it. That'd leave the path unchanged. @ViktorHofer what do you think?

@premun
Copy link
Member

premun commented Jul 18, 2024

Re-downloading the right RID can be nice - when jumping locally between Windows and WSL, you need to keep nuking the .dotnet dir (dotnet--version does not even run though).

Alternatively, why not just adding an optional argument with the destination dir and override it in runtime's scenarios?

@jkotas
Copy link
Member

jkotas commented Jul 24, 2024

when jumping locally between Windows and WSL, you need to keep nuking

You can work around this by installing the right .NET SDK in a global location on both systems. These scripts are able to auto-detect that you have the right version available and skip downloading the repo-local copy.

@am11
Copy link
Member Author

am11 commented Jul 24, 2024

Another example is

<DotNetRoot Condition="'$(DotNetRoot)' == ''">$([MSBuild]::NormalizeDirectory('$(RepoRoot)', '.dotnet'))</DotNetRoot>

It will not fail with the current state of this PR because of line 22. DOTNET_INSTALL_DIR is set by the script and we don't hit this path when tools.ps1/sh are involved. If we have another example where DOTNET_INSTALL_DIR is not honored, it is likely a bug which that needs fixing. I covered the ones in runtime dotnet/runtime#104295.

IMO we should take this change and fix like those few places taking wrong assumptions and there aren't that many. I think this change is even less riskier than any other change we make in arcade which can break downstream, in that we have it behind an enviornment variable, which we can remove if we want after updating all consumer repos to use the new scheme.

@am11
Copy link
Member Author

am11 commented Nov 14, 2024

@ViktorHofer, any further thoughts on this? Yesterday, I was building dotnet/winforms on win-arm64 by invoking build.cmd on powershell arm64. Before the package restore begins, it installed:

  • win-arm64 dotnet version from globa.json at .dotnet
  • latest daily version of win-x64 dotnet at .dotnet
  • latest daily version of win-86 dotnet at .dotnet/x86 <- this is similar to what the PR is achieving (except it'd be /win-86 instead of /x86 and would apply for all three archs not just x86..)

as I was fixing the compilation issue with latest roslyn, all three versions were the same (global.json had latest version). Consequently, .dotnet\host\fxr\10.0.0-alpha.1.24560.1\hostfxr.dll was x86_64 because it installs second and overwrites arm64 one, which failed to load in the arm64 process. Workaround was to manually unzip arm64 package one more time before re-invoking build.cmd.

Separate directories is not new in dotnet install. There are multiple scenarios discussed where it'd help (docker, wsl, winform). If we are considering it we should take this change at the beginning of 10.0 cycle and update the downstream repos gradually with trivial changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change the default dotnet installation location from .dotnet/ to .dotnet/$hostRid/
5 participants