-
Notifications
You must be signed in to change notification settings - Fork 10.2k
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
Conversation
@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:
|
src/Installers/Windows/AspNetCoreModule-Setup/ANCMIISExpressV2/ancm_iis_expressv2.wxs
Show resolved
Hide resolved
@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 |
Do you have the error message? I think it should be safe to ignore too, but @joeloff may know for sure |
Yeah its just
|
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. |
Take a look at https://github.com/dotnet/installer/blob/661fdd30e9ba5de39b430d879d46ce00323a0ad5/src/redist/targets/packaging/windows/clisdk/variables.wxi#L26 for how to leverage the preprocessor |
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. |
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?
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. |
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
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. |
@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:
Why is it trying to link before building the dll (or is this in parallel?) |
@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. |
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 |
@@ -2,13 +2,7 @@ | |||
|
|||
<Include> | |||
<?define DiscoverabilityKeyRoot = "SOFTWARE\Microsoft\IIS Extensions"?> | |||
<?define OOBBuildVersion = $(var.BLDVERMAJOR).$(var.BLDVERMINOR).$(var.BLDNUMMAJOR).$(var.BLDNUMMINOR)?> |
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.
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 ?> |
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.
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
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.
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> |
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.
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
This reverts commit a37bad2.
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
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:
Next PR will start making things light up (and work).