Skip to content

Enable ARM64 installers build. (#25579) #30463

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 10 commits into from
Mar 3, 2021

Conversation

JunTaoLuo
Copy link
Contributor

@JunTaoLuo JunTaoLuo commented Feb 25, 2021

Backporting dda1d33 to release/3.1

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.

This change is required in addition to dotnet/core-setup#9124 to add support for win-arm64 for the SDK.

@JunTaoLuo JunTaoLuo requested review from NikolaMilosavljevic and a team February 25, 2021 18:46
@ghost ghost added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Feb 25, 2021
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.

Looks legit. Does this require an internal verification build❔

@JunTaoLuo
Copy link
Contributor Author

I've been running internal builds to verify: latest one is https://dnceng.visualstudio.com/internal/_build/results?buildId=1012303&view=results but I suspect I'll make more changes before I confirm it's verified.

@JunTaoLuo JunTaoLuo marked this pull request as ready for review February 26, 2021 18:15
@JunTaoLuo JunTaoLuo added the ask-mode This issue / PR is a patch candidate which we will bar-check internally before patching it. label Feb 26, 2021
@JunTaoLuo
Copy link
Contributor Author

JunTaoLuo commented Mar 2, 2021

@dotnet/aspnet-build @NikolaMilosavljevic please take a look at the last two commits. I got the targeting pack building for win-arm64 locally and I think these are all the changes we need to make. I'll do some final version verifications once the internal build is complete: https://dnceng.visualstudio.com/internal/_build/results?buildId=1018153&view=results.

@@ -84,7 +84,8 @@
<RuntimeInstallerBaseName>aspnetcore-runtime</RuntimeInstallerBaseName>
<TargetingPackInstallerBaseName>aspnetcore-targeting-pack</TargetingPackInstallerBaseName>

<!-- Produce targeting pack installers/packages once per major.minor except in extraordinary cases i.e. 3.1.10. -->
<!-- Produce targeting pack installers/packages once per major.minor except in extraordinary cases i.e. 3.1.13 win-arm64. -->
<IsTargetingPackBuilding Condition=" '$(AspNetCorePatchVersion)' == '13' AND '$(TargetArchitecture)' == 'arm64' AND $([MSBuild]::IsOSPlatform('Windows')) ">true</IsTargetingPackBuilding>
Copy link
Member

Choose a reason for hiding this comment

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

Are we sure about 13 being the version of the special build?

Copy link
Member

Choose a reason for hiding this comment

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

Should be 14

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad, I need to rebase on your branding update.

@@ -33,7 +33,7 @@
<!-- TargetingPackVersionPrefix is used by projects, like .deb and .rpm, which use slightly different version formats. -->
<TargetingPackVersionPrefix>$(VersionPrefix)</TargetingPackVersionPrefix>
<!-- Targeting packs do not produce patch versions in servicing builds. No API changes are allowed in patches. -->
<TargetingPackVersionPrefix Condition="'$(IsTargetingPackBuilding)' != 'true'">$(AspNetCoreMajorVersion).$(AspNetCoreMinorVersion).10</TargetingPackVersionPrefix>
<TargetingPackVersionPrefix Condition="'$(IsTargetingPackBuilding)' != 'true'">$(AspNetCoreMajorVersion).$(AspNetCoreMinorVersion).13</TargetingPackVersionPrefix>
Copy link
Member

Choose a reason for hiding this comment

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

Would arm64 TP package have ..0 version?

Copy link
Member

@wtgodbe wtgodbe Mar 2, 2021

Choose a reason for hiding this comment

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

We want to build just the win-arm64 .msi, with a 3.1.0 version - we don't want to build the .nupkg

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@NikolaMilosavljevic Historically, the version of the installer and targeting pack follows whichever servicing patch built it. This is why in the official 3.1.406 SDK, you'll see that the targeting pack is installed under packs\Microsoft.AspNetCore.App.Ref\3.1.10.

@wtgodbe I tried but couldn't easily split the logic controlling building the targeting pack nupkg and the installers. They are all based on IsTargetingPackBuilding. In fact, the installer build extracts the zip of the targeting pack that was built when IsPackable is true in the Microsoft.AspNetCore..App.Ref project, which means it also builds the nupkg. I don't want to add additional logic to handle this scenario and I am under the impression that the SDK can choose to ingest just the msi installers and ignore the nupkg. Is that possible @NikolaMilosavljevic?

Copy link
Member

Choose a reason for hiding this comment

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

We could choose not to ingest the new .nupkg in dotnet/installer, but that's manual and risks us messing it up - we'd also have to manually remove it from the drop so it doesn't get published to nuget.org. Maybe the best thing would be to just not publish it - you could add a post-build target that deletes the .nupkg from ArtifactsShippingPackagesDir before the manifest gets generated, for example.

Copy link
Member

Choose a reason for hiding this comment

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

Well actually wait, we do want installer to ingest the .msi, which it knows how to do based on the version of the .nupkg's (Microsoft.AspNetCore.App.Ref and Microsoft.AspNetCore.App.Ref.Internal). I think we just want to not publish the .nupkg to nuget.org, meaning it needs to have the same version as the last targeting pack, since installer will still try to restore the .nupkg in future builds (so it should keep getting the 3.1.10 one from nuget.org). Will try to define a concrete plan in email

hoyosjs and others added 6 commits March 1, 2021 21:42
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
Copy link
Contributor Author

JunTaoLuo commented Mar 2, 2021

Looking at the artifacts of the internal build, here are the new artifacts in this build compared to the existing build:

aspnetcore-runtime-3.1.14-servicing.21151.14-win-arm64.msi
aspnetcore-runtime-3.1.14-servicing.21151.14-win-arm64.msi.sha512
aspnetcore-runtime-internal-3.1.14-win-arm64.wixlib
aspnetcore-runtime-internal-3.1.14-win-arm64.wixlib.sha512
aspnetcore-targeting-pack-3.1.14.tar.gz
aspnetcore-targeting-pack-3.1.14.tar.gz.sha512
aspnetcore-targeting-pack-3.1.14.zip
aspnetcore-targeting-pack-3.1.14.zip.sha512
aspnetcore-targeting-pack-3.1.14-servicing.21151.14-win-arm64.msi
aspnetcore-targeting-pack-3.1.14-servicing.21151.14-win-arm64.msi.sha512
Microsoft.AspNetCore.App.Ref.3.1.14.symbols.nupkg
VS.Redist.Common.AspNetCore.SharedFramework.arm64.3.1.3.1.14-servicing.21151.14.symbols.nupkg
VS.Redist.Common.AspNetCore.TargetingPack.arm64.3.1.3.1.14-servicing.21151.14.symbols.nupkg
Microsoft.AspNetCore.App.Ref.3.1.14.nupkg

That seems to contain everything we want but note that we do want to ignore the targeting pack zips/tar.gzs

@JunTaoLuo
Copy link
Contributor Author

Comparing the contents of the targeting packs and I only see differences in file versions (ignore 3.1.13 versions since I used the artifacts before version fixup for comparison) as expected:
image
image

@JunTaoLuo JunTaoLuo force-pushed the johluo/win-arm64-installers branch from df99369 to 6355ec6 Compare March 2, 2021 18:40
Copy link
Member

@wtgodbe wtgodbe left a comment

Choose a reason for hiding this comment

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

Why isn't Microsoft.Aspnetcore.App.Ref.Internal produced? dotnet/installer needs it to find the blob location of the installers

@JunTaoLuo
Copy link
Contributor Author

Why isn't Microsoft.Aspnetcore.App.Ref.Internal produced? dotnet/installer needs it to find the blob location of the installers

Good question let me check.

@JunTaoLuo
Copy link
Contributor Author

@wtgodbe the Ref.Internal package is now built.

@dougbu
Copy link
Member

dougbu commented Mar 3, 2021

I mentioned two viable options in our email thread:

  1. Produce 3.1.10 targeting packs again but only release the new Windows ARM64 installer.
  2. Produce an release a full set of 3.1.14 targeting packs.

Am I correct you're going w/ option (1)❔

@JunTaoLuo
Copy link
Contributor Author

Correct, there were multiple email threads and the latest decision so far is to go with Option 1.

@JunTaoLuo
Copy link
Contributor Author

JunTaoLuo commented Mar 3, 2021

Marking this as servicing approved since it's the core-setup change dotnet/core-setup#9124 was approved and this change is part of the overall effort to backport support for win-arm64.

@JunTaoLuo JunTaoLuo added Servicing-approved Shiproom has approved the issue Servicing-consider Shiproom approval is required for the issue and removed ask-mode This issue / PR is a patch candidate which we will bar-check internally before patching it. labels Mar 3, 2021
@ghost
Copy link

ghost commented Mar 3, 2021

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.

@JunTaoLuo JunTaoLuo added tell-mode Indicates a PR which is being merged during tell-mode and removed Servicing-approved Shiproom has approved the issue Servicing-consider Shiproom approval is required for the issue labels Mar 3, 2021
@JunTaoLuo JunTaoLuo merged commit 69108ce into release/3.1 Mar 3, 2021
@JunTaoLuo JunTaoLuo deleted the johluo/win-arm64-installers branch March 3, 2021 18:06
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 tell-mode Indicates a PR which is being merged during tell-mode
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants