Skip to content

Retarget DOTNETHOME when installing x64 on ARM64 #11843

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

Merged
merged 7 commits into from
Sep 17, 2021

Conversation

ericstj
Copy link
Member

@ericstj ericstj commented Sep 2, 2021

Redirects installer MSIs to x64 subfolder.

DOTNETHOME may not always be set, be sure to install the tag to a
predictable location.
@ericstj
Copy link
Member Author

ericstj commented Sep 14, 2021

I may need to address the InstallLocation keys when installing to default location, even though the SDK previously wrote them regardless of the product actually being installed it seems like they might be necessary, at the very least just to circumvent later bad defaults that assumed ProgramFiles\dotnet.

I looked at usages of the key.

https://github.com/dotnet/deployment-tools/blob/3c246e582164af60a222930a2073ba2e1fa6b807/src/clickonce/launcher/HostFinder.cs#L23 mentions but doesn't use this location, though it does have a bug

https://github.com/dotnet/aspnetcore/blob/52eff90fbcfca39b7eb58baad597df6a99a542b0/src/Servers/IIS/AspNetCoreModuleV2/CommonLib/HostFxrResolver.cpp#L269-L298 looks like it would be broken, though it does seem to be somewhat unaware of ARM64 entirely. Filed a bug for this.

Found some other interesting 3rd party hits:
https://github.com/hkrn/nanoem/blob/88925dfa0fb4cb9357ef7dc0a3ae98a34ca15675/emapp/plugins/pep_coreclr/pep_coreclr.cc#L385
https://github.com/qmgindi/Au/blob/fb3c79f21328c9cf5a6c7739b435b2bd9983ab80/Au.AppHost/AppHost.cpp#L231
https://github.com/pebakery/pebakery/blob/93fc046d8d56d05ec137caa785a520bb663e4781/Launcher/NetDetector.cpp#L215 mentions it but doesn't use it since it's not written by the runtime.

https://github.com/dnSpy/dnSpy/blob/2b6dcfaf602fb8ca6462b8b6237fdfc0c74ad994/Extensions/dnSpy.Debugger/dnSpy.Debugger.DotNet.CorDebug/Utilities/DotNetHelpers.cs#L79-L87 this actually uses the key to "customize" our ARP entry and add their own icon 😆
https://github.com/Tyrrrz/DotnetRuntimeBootstrapper/blob/3f07f7935ad727cd8ee15908b494461a39001b0d/DotnetRuntimeBootstrapper.Executable/Env/Dotnet.cs#L22-L43

We changed the bundle to not pass in default values for DOTNETHOME
because it cannot calculate them in all cases: there is no way to
conditionally set a path which does not exist.  As a result, the MSI
needs to calculate them so that it can write the appropriate defaults.
@ericstj
Copy link
Member Author

ericstj commented Sep 15, 2021

Ok I've added searches to the CLISDK MSI to set the DOTNET_* values even when the bundle doesn't set them. This should be good to go.

@ericstj ericstj marked this pull request as ready for review September 15, 2021 21:21

<!-- Permit same path on non-ARM64 machines since past SDKs always wrote this value -->
<bal:Condition Message="The installation path for ARM64 SDK installations: &quot;[DOTNETHOME_ARM64]&quot; cannot be the same as for x64 SDK installations: &quot;[DOTNETHOME_X64]&quot;">
WixBundleInstalled OR (NOT DOTNETHOME_ARM64 ~= DOTNETHOME_X64) OR (NOT NativeProcessorArchitecture=&quot;ARM64&quot;) OR DOTNETHOMESIMILARITYCHECKOVERRIDE
Copy link
Member Author

Choose a reason for hiding this comment

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

Note that I special cased this condition because all the existing SDKs already write keys for x64 and ARM64 on Windows. The value for ARM64 on an x64 machine is innocuous, since it exists and won't cause any bad behavior, unless windows finds a way to let ARM64 code run on x64 😆

There is a more annoying pathological bug as a result of this, but I'd like to peel that off. See #11999

@ericstj ericstj requested a review from joeloff September 17, 2021 14:35
@ericstj
Copy link
Member Author

ericstj commented Sep 17, 2021

Pinging again. Folks I need feedback on this so that we can backport it to 6.0.

</Property>

<!-- Set the DOTNETHOME property if not passed in and we found a path -->
<SetProperty Id="DOTNETHOME_X86" Value="[DOTNETHOME_X86_SEARCH]" Before="CostFinalize">
Copy link
Member

Choose a reason for hiding this comment

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

I think I got this. The only time we'd be in trouble is a clean install, where the bundle fails to pass DOTNETHOME_xxx and the search won't find anything because we've never been installed and that would technically point to an authoring error. I need to check how the SDK is authored into VS because there VS takes the place of the bundle to pass in DOTNET_HOME to the MSI

Copy link
Member Author

Choose a reason for hiding this comment

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

We shouldn't have to pass in defaults. A clean install won't detect it as it runs searches up front in the bundle. We can run similar searches here so that we detect it after the host was installed. The bundle's searches miss this because they run before the host is installed.

In general we have to be very careful when using public properties to control installed content: think through all scenarios of install/repair/patch(if supported)/uninstall/upgrade to make sure property values are correct. Never count on the MSI being "called correctly" to get that stuff right.

I need to check how the SDK is authored into VS because there VS takes the place of the bundle to pass in DOTNET_HOME to the MSI

We should remove those. The meaning of DOTNETHOME public properties is "final install location of dotnet root directory". This assumption is made in SDK bundle authoring and one that is a public contract with users that we cannot break. We're changing the default for x64. If someone is specifying that (including VS bundle) by duplicating the defaults it needs to change to reflect the new default.

@ericstj ericstj merged commit df4dff1 into dotnet:main Sep 17, 2021
@ericstj
Copy link
Member Author

ericstj commented Sep 17, 2021

/backport to release/6.0.1xx-rc2

ericstj added a commit to ericstj/installer that referenced this pull request Sep 18, 2021
Retarget DOTNETHOME when installing x64 on ARM64
@ericstj
Copy link
Member Author

ericstj commented Sep 22, 2021

@joeloff @marcpopMSFT -- anything that needs to happen to get this change into the 6.0 branch too? I see it seems to be missing there

@joeloff
Copy link
Member

joeloff commented Sep 22, 2021

If it's in RC2 then that will get flowed during a mopup merge back into 6.0.1xx

mmitche added a commit to mmitche/installer that referenced this pull request Sep 23, 2021
…ation

After dotnet#11843, post build signing started failing, failing to find one of the input wixobj files. The wixobj was not included in the wixpack. This is because the installer repo uses custom scripts to generate MSIs, rather than the arcade tasks (which would handle adding this transparently), so the input files need to be added explicitly.
mmitche added a commit that referenced this pull request Sep 23, 2021
…ation (#12100)

After #11843, post build signing started failing, failing to find one of the input wixobj files. The wixobj was not included in the wixpack. This is because the installer repo uses custom scripts to generate MSIs, rather than the arcade tasks (which would handle adding this transparently), so the input files need to be added explicitly.
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.

2 participants