Build ARM64 in main windows build job #38997

HaoK · 2021-12-13T18:00:40Z

No description provided.

HaoK · 2021-12-13T21:06:56Z

@dotnet/aspnet-build surprisingly this got as far as running tests, looks like the test x64 job is trying to run the arm64 tests as well, do you guys have any suggestions on what the best way for me to skip those tests in our windows x64 job?

eng/Build.props

HaoK · 2021-12-14T00:14:41Z

/azp run

azure-pipelines · 2021-12-14T00:15:07Z

Azure Pipelines successfully started running 3 pipeline(s).

HaoK · 2021-12-16T03:18:49Z

Well this is green and building now, so at least that's good, @dougbu so this is basically a continuation of #30680 which you had a fair bit of back and forth with Sourabh, before I start bringing some more of those changes forward (specifically the installer stuff), did the issues around a separate windows native build step get resolved, i.e.

Another possibility is to build the Windows native assets once in a separate job that all other Windows jobs depend on. That would take some care to upload all the relevant files to an AzDO artifact, download them, and place the files where managed projects and the installer builds expect them. Might get messy but would lengthen the build only slightly (for the job startup delay plus the download / file placement work).

Do we still need to do any major job surgery?

dougbu · 2021-12-16T04:07:41Z

Do we still need to do any major job surgery?

I am not sure. Depends on how the bits are laid out when publishing the relevant tests. See our Teams conversation about possibilities and the need to see how the existing IIS tests work on Windows x64.

Undo

eng/build.ps1

eng/Build.props

eng/build.ps1

eng/scripts/vs.16.buildtools.json

eng/targets/Cpp.Common.props

This reverts commit 93154f1.

This reverts commit 40cc695.

This reverts commit 193dd37.

dougbu · 2022-01-04T00:20:46Z

@dougbu how much will your crossgen2 change mitigate that?

I don't know yet because I haven't experimented enough. Suspect it will slightly lengthen a single "All SharedFx" job, eliminating the new Build ARM64 job in the middle of our longest job in official builds. Is the new job responsible for a significant portion of the "10-15 minutes" you mentioned❔

wtgodbe · 2022-01-06T23:10:23Z

src/Installers/Windows/AspNetCoreModule-Setup/IIS-Setup/include.wxi

@@ -23,6 +23,12 @@
    <?define ProgramFilesFolder = ProgramFilesFolder ?>
    <?define SystemFolder = SystemFolder ?>
    <?define PlatformValue = Intel ?>
+  <?elseif $(var.Platform) ~= "arm64" ?>


Why ~= here as opposed to = above?

I think the difference is only case insensitive, I can switch it to equals to be consistent if you prefer, we seem to be inconsistent with ARM64/arm64 more than the other platforms though

wtgodbe · 2022-01-06T23:11:41Z

src/Servers/IIS/AspNetCoreModuleV2/AspNetCore/AspNetCore.vcxproj

@@ -43,26 +51,13 @@
  <PropertyGroup Label="Configuration">
    <VCToolsVersion />
  </PropertyGroup>
-  <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Debug|Win32'" Label="Configuration">
+  <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Debug|Win32' OR '$(Configuration)|$(Platform)'=='Debug|x64' OR '$(Configuration)|$(Platform)'=='Debug|ARM64' OR '$(Configuration)|$(Platform)'=='Debug|Any CPU'" Label="Configuration">


What platforms are you rejecting here (i.e. could you just check Configuration = Debug?)

Do we have a regular ARM (not 64) bit platform that we build still? I believe that's the only one that's still split out from the main windows job on CI too

Directory.Build.props

adityamandaleeka · 2022-01-07T19:45:33Z

src/Servers/IIS/AspNetCoreModuleV2/IISLib/IISLib.vcxproj

-    </Link>
-  </ItemDefinitionGroup>
-  <ItemDefinitionGroup Condition="'$(Configuration)|$(Platform)'=='Release|x64' OR '$(Configuration)|$(Platform)'=='Release|Any CPU'">
+  <ItemDefinitionGroup Condition="'$(Configuration)|$(Platform)'=='Release|Win32' OR '$(Configuration)|$(Platform)'=='Release|x64' OR '$(Configuration)|$(Platform)'=='Release|ARM64' OR '$(Configuration)|$(Platform)'=='Release|Any CPU'">


Thanks for removing the duplication here. We should probably do that in more of these proj files at some point (not required in this PR :))

src/Servers/IIS/AspNetCoreModuleV2/CommonLibTests/CommonLibTests.vcxproj

adityamandaleeka · 2022-01-07T20:00:41Z

src/Servers/IIS/AspNetCoreModuleV2/CommonLibTests/CommonLibTests.vcxproj

+    <ClCompile>
+      <PrecompiledHeader>NotUsing</PrecompiledHeader>
+      <Optimization>Disabled</Optimization>
+      <PreprocessorDefinitions>X64;_DEBUG;_CONSOLE;%(PreprocessorDefinitions)</PreprocessorDefinitions>


Can you please confirm whether X64 is the right define to use for ARM64?

Will do, I'm guessing this should be arm64, but will address in a follow up PR

adityamandaleeka · 2022-01-07T20:05:25Z

src/Installers/Windows/AspNetCoreModule-Setup/ANCMV2/AncmV2.wixproj

@@ -45,6 +45,7 @@
    <ProjectReference Include="..\CustomAction\aspnetcoreCA.vcxproj">
      <Name>aspnetcoreCA</Name>
      <SetPlatform Condition="'$(Platform)' == 'x86'">Platform=Win32</SetPlatform>
+      <SetPlatform Condition="'$(Platform)' == 'arm64'">Platform=ARM64</SetPlatform>


Curious, why isn't this needed for x64?

Just guessing that x64 is the default perhaps, will leave this unresolved to follow up on this later

.azure/pipelines/ci.yml

adityamandaleeka · 2022-01-07T20:15:39Z

Left some comments, but this looks good otherwise as a first cut at getting ARM64 enabled in our builds. Thanks @HaoK!

dougbu reviewed Dec 13, 2021

View reviewed changes

eng/Build.props Show resolved Hide resolved

adityamandaleeka and others added 12 commits December 15, 2021 14:20

Enable arm64 build of ANCM native binaries.

389c6df

Add arm64 to tests and assets

383a918

Fixup

c44327a

Split arm64 into its own group

61ef6e8

Update assets.props

29eaedb

Update assets.props

1867969

Update Build.props

5badb65

Update Build.props

a0ba135

Try skipping common lib tests on arm64

ab5db1f

Fix platform

72cd529

Fix sln

1671c0f

Fix sln

6236597

HaoK force-pushed the haok/arm branch from e2b3c02 to 798db6d Compare December 15, 2021 22:23

Skip arm64 tests on non arm64

a962540

Undo

HaoK force-pushed the haok/arm branch from 798db6d to a962540 Compare December 16, 2021 17:45

HaoK added 2 commits December 16, 2021 10:19

Add vs components for arm64

40cc695

msbuild tweaks

96819ba

HaoK commented Dec 16, 2021

View reviewed changes

eng/build.ps1 Outdated Show resolved Hide resolved

Installer arm64 stuff

93154f1

dougbu reviewed Dec 16, 2021

View reviewed changes

eng/Build.props Show resolved Hide resolved

eng/Build.props Show resolved Hide resolved

eng/build.ps1 Outdated Show resolved Hide resolved

eng/scripts/vs.16.buildtools.json Outdated Show resolved Hide resolved

eng/targets/Cpp.Common.props Show resolved Hide resolved

HaoK added 4 commits December 16, 2021 13:41

Revert "Installer arm64 stuff"

193dd37

This reverts commit 93154f1.

Revert "Add vs components for arm64"

ff1cae8

This reverts commit 40cc695.

Revert uneeded stuff for this iteration

c410192

Revert "Revert "Installer arm64 stuff""

2c6946e

This reverts commit 193dd37.

HaoK requested a review from adityamandaleeka January 4, 2022 20:03

HaoK changed the title ~~[WIP] ANCM: Build ARM64 in main windows build job~~ Build ARM64 in main windows build job Jan 6, 2022

HaoK marked this pull request as ready for review January 6, 2022 22:22

HaoK requested review from BrennanConroy, halter73, Pilchie, Tratcher and a team as code owners January 6, 2022 22:22

Tratcher removed their request for review January 6, 2022 22:26

wtgodbe reviewed Jan 6, 2022

View reviewed changes

adityamandaleeka reviewed Jan 7, 2022

View reviewed changes

Directory.Build.props Outdated Show resolved Hide resolved

adityamandaleeka reviewed Jan 7, 2022

View reviewed changes

src/Servers/IIS/AspNetCoreModuleV2/CommonLibTests/CommonLibTests.vcxproj Outdated Show resolved Hide resolved

HaoK added 2 commits January 7, 2022 11:47

Update Directory.Build.props

661dad3

Update CommonLibTests.vcxproj

feabeb3

adityamandaleeka reviewed Jan 7, 2022

View reviewed changes

.azure/pipelines/ci.yml Outdated Show resolved Hide resolved

adityamandaleeka approved these changes Jan 7, 2022

View reviewed changes

Fix displayname

f588bb6

HaoK enabled auto-merge (squash) January 7, 2022 23:05

HaoK merged commit c8c1700 into main Jan 8, 2022

HaoK deleted the haok/arm branch January 8, 2022 00:15

ghost added this to the 7.0-preview1 milestone Jan 8, 2022

BrennanConroy mentioned this pull request Jan 10, 2022

Fix internal builds #39411

Merged

amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Build ARM64 in main windows build job #38997

Build ARM64 in main windows build job #38997

HaoK commented Dec 13, 2021

HaoK commented Dec 13, 2021

HaoK commented Dec 14, 2021

azure-pipelines bot commented Dec 14, 2021

HaoK commented Dec 16, 2021

dougbu commented Dec 16, 2021

dougbu commented Jan 4, 2022

wtgodbe Jan 6, 2022

HaoK Jan 6, 2022

wtgodbe Jan 6, 2022

HaoK Jan 6, 2022

adityamandaleeka Jan 7, 2022

adityamandaleeka Jan 7, 2022

HaoK Jan 7, 2022

adityamandaleeka Jan 7, 2022

HaoK Jan 7, 2022

adityamandaleeka commented Jan 7, 2022

Build ARM64 in main windows build job #38997

Build ARM64 in main windows build job #38997

Conversation

HaoK commented Dec 13, 2021

HaoK commented Dec 13, 2021

HaoK commented Dec 14, 2021

azure-pipelines bot commented Dec 14, 2021

HaoK commented Dec 16, 2021

dougbu commented Dec 16, 2021

dougbu commented Jan 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adityamandaleeka commented Jan 7, 2022