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

Working ANCM arm64 installers #39523

Merged
merged 54 commits into from
Mar 5, 2022
Merged

Working ANCM arm64 installers #39523

merged 54 commits into from
Mar 5, 2022

Conversation

HaoK
Copy link
Member

@HaoK HaoK commented Jan 14, 2022

Part of #39281

First checkpoint: working arm64 installers that currently just put things down exactly where the x64 installer does, just adds arm64 names/component ids and installer logic. I verified that the artifacts do install and drop ANCM on an arm64 machine:

image

Next PR will start making things light up (and work).

@ghost ghost added the area-runtime label Jan 14, 2022
@HaoK HaoK changed the title [WIP] Update tests for ARM64 [WIP] Update installer/tests for ARM64 Feb 22, 2022
@HaoK
Copy link
Member Author

HaoK commented Feb 24, 2022

@aspnet-build am I doing bad things to concurrency by adding more arm64 build steps here for the installers?

First time I've seen this error in a long time, on the ubuntu test job:

.dotnet/sdk/7.0.100-preview.2.22114.1/Microsoft.Common.CurrentVersion.targets(4808,5): error MSB3026: (NETCORE_ENGINEERING_TELEMETRY=Build) Could not copy "/mnt/vss/_work/1/s/artifacts/bin/Microsoft.AspNetCore.Http.Extensions/Release/net7.0/Microsoft.AspNetCore.Http.Extensions.dll" to "/mnt/vss/_work/1/s/artifacts/bin/SystemdTestApp/Release/net7.0/Microsoft.AspNetCore.Http.Extensions.dll". Beginning retry 1 in 1000ms. The process cannot access the file '/mnt/vss/_work/1/s/artifacts/bin/Microsoft.AspNetCore.Http.Extensions/Release/net7.0/Microsoft.AspNetCore.Http.Extensions.dll' because it is being used by another process. 

@HaoK
Copy link
Member Author

HaoK commented Feb 24, 2022

@wtgodbe looks like I'm getting an error about 64bit using a 32bit in Product.wxs not sure why this one is complaining...

Looks like you added these recently for microsoft update, any ideas what I need to do to make this happy? SO claims this might be safe to ignore since we are building cross bitness installers with these wxs files

##[error]src\Installers\Windows\HostOptions\Product.wxs(53,0): error LGHT0204: (NETCORE_ENGINEERING_TELEMETRY=Build) ICE80: This 64BitComponent OPT uses 32BitDirectory MM

https://github.com/dotnet/aspnetcore/blob/haok/arm64/src/Installers/Windows/HostOptions/Product.wxs#L53

@wtgodbe
Copy link
Member

wtgodbe commented Feb 25, 2022

Do you have the error message? I think it should be safe to ignore too, but @joeloff may know for sure

@HaoK
Copy link
Member Author

HaoK commented Feb 25, 2022

Yeah its just

D:\a\_work\1\s\src\Installers\Windows\HostOptions\Product.wxs(53): error LGHT0204: ICE80: This 64BitComponent OPT uses 32BitDirectory MM [D:\a\_work\1\s\src\Installers\Windows\HostOptions\HostOptions.wixproj]
##[error]src\Installers\Windows\HostOptions\Product.wxs(53,0): error LGHT0204: (NETCORE_ENGINEERING_TELEMETRY=Build) ICE80: This 64BitComponent OPT uses 32BitDirectory MM

From https://dev.azure.com/dnceng/public/_build/results?buildId=1631437&view=logs&jobId=1b89928a-2219-5ef9-602f-f95beb3da4dc&j=1b89928a-2219-5ef9-602f-f95beb3da4dc&t=39263dad-2be6-5f40-bde4-1bab0d7931b4

@joeloff
Copy link
Member

joeloff commented Feb 25, 2022

Yeah its just

D:\a\_work\1\s\src\Installers\Windows\HostOptions\Product.wxs(53): error LGHT0204: ICE80: This 64BitComponent OPT uses 32BitDirectory MM [D:\a\_work\1\s\src\Installers\Windows\HostOptions\HostOptions.wixproj]
##[error]src\Installers\Windows\HostOptions\Product.wxs(53,0): error LGHT0204: (NETCORE_ENGINEERING_TELEMETRY=Build) ICE80: This 64BitComponent OPT uses 32BitDirectory MM

From https://dev.azure.com/dnceng/public/_build/results?buildId=1631437&view=logs&jobId=1b89928a-2219-5ef9-602f-f95beb3da4dc&j=1b89928a-2219-5ef9-602f-f95beb3da4dc&t=39263dad-2be6-5f40-bde4-1bab0d7931b4

You should fix this. Again, preprocessor condition that defines something like PFile=ProgramFilesFolder or ProgramFilesFolder64. The chain of directories will impact the GUID generation for components, so do not suppress the ICE error.

@joeloff
Copy link
Member

joeloff commented Feb 25, 2022

@joeloff
Copy link
Member

joeloff commented Feb 25, 2022

Do you need to be concerned about support arm64 and x64 SxS on the same machine like we do with the runtimes/SDK? Because arm64 and x64 share the same hive, you have to do additional work to make sure x64 on arm64 installs into a subfolder, though I'm not sure what that implication would be for ANCM and if IIS even supports SxS with emulation.

@HaoK
Copy link
Member Author

HaoK commented Feb 25, 2022

You should fix this. Again, preprocessor condition that defines something like PFile=ProgramFilesFolder or ProgramFilesFolder64. The chain of directories will impact the GUID generation for components, so do not suppress the ICE error.

Can you clarify what the fix we need to make here is? so is this complaining that some folders are not using the 64bit ones?

Do you need to be concerned about support arm64 and x64

I'm not sure, does it seem unreasonable for us to just only support arm64 and not drop x64 binaries on an arm64 machine? This might be something we need to ask the IIS team about I guess.

@joeloff
Copy link
Member

joeloff commented Feb 26, 2022

You should fix this. Again, preprocessor condition that defines something like PFile=ProgramFilesFolder or ProgramFilesFolder64. The chain of directories will impact the GUID generation for components, so do not suppress the ICE error.

Can you clarify what the fix we need to make here is? so is this complaining that some folders are not using the 64bit ones?

Yes, for x64/arm64 you want to use the 64-bit versions of everything, including registry keys, unless you specifically need to target the 32-bit registry hive. For files, x64 and arm64 should use ProgramFiles64Folder always.

Starting from the SourceDir at the top, the full path is constructed for each file, this is known as the keypath. That value is then used to generate a stable GUID for the component which impacts things like reference counts, two products that install a component with the same key path must have the same GUID.

Do you need to be concerned about support arm64 and x64

I'm not sure, does it seem unreasonable for us to just only support arm64 and not drop x64 binaries on an arm64 machine? This might be something we need to ask the IIS team about I guess.

Definitely ask IIS team. I can imagine that just like hosting both x86 and x64 app pools someone might want to host arm64 and x64 app pools on the same machine. But I have no idea. There was a pretty big drive late in .NET 6 around x64 emulation on arm64 so that's why I brought it up.

@HaoK
Copy link
Member Author

HaoK commented Feb 28, 2022

@dougbu do we have a weird build race condition here? My understanding of what I thought should happen was buildx64/arm64 should have already built the ANCM/outofproc dlls, I see that in the logs that all 3 x86/arm64/x86 ANCM is built properly...

But the installers would fail not seeing the arm64 ANCM dlls, so I got rid of the -nobuildNative, which now results in something in the installers build trying to consume outofproc dll while its trying to build it??

From the build installers job:

LINK : fatal error LNK1104: cannot open file 'D:\a\_work\1\s\artifacts\bin\OutOfProcessRequestHandler\arm64\Release\aspnetcorev2_outofprocess.dll' [D:\a\_work\1\s\src\Servers\IIS\AspNetCoreModuleV2\OutOfProcessRequestHandler\OutOfProcessRequestHandler.vcxproj]
##[error]LINK(0,0): error LNK1104: (NETCORE_ENGINEERING_TELEMETRY=Build) cannot open file 'D:\a\_work\1\s\artifacts\bin\OutOfProcessRequestHandler\arm64\Release\aspnetcorev2_outofprocess.dll'
  Finished generating code
  OutOfProcessRequestHandler.vcxproj -> D:\a\_work\1\s\artifacts\bin\OutOfProcessRequestHandler\ARM64\Release\aspnetcorev2_outofprocess.dll

Why is it trying to link before building the dll (or is this in parallel?)

@dougbu
Copy link
Member

dougbu commented Feb 28, 2022

@HaoK the native projects shouldn't need to rebuild for the installers. Something is wonky with either OutOfProcessRequestHandler.vcxproj or the properties it's passed in this case. A binary log should help find what's causing the build and checking what else is running at the same time.

@HaoK
Copy link
Member Author

HaoK commented Mar 1, 2022

Okay I guess it was some weirdness with building native twice, turning off parallel seems to be sufficient, i manually tried the installers for ANCM + hosting bundle + ANCM+IISExpress and they all behave as expected on the arm64 machine (IISExpress will fail since no matching verison of IIS installed).

For the purposes of the first pass of this PR (working/building installer), I think we should consider this mergeable

@HaoK HaoK changed the title [WIP] Update installer/tests for ARM64 Working arm64 installers Mar 1, 2022
@HaoK HaoK marked this pull request as ready for review March 1, 2022 00:31
@HaoK HaoK changed the title Working arm64 installers Working ANCM arm64 installers Mar 1, 2022
.azure/pipelines/ci.yml Outdated Show resolved Hide resolved
@@ -2,13 +2,7 @@

<Include>
<?define DiscoverabilityKeyRoot = "SOFTWARE\Microsoft\IIS Extensions"?>
<?define OOBBuildVersion = $(var.BLDVERMAJOR).$(var.BLDVERMINOR).$(var.BLDNUMMAJOR).$(var.BLDNUMMINOR)?>
Copy link
Member Author

Choose a reason for hiding this comment

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

Note: this was causing build issues from the HostOptions stuff that are now also reusing this wxi to be able to properly get the right 64/32 bit ProgramFiles, it doesn't look like we are using OOBBuildVersion anywhere since it was fine to just remove it (as BLDVERMINOR/MAJOR aren't being defined everywhere else). So I think this was just dead definitions

@@ -17,18 +11,23 @@
<?define SystemFolder = System64Folder ?>
<?define SysWow64Folder = SystemFolder ?>
<?define PlatformValue = x64 ?>
<?define ANCMInstallerVersion = 400 ?>
Copy link
Member Author

Choose a reason for hiding this comment

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

We seem to be diverging from the Directory.Build.props which set installer version to 200 or 500, so I kept this the way we were before hardcoding to 400, but we need to make it at least 500 for ARM64, hence why we don't just use InstallerVersion here

Copy link
Member

Choose a reason for hiding this comment

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

Most supported OS versions should have MSI 5.0 installed, but we've mostly stuck to 2.0 for some reason. It's possible that ANCM required features that were introduced in 4.0 which is why they default to 400, but I can't say for sure.

<PropertyGroup>
<AdditionalIncludeDirectories>$(IIS-Common)version;$(IIS-Common)Include;$(AdditionalIncludeDirectories)</AdditionalIncludeDirectories>
<PrecompiledHeader>Create</PrecompiledHeader>
<PrecompiledHeaderFile>precomp.h</PrecompiledHeaderFile>
<PrecompiledHeaderOutputFile>$(IntDir)precomp.pch</PrecompiledHeaderOutputFile>
</PropertyGroup>
<PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Release|ARM64'">
<TargetName>$(ProjectName)</TargetName>
Copy link
Member Author

Choose a reason for hiding this comment

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

This is another example of ARM64 perhaps not being fully correct, as TargetName doesn't seem to be defined for ARM64, where the other projects picked up the correct default

eng/Build.props Outdated Show resolved Hide resolved
@HaoK
Copy link
Member Author

HaoK commented Mar 5, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@HaoK HaoK merged commit b4ccd7b into main Mar 5, 2022
@ghost ghost added this to the 7.0-preview3 milestone Mar 5, 2022
HaoK added a commit that referenced this pull request Mar 6, 2022
HaoK added a commit that referenced this pull request Mar 6, 2022
HaoK added a commit that referenced this pull request Mar 6, 2022
@amcasey 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
@wtgodbe wtgodbe deleted the haok/arm64 branch November 15, 2023 17:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants