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

Remove Bundle override of DOTNETHOME #7855

Merged
merged 1 commit into from
Sep 8, 2021

Conversation

ericstj
Copy link
Member

@ericstj ericstj commented Sep 8, 2021

The bundle was passing in a hardcoded DOTNETHOME value unconditionally
to MSIs in the chain. This value doesn't have the redirection we're
adding to the MSI. We can't add that redirection in the bundle, since
Burn doesn't have conditional variables. It can only set variables as a
result of a search to either the value of the search (if successful) or
1/0.

An alternate solution to this would be to just make the MSI treat a value of DOTNETHOME=[ProgramFiles64Folder]dotnet in the same way as an unspecified value of DOTNETHOME. Then we wouldn't have to touch the bundles. The downside of that is that it wouldn't allow someone to specify a value of [ProgramFiles64Folder]dotnet for x64 on an ARM64 machine.

Related https://github.com/dotnet/installer/pull/11843/files#diff-42255eeced0cb7937c40024646092ce85055a361283ed45dec27adbab7996905L124

Here's a sample of the alternate solution we could use in the MSI: https://gist.github.com/ericstj/6edf16be01a4a0fb4eea9ff2afa54f9c/revisions#diff-3acf3c307483c682b16a5942aaca34cf87cc8d81641a5aac5c3e00d7a746f340R66

Thoughts? cc @richlander

The bundle was passing in a hardcoded DOTNETHOME value unconditionally
to MSIs in the chain.  This value doesn't have the redirection we're
adding to the MSI.  We can't add that redirection in the bundle, since
Burn doesn't have conditional variables.  It can only set variables as a
result of a search to either the value of the search (if successful) or
1/0.
@joeloff
Copy link
Member

joeloff commented Sep 8, 2021

Shouldn't the runtime bundle follow the same search logic we have in the SDK for custom installs? It seems having support in some, but not all bundles is a problem because you'd end up with runtimes that won't find SDKs, etc. But then do the runtime MSIs even include the relevant registry keys we use in the SDK search logic?

@ericstj
Copy link
Member Author

ericstj commented Sep 8, 2021

Shouldn't the runtime bundle follow the same search logic we have in the SDK for custom installs

Yeah, I noticed that too. It's a pre-existing bug unfortunately, and I wasn't attempting to fix it here. You can run into this today if you install with runtime bundle after customizing the SDK location. It's probably a lot easier and more consistent way to enforce "sticky" installer retargeting in the MSIs themselves since they can guarantee consistency. Put the registry search in the MSIs so that no matter how they are installed they do the right thing.

But then do the runtime MSIs even include the relevant registry keys we use in the SDK search logic?

They do not, it is implemented in SDK MSI: https://github.com/dotnet/installer/blob/8c476b58df6032c6713c69535d2679632abac9ba/src/redist/targets/packaging/windows/clisdk/registrykeys.wxs#L18-L36

Copy link
Member

@NikolaMilosavljevic NikolaMilosavljevic left a comment

Choose a reason for hiding this comment

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

LGTM. Chained MSIs should own the determination of the default installation path.

@ericstj
Copy link
Member Author

ericstj commented Sep 8, 2021

Opened #7860 to track the observation @joeloff made.

We may still decide to go down the path of the MSI ignoring a default value of DOTNETHOME for x64 on non-native arch, but I think this fix is good regardless since it removes an unnecessary override of the MSI's default behavior.

ericstj added a commit to ericstj/arcade that referenced this pull request Sep 17, 2021
The bundle was passing in a hardcoded DOTNETHOME value unconditionally
to MSIs in the chain.  This value doesn't have the redirection we're
adding to the MSI.  We can't add that redirection in the bundle, since
Burn doesn't have conditional variables.  It can only set variables as a
result of a search to either the value of the search (if successful) or
1/0.
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.

3 participants