-
Notifications
You must be signed in to change notification settings - Fork 446
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
Finalize ARM64 support (Installers, publishing-rid, revert winforms support) #8470
Finalize ARM64 support (Installers, publishing-rid, revert winforms support) #8470
Conversation
Also ignore the changes in Versions.props. These are to test while ASP flows an official version of the ARM64 assets (dotnet/aspnetcore#25579) cc: @tommcdon |
3fdac90
to
7f67b54
Compare
Related to #7985 |
2f34f1a
to
f1e463b
Compare
Official test build with versions.props changed backed out to test shipping bits: https://dev.azure.com/dnceng/internal/_build/results?buildId=811915&view=results |
f1e463b
to
75de7b2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but I don't have a deep understanding in this area. @joeloff do you mind reviewing too?
@sfoslund I'm looking at it |
@richlander @wli3 @sfoslund Are these OK? Both from a product perspective as well as a technical perspective? I am still not familiar with when things get downloaded on the fly or not. |
I don't see a problem from the technical perspective. |
Good ❤️ |
@marcpopMSFT given this is for RC2, who'd be the right M2 to ask for this to be considered by tactics? |
@hoyosjs The sdk is on the VS schedule for approvals and so doesn't need M2 signoff yet. This is a significant enough change though that it's worth mentioning to tactics as an FYI. |
@hoyosjs @joeloff I gave tactics a heads up on this change. Is there anything left we need to cover before we can merge this and get testing on the official signed installer? CC @jeffhandley who expressed interest in testing and @tommcdon who's been engaged with the arm64 work on the runtime side. |
@hoyosjs If you can provide a link to the signed installer, I'll relay it to my team and some folks will try it out. Several people on the Libraries team have ARM64 devices and have been dogfooding. |
Further testing done:
Bugs I noticed:
People who might care about this: @safern, I know you have the Surface X. @jeffhandley https://dev.azure.com/dnceng/internal/_build/results?buildId=811915&view=results has builds that can be tested in the artifacts. @richlander are these experiences, plus the ones describing what's available in-box acceptable for now? |
@hoyosjs The UI strings are static. And they're referenced from the theme.xml that defines the screens. You can condition the string by adding both strings to the UI, but then set the visibility property on a known property that would only be available if your arm64/x64. You would effectively duplicate this line
But give it different names: SuccessInstallHeader and SuccessInstallHeaderArm64 and define two strings
And in the bundle, define the variables with the same name as the UI controls.
This ensures that only one of the two controls is active in the UI. |
@joeloff the problem with that approach is we end up with the success message on uninstall overlayed with the uninstall header. (The success page always has |
Ok |
@hoyosjs ideally you want to condition the variable on something like WixBundleAction, but bundles don't support conditionals, except if you set the variable, then override it through a registry search, which would complicate things significantly A simpler approach is to create an ARM64 version of the theme file. It can be an exact copy and share the .wxl file, except we swap out the one string. Once WPF/WinForms are supported, we flip back to a shared theme file. |
…dowsDesktop support
@joeloff that worked nicely enough. Thanks! |
@hoyosjs glad to hear the theme change worked. I think if this build is green we should merge |
Yeah, that's about as much manual testing as I can think of to catch potential issues here. |
Merge pull request #8470 from hoyosjs/juhoyosa/arm64-bundled-installer Finalize ARM64 support (Installers, publishing-rid, revert winforms support)
…x merge conflicts. **Summary of the change** Enable an arm64 windows installer. Cherry pick the below commit and fix merge conflicts. Update the installer UI to be consistent with 3.1 installer UI. **Testing** Signed off by Tom and Richa targeting arm64 installer as well as baseline arm64 scenarios. Merge pull request #8470 from hoyosjs/juhoyosa/arm64-bundled-installer Finalize ARM64 support (Installers, publishing-rid, revert winforms support) ![arm64 installer.png](https://dev.azure.com/dnceng/7ea9116e-9fac-403d-b258-b31fcf1bb293/_apis/git/repositories/c20f712b-f093-40de-9013-d6b084c1ff30/pullRequests/13891/attachments/arm64%20installer.png)
@wli3 @marcpopMSFT @sfoslund this is the PR I folded the other one into. It's still in draft as I'm testing but I wanted some feedback.
I've excluded the following assets from the ARM64 installer. Are these ok?