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

Enable build of ARM64 installers in ASP.NET Core #25579

Merged
merged 1 commit into from
Sep 10, 2020

Conversation

hoyosjs
Copy link
Member

@hoyosjs hoyosjs commented Sep 3, 2020

Enable ARM64 installers build.

  • Changes WiX toolset used to 3.14 to support ARM64
  • Generates targeting pack from the x86/x64 leg, as it gets produced using a zip that gets generated there.
  • The ARM leg now produces all the necessary msi's, exe, and wixlib needed for the installer to generate a bundle.

NuGet.config Outdated
@@ -11,6 +11,7 @@
<add key="roslyn-tools" value="https://dotnet.myget.org/F/roslyn-tools/api/v3/index.json" />
<!-- Used for the SiteExtension 3.1 bits that are included in the 5.0 build -->
<add key="dotnet31-transport" value="https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet3.1-transport/nuget/v3/index.json" />
<add key="general-testing" value="https://pkgs.dev.azure.com/dnceng/public/_packaging/general-testing/nuget/v3/index.json" />
Copy link
Member Author

Choose a reason for hiding this comment

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

I added this feed for testing. The main issue was that WiX doesn't have a package for the toolset that supports ARM64. Other repositories don't use wixproj, so we didn't need it before this. I don't know what feed this should go to, or if this is even a good idea.

Copy link
Member

@dougbu dougbu Sep 4, 2020

Choose a reason for hiding this comment

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

Using general-testing is not a good idea because pretty much anything could be pushed there. Suggest working w/ core-eng to get the new WiX into dotnet-eng

Copy link
Member Author

@hoyosjs hoyosjs Sep 8, 2020

Choose a reason for hiding this comment

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

Yeah, this was to verify this won't break in CI. I'm asking for some guidance from the engineering team here.

Copy link
Member Author

Choose a reason for hiding this comment

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

@dougbu I got the green light to move this as is to dotnet-eng. I've reverted all changes to NuGet.config

eng/Build.props Show resolved Hide resolved
@hoyosjs hoyosjs marked this pull request as ready for review September 3, 2020 22:27
@hoyosjs
Copy link
Member Author

hoyosjs commented Sep 3, 2020

Official test build: https://dev.azure.com/dnceng/internal/_build/results?buildId=800872&view=results

Further validation:

  • Confirmed files are R2R, signed files.
  • Now checking registry keys and signatures of the bundles.

cc: @joeloff, @tommcdon, @dougbu

@Pilchie Pilchie added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Sep 3, 2020
@Pilchie
Copy link
Member

Pilchie commented Sep 3, 2020

Do we want to target this to release/5.0-rc2?

@hoyosjs
Copy link
Member Author

hoyosjs commented Sep 3, 2020

@Pilchie, yes. I will port once this gets reviewed and goes in.

@joeloff
Copy link
Member

joeloff commented Sep 3, 2020

@Pilchie if we did, we might be able to unblock the SDK and get that part completed for RC2 as well, but it would be tight. I'll have a chat with other SDK folks. @marcpopMSFT as an FYI

@tommcdon
Copy link
Member

tommcdon commented Sep 3, 2020

FYI @richlander

NuGet.config Outdated
@@ -11,6 +11,7 @@
<add key="roslyn-tools" value="https://dotnet.myget.org/F/roslyn-tools/api/v3/index.json" />
<!-- Used for the SiteExtension 3.1 bits that are included in the 5.0 build -->
<add key="dotnet31-transport" value="https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet3.1-transport/nuget/v3/index.json" />
<add key="general-testing" value="https://pkgs.dev.azure.com/dnceng/public/_packaging/general-testing/nuget/v3/index.json" />
Copy link
Member

@dougbu dougbu Sep 4, 2020

Choose a reason for hiding this comment

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

Using general-testing is not a good idea because pretty much anything could be pushed there. Suggest working w/ core-eng to get the new WiX into dotnet-eng

build.ps1 Outdated Show resolved Hide resolved
eng/Build.props Show resolved Hide resolved
@dougbu
Copy link
Member

dougbu commented Sep 8, 2020

I can't approve this until the general-testing feed is removed. @hoyosjs you've addressed my other comments well. (nit: "only" isn't spelled "inly" even in comments.)

Also suggest @joeloff or someone else w/ more Wix knowledge have a look at these changes. They follow the existing patterns but I may be missing something.

@dougbu dougbu requested a review from joeloff September 8, 2020 16:04
Copy link
Member

@dougbu dougbu left a comment

Choose a reason for hiding this comment

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

I'm fine with this but would really appreciate additional eyes on it @joeloff @dotnet/aspnet-build or anyone w/ more WiX experience than me, please chime in.

@joeloff
Copy link
Member

joeloff commented Sep 9, 2020

Looks good. I'm guessing that the hosting bundle will remain x86/x64 only for the time being. Unlike the other installers, the hosting bundle prefers to install all architectures by default and I think there will be a clash since for x64, there aren't new properties defined yet (at least none that I can see in Windows).

My big concern is the wixlib because we rely on that to share the package chaining across the shared framework, hosting and SDK installer bundles.

@hoyosjs
Copy link
Member Author

hoyosjs commented Sep 9, 2020

@joeloff any verification I can do to verify? I have official runs so I have signed installers and can test anything that's needed. However, this is becoming time sensitive.

@joeloff
Copy link
Member

joeloff commented Sep 9, 2020

@hoyosjs if the have the signed MSI, you can try them out on a VM. You can also double check the Product/Upgrade code for the x64/arm64 MSIs just to be sure they are different (you can use ORCA for that). The build seeds the GUIDs using the Platform property, so they should be different.

@hoyosjs
Copy link
Member Author

hoyosjs commented Sep 9, 2020

@joeloff I've tested this:

image

My big concern is the wixlib because we rely on that to share the package chaining across the shared framework, hosting and SDK installer bundles.

Do you have a particular concern I can check against?

@hoyosjs
Copy link
Member Author

hoyosjs commented Sep 9, 2020

Issues I keep hitting are #25623 and a gradle issue solved by #25707. Closing and reopening to create a rebase.

@hoyosjs hoyosjs closed this Sep 9, 2020
@hoyosjs hoyosjs reopened this Sep 9, 2020
@hoyosjs
Copy link
Member Author

hoyosjs commented Sep 10, 2020

@dougbu I've tried rebasing but I keep getting red. Looking around I see you had a comment describing all the issues I've seen. However they seem to have gotten fixes upstream, but they keep failing. Any advice?

#25293 (comment)

@dougbu
Copy link
Member

dougbu commented Sep 10, 2020

@hoyosjs the problem here is you're targeting the master branch and all the fixes mentioned have been checked into release/5.0-rc2. Need to either wait for #25650 to merge or target release/5.0-rc2 instead. Recommend doing the second because time for RC2 is getting short.

@hoyosjs hoyosjs changed the base branch from master to release/5.0-rc2 September 10, 2020 06:20
Changes WiX toolset used to 3.14 to support ARM64
Generates targeting pack from the x86/x64 leg, as it gets produced using a zip that gets generated there.
The ARM64 leg now produces all the necessary msi's, exe, and wixlib needed for the installer to generate a bundle.
@hoyosjs
Copy link
Member Author

hoyosjs commented Sep 10, 2020

@dougbu I didn't know there was an automated FI from RC2 to master. I've retargeted this to RC2.

@hoyosjs
Copy link
Member Author

hoyosjs commented Sep 10, 2020

Feel free to merge once it's green

@Pilchie can this please be considered for RC2 for the ARM64 installer?

@ghost
Copy link

ghost commented Sep 10, 2020

Hello human! Please make sure you've included the Shiproom Template in a comment or (preferably) the PR description. Also, make sure this PR is not marked as a draft and is ready-to-merge.

@dougbu
Copy link
Member

dougbu commented Sep 10, 2020

All checks 🟢 Waiting on your word @Pilchie

@Pilchie
Copy link
Member

Pilchie commented Sep 10, 2020

Just wanted to double check with Tactics - we're good to merge this now.

@Pilchie Pilchie added Servicing-approved Shiproom has approved the issue and removed Servicing-consider Shiproom approval is required for the issue labels Sep 10, 2020
@Pilchie
Copy link
Member

Pilchie commented Sep 10, 2020

Approved for .NET 5 RC2.

@dougbu dougbu merged commit dda1d33 into dotnet:release/5.0-rc2 Sep 10, 2020
@dougbu
Copy link
Member

dougbu commented Sep 10, 2020

Thanks very much @hoyosjs

@tommcdon tommcdon mentioned this pull request Sep 10, 2020
10 tasks
JunTaoLuo pushed a commit that referenced this pull request Feb 25, 2021
Changes WiX toolset used to 3.14 to support ARM64
Generates targeting pack from the x86/x64 leg, as it gets produced using a zip that gets generated there.
The ARM64 leg now produces all the necessary msi's, exe, and wixlib needed for the installer to generate a bundle.
JunTaoLuo pushed a commit that referenced this pull request Mar 2, 2021
Changes WiX toolset used to 3.14 to support ARM64
Generates targeting pack from the x86/x64 leg, as it gets produced using a zip that gets generated there.
The ARM64 leg now produces all the necessary msi's, exe, and wixlib needed for the installer to generate a bundle.
JunTaoLuo pushed a commit that referenced this pull request Mar 3, 2021
* Enable ARM64 installers build. (#25579)

Changes WiX toolset used to 3.14 to support ARM64
Generates targeting pack from the x86/x64 leg, as it gets produced using a zip that gets generated there.
The ARM64 leg now produces all the necessary msi's, exe, and wixlib needed for the installer to generate a bundle.

* Build tageting pack installers

* Set TP version to 3.1.10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework Servicing-approved Shiproom has approved the issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants