InstallLocation registry keys are not in sync with runtime installation

[ericstj]

So this is caused by a combination of factors.

1. The SDK bundle always sets DOTNETHOME_ARCH values
   

   installer/src/redist/targets/packaging/windows/clisdk/bundle.wxs

   Lines 124 to 126 in 5c6d74b

   	<Variable Name="DOTNETHOME_X86" Type="string" Value="[ProgramFilesFolder]dotnet" bal:Overridable="yes" />
   	<Variable Name="DOTNETHOME_X64" Type="string" Value="[ProgramFiles64Folder]dotnet" bal:Overridable="yes" />
   	<Variable Name="DOTNETHOME_ARM64" Type="string" Value="[ProgramFiles64Folder]dotnet" bal:Overridable="yes" />

2. The registry keys for all architectures are included in every MSI:

   installer/src/redist/targets/packaging/windows/clisdk/registrykeys.wxs

   Lines 18 to 35 in 5c6d74b

   	<Component Id="DotnetInstallLocation_arm64" Directory="TARGETDIR" Win64="no">
   	<Condition>VersionNT64 AND DOTNETHOME_ARM64</Condition>
   	<RegistryKey Root="HKLM" Key="SOFTWARE\dotnet\Setup\InstalledVersions\arm64">
   	<RegistryValue Action="write" Name="InstallLocation" Type="string" Value="[DOTNETHOME_ARM64]" KeyPath="yes"/>
   	</RegistryKey>
   	</Component>
   	<Component Id="DotnetInstallLocation_x64" Directory="TARGETDIR" Win64="no">
   	<Condition>VersionNT64 AND DOTNETHOME_X64</Condition>
   	<RegistryKey Root="HKLM" Key="SOFTWARE\dotnet\Setup\InstalledVersions\x64">
   	<RegistryValue Action="write" Name="InstallLocation" Type="string" Value="[DOTNETHOME_X64]" KeyPath="yes"/>
   	</RegistryKey>
   	</Component>
   	<Component Id="DotnetInstallLocation_x86" Directory="TARGETDIR" Win64="no">
   	<Condition>DOTNETHOME_X86</Condition>
   	<RegistryKey Root="HKLM" Key="SOFTWARE\dotnet\Setup\InstalledVersions\x86">
   	<RegistryValue Action="write" Name="InstallLocation" Type="string" Value="[DOTNETHOME_X86]" KeyPath="yes"/>
   	</RegistryKey>
   	</Component>

This is partly fixed by my PR to remove the defaults from the bundle. This also added some logic in the MSI when values aren't present to only set them if dotnet.exe actually exists, however another characteristic is that the bundle and MSI will preserve past entries.

We could try to fix this by only preserving past registry entries if they point to actual copies of dotnet.exe. That wouldn't be enough since the x64/ARM64 paths were busted previously.

We could ignore this under the premise of ARM64 doesn't support previous builds of x64 installers (and I added a blocker in the installation that might force someone to uninstall it). The ARM64 key would still be present on x64, but maybe could fix that too by conditioning the key to not install if the native machine was ARM64.

Thoughts?

[vitek-karas]

arm64 key present on x64 is basically a non-issue - at least for the host/runtime, since that only accessed the architecture it's actually running on (and there's no way to run arm64 code on x64).

[ericstj]

cc @joeloff @wli3 @richlander

Just to reiterate the questions:

1. Should we try to clean these up with file checks backing the registry checks in both the bundle and MSI?
2. Should we clean up the ARM64 key from the x64 installer (on non ARM64 machine)?

[joeloff]

@ericstj I think so. Each installer should only create the keys applicable to itself. We can keep everything in the 32-bit hive - I believe that was done so everything was in a single location, but I don't see the point of having arm64 writing a detection key for x86 that has no value.

At least for the installed path, that was only to ensure that users cannot install x86 and x64 on top of each other using a custom location from the commandline, but that should still work if we cleanup/fix the registry

[ericstj]

keep everything in the 32-bit hive

Correct, no changes there.

Each installer should only create the keys applicable to itself

I agree with you on that principle, however we're past that point. For better or worse, the SDK is responsible for writing the detection keys for the runtime. We could change that now if you think it's better. We might be able to get the refcounting right 😆

I don't see the point of having arm64 writing a detection key for x86 that has no value.

The SDK is writing (and necessarily reading to honor retargeting) the detection keys for runtime. So I can imagine cases where the x64 installer wants to write the x86 key, or ARM64 installer writes x64 and x86. If someone were to install those runtimes but only one SDK. Today the SDK shares these components across all architectures. I'm not suggesting a change to that. I assume there was to be a legit reason folks didn't use ifdefs here originally. From the start the x64 installer was writing the both the x64 and x86 InstallLocation.

If I could go back in time, I'd put this whole thing inside the MSIs. Host would register the "InstallLocation" and persist it with registry search on repair/upgrade. All the other MSI's would read it and honor it. They could block if the key wasn't present. Bundle would read it only for the purposes of blocking a move on upgrade, or a architecture clash. The only property the bundle would ever pass down would be "DOTNETHOME" in the case the user's choice made it past those blocks.

It might be possible to do all this retroactively, but I'm not sure doing it in 6.0 RC2 is the best time.

[ericstj]

The host relies on these keys for detection of the runtime. Without them in the runtime bundle scenarios for installing only runtimes on arm64 are broken. They really belong in the host MSIs. We will see if we can make this move with low risk and if so take the changes to runtime/installer for 6.0.

[joeloff]

Yes. Each MSI should write its specific keys and we should pick one MSI per bundle that can write the install location value, so that after the first installed bundle, every subsequent bundle defaults to the existing location

[ericstj]

The host MSI will write them since it's the component that reads them and at the "bottom" of the stack.

[ericstj]

So here's what I'd propose to fix this:
ericstj/runtime@4d97756
ericstj@2b19c6b

I double checked component GUIDs and they remain the same despite this change in authoring.

One oddity of this approach is that we will lose keys if folks don't upgrade the host. Take the following example:

Initial state:
5.0 x86 SDK
5.0 x64 SDK
InstallLocation exists for x86, x64, and arm64 (bogus value)

Upgrade to 6.0 x64 SDK
InstallLocation will only exist for x64, since that's the only host that's upgraded.

Do we care? If we do, I could add more components to the host MSIs and have them do the searches like I had added in installer. I'd prefer to keep this simpler.

Nevermind. I was worried about this thinking that 6.0 SDK would replace the 5.0 SDK, but that's not the case. They are side-by-side. So the 5.0 SDK would keep a ref-count on the components. The 6.0 SDK only replaces other pre-release 6.0 SDKs. I'm OK if we lose the InstallLocation key for a pre-release 6.0 host. Folks can upgrade that to GA to fix it.

I did not add detection to every MSI yet, and instead let the bundle do the detection. We won't be able to delete the bundle detection completely because we're not rebuilding every MSI that the bundle installs. We're also relying on the bundle for doing the directory clashing test, however I could push that check down into the host MSI.

[ericstj]

Here's a mockup of what a larger change would look like to defer to all the MSIs to detect their own location (building upon the previous suggestion)
ericstj@f8a7441
ericstj/aspnetcore@de64bd2
ericstj/arcade@836f0ef

I think this is a good change to take in 7.0 (along with coming up with some way to better share the authoring), but I feel like it's too risky for 6.0. I'd like to get opinions from @joeloff @NikolaMilosavljevic since they are code owners here.

[joeloff]

And @wtgodbe since he's taking over ASP.NET

[joeloff]

So if I'm reading this correctly, whichever MSI installs first writes the location, everyone after that reads + ref counts? This seems like a good change for 7.0

[ericstj]

Everyone reads, there are no shared ref-counts. The only package which installs the registry key is the host package.

The one place where ref-counting does come into play is overlap with previous SDKs. The previous side-by-side SDKs wrote the keys (3.1, 5.0) so they will maintain a refcount on the key and prevent the 6.0 SDK from uninstalling them.

From 6.0 onward, the host MSI is the only thing that writes the keys and only the hosts which are actually installed.

Also to be clear, I'm recommending we do the following for 6.0 (+7.0)
ericstj/runtime@4d97756
ericstj/installer@2b19c6b

And only do the following on 7.0
ericstj/installer@f8a7441
ericstj/aspnetcore@de64bd2
ericstj/arcade@836f0ef