Skip to content

Conversation

@ericstj
Copy link
Member

@ericstj ericstj commented Sep 23, 2021

Issue dotnet/installer#11999

The host will be responsible for writing these from this point forward, and only when it is installed.

Installer side of changes: dotnet/installer#12106

We intend to backport this to 6.0, and follow up with further changes in 7.0 to clean this up more.

The keys are absent when only the runtime is installed. We have scenarios where users will only be able to install the x64 runtime and we need it write these keys, otherwise the host won't be able to find the redirected x64 runtime on ARM64.

Copy link
Member

@vitek-karas vitek-karas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but I'm no expert in wix, so please get another review :-)

@ericstj
Copy link
Member Author

ericstj commented Sep 24, 2021

@joeloff let me know that VS actually installs the entire x86 product except the host on x64 machines. This was surprising to me.

That means the x86 InstallLocation would not be written for a clean install of VS on an x64 machine. We may already be in that state regardless of this PR, since we've suggested that VS stop passing in defaults (since it can't calculate those on ARM64) and the detection I added to the SDK uses dotnet.exe as the indicator.

Do folks here think we should change VS not to do that, and install the x86 host so that it matches the behavior of the runtime bundle?
Should we add/move this key to a different MSI so that it is still written in that scenario?
Should we leave this key absent in that scenario?

@joeloff
Copy link
Member

joeloff commented Sep 24, 2021

I think it's okay to leave absent. The path issue is one problem I think we had in the past where a user installs VS, then drops to the CLI and ends up using x86 SDK instead of x64.

@phenning Does WTE still search through the registry to figure out the .NET installations and decide which templates to offer?

@vitek-karas
Copy link
Member

It would be really nice to have consistent behavior regardless of how the product is installed. We already have a discrepancy between installers and dotnet-install, but that is at least somewhat intentional.

@phenning
Copy link
Member

@joeloff We try to look up the arch specific registry key under SOFTWARE\dotnet\setup\installedVersions to find the install location.

If that is not there, we fallback to trying to use the appropriate Programs File path.

We use this to find the dotnet\templates folder and other paths, such as the shared runtime.

@hoyosjs
Copy link
Member

hoyosjs commented Sep 24, 2021

Do folks here think we should change VS not to do that, and install the x86 host so that it matches the behavior of the runtime bundle?
Should we add/move this key to a different MSI so that it is still written in that scenario?
Should we leave this key absent in that scenario?

Hm. I am not sure which to do then. The only case where I can see a component that's required for anything to be "global state" is the shared framework - but then the only way you reach if is using the global install using a host + hostfxr or such. Or if you deploy an app that is not self contained without having the SFX, and then it just needs to find MNA in the shared component. So, the lowest block is the framework, but it feels weird to add this to the shared framework MSI.

@joeloff
Copy link
Member

joeloff commented Sep 24, 2021

Ok, I think we're fine then with the changes and not adding both hosts to VS. I just wanted to be sure nothing was looking for that location. Adding the x86 host will likely cause more issues for VS as we've seen those in the past when multiple SDKs for both x86 and x64 are installed.

@ericstj
Copy link
Member Author

ericstj commented Sep 24, 2021

Let's try this as-is for 6.0 and see. The workaround if something breaks is pretty simple (add the value).

Adding the x86 host will likely cause more issues for VS as we've seen those in the past when multiple SDKs for both x86 and x64 are installed.

Are those all around the PATH being set? We should start off 7.0 with applying the condition to PATH setting on x86 to only do it on x86 machine.

@richlander
Copy link
Member

This change goes over my head (or it's too late on Friday night). If everyone else likes it, it's good with me.

@joeloff
Copy link
Member

joeloff commented Sep 27, 2021

Let's try this as-is for 6.0 and see. The workaround if something breaks is pretty simple (add the value).

Adding the x86 host will likely cause more issues for VS as we've seen those in the past when multiple SDKs for both x86 and x64 are installed.

Are those all around the PATH being set? We should start off 7.0 with applying the condition to PATH setting on x86 to only do it on x86 machine.

Most of it, yes. MU amplified the issue a little bit because if we detect a 32-bit component that ships with the 32-bit SDK (even thought that MSI was installed by VS), we will offer the 32-bit SDK as there's nothing else available to patch the MSI and then your PATH ordering can flip. If you're lucky, VS offers the update first, but since MU is non-deterministic, we can't guarantee the order.

@ericstj
Copy link
Member Author

ericstj commented Sep 27, 2021

@richlander we're just changing the MSI that writes the InstallLocation key. Previously it was the SDK now it will be the host. When the SDK was writing it, it was trying to write all InstallLocation-architectures in every SDK architecture. Now the host will only write the architecture it's actually installing. This change will be essential downlevel to ensure we can install runtimes only and have the app-host find the runtime.

@ericstj ericstj merged commit 7344435 into dotnet:main Sep 27, 2021
@ericstj
Copy link
Member Author

ericstj commented Sep 27, 2021

/backport to release/6.0

@github-actions
Copy link
Contributor

Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1279103894

@ghost ghost locked as resolved and limited conversation to collaborators Nov 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants