Enable build on hosted arm64

[omajid]

This is attempt #2.

Initialize HostArch to the arch-style used in RIDs directly. Unless running in Visual Studio, when it should be forced to x64.

Initialize ArchGroup to HostArch unless overridden.

Use the HostArch for the tool runtime instead of assuming x64.

[omajid]

cc @joperezr @ViktorHofer

[omajid]

I dont have access to VS to verify this change :(

[ViktorHofer]

What about running VS on an x86 system?

[omajid]

@ViktorHofer Ah, good point. It would get forced to x64 🙁

Then again, it seems to me the current code (without my changes) forces it to x64 too:

    <HostArch>$([System.Runtime.InteropServices.RuntimeInformation]::ProcessArchitecture)</HostArch>
    <ArchGroup Condition="'$(ArchGroup)' == '' AND '$(HostArch)' == 'Arm'">arm</ArchGroup>
    <ArchGroup Condition="'$(ArchGroup)' == '' AND '$(HostArch)' == 'Arm64'">arm64</ArchGroup>
    <ArchGroup Condition="'$(ArchGroup)' == ''">x64</ArchGroup>

HostArch gets set to X86 and ArchGroup gets set to x64.

Do you know if building/testing with VS currently works on x86 systems out of the box?

[ViktorHofer]

Then again, it seems to me the current code (without my changes) forces it to x64 too

No, see the comment here: https://github.com/dotnet/corefx/blob/master/Directory.Build.props#L57-L63. We set that property based on the configuration string that is supplied by VS.

[joperezr]

Can you please explain what is blocking the build on a hosted arm64 machine today? Why can't you just call ./build.sh -arch arm64 or whatever arch you want? Building inside a hosted environment is not a common case, so I would try not to optimize for those as long as we have a way of building by passing in an extra arg.

[omajid]

I wrote about this in #40427, copying it here:

When I build a .NET Core component on linux-x64, I just do ./build.sh. I was hoping that things could be just as simple on other platforms. On linux-arm64, I would like to just do ./build.sh and have everything work out of the box without having to worry about runtime ids, architecture names or property names.

Is that an unrealistic goal?

@wfurt agreed:

I agree with @omajid that it would be nice if ./build.sh just work. In ideal case, all scripted cases can be parametrized but defaults for developers should be as simple as possible.

[joperezr]

On linux-arm64, I would like to just do ./build.sh and have everything work out of the box without having to worry about runtime ids, architecture names or property names.

I see. Why does this not work today though? if you are in linux-arm64 then HostOS should be Arm64 so should archgroup automatically default to arm64 as well?

[omajid]

Out of the box, ./build.sh fails on arm64 with this error:

/home/omajid/.nuget/packages/microsoft.net.sdk.il/5.0.0-alpha1.19413.7/targets/Microsoft.NET.Sdk.IL.targets(135,5): error MSB3073: The command "/home/omajid/corefx/artifacts/toolset/ilasm/ilasm -QUIET -NOLOGO -DLL  -INCLUDE=include/netcoreapp -OUTPUT="/home/omajid/corefx/artifacts/obj/System.Runtime.CompilerServices.Unsafe/netcoreapp-Debug/System.Runtime.CompilerServices.Unsafe.dll" -KEY="/home/omajid/.nuget/packages/microsoft.dotnet.arcade.sdk/1.0.0-beta.19416.16/tools/snk/MSFT.snk" System.Runtime.CompilerServices.Unsafe.il" exited with code 126. [/home/omajid/corefx/src/System.Runtime.CompilerServices.Unsafe/src/System.Runtime.CompilerServices.Unsafe.ilproj]  

it turns out the ilasm executible is bad:

$ file /home/omajid/corefx/artifacts/toolset/ilasm/ilasm
/home/omajid/corefx/artifacts/toolset/ilasm/ilasm: ELF 64-bit LSB executable, x86-64, version 1 (SYSV), dynamically linked, interpreter /lib64/ld-linux-x86-64.so.2, for GNU/Linux 2.6.32, BuildID[sha1]=6b99261d576d77b0cb852e8d30e328a3a12f1e80, not stripped

It's incorrectly using the ilasm binary for x64 instead of the binary on arm64.

A hardcoded fix for that is:

-    <ToolRuntimeRID>$(_runtimeOS)-x64</ToolRuntimeRID>
+    <ToolRuntimeRID>$(_runtimeOS)-arm64</ToolRuntimeRID>

But that will only work on arm64. If I generalize this to:

-    <ToolRuntimeRID>$(_runtimeOS)-x64</ToolRuntimeRID>
+    <ToolRuntimeRID>$(_runtimeOS)-$(ArchGroup)</ToolRuntimeRID>

Then that (AFAIK) breaks cross compilation - because we want to use x64 tools when building arm64 on x64.

So we need to use architecture of the platform that is running the build - $(HostArch.ToString.ToLowerInvariant()) - here.

My fix then goes (maybe too far?) and sets ArchGroup (which can take values of x86, x64, arm64 and arm) explicitly based on HostArch.

@tmds suggested offline that maybe we could only set ArchGroup to $(HostArch) on non-Windows platforms? That might take care of the issues with VS architecture?

[omajid]

I am also interested in fixing up how we use the host architecture within our build scripts to handle potential new architectures in the future without further changes.

[joperezr]

I see, if all that needs in order to unblock your scenario is to change ToolRuntimeRID, then can't we just scope the change to that property? As in, can't we just have something like:

<ToolRuntimeRID>$(_runtimeOS)-$(HostArch.ToString.ToLowerInvariant())</ToolRuntimeRID>

[omajid]

Yeah, that seems like a good idea. Do you know how that would affect VS on x86/x64?

[joperezr]

Do you know how that would affect VS on x86/x64?

Oh you are right that would probably make VS build not work for that case. Perhaps we could keep the default to x64 and add a condition to set it to the above if BuildingInsideVisualStudio is not set, so basically similar to your current approach except that we would be narrowing down the impact by setting ToolRuntimeRID instead of ArchGroup.

[omajid]

I have verified that the my latest changes build on arm64 as well as x86_64. Any thoughts on it?

[omajid]

@safern, this is the updated version of #40311

[omajid]

Anyone have any thoughts on this?

[joperezr]

I think I'm fine with the change as it is. We might still be missing a special case where this would break (for example cross builds or similar) but I believe that if we do find such a case we can fix it easily. @ericstj @ViktorHofer @safern any additional thoughts? or is this good to go?

[ViktorHofer]

I'm ok with the change as well. Thanks for boxing this through.

[ViktorHofer]

Re-running the CI according to our merge policies.

[safern]

Looks good to me.

[ViktorHofer]

Thanks

[omajid]

Thanks for the review and merging this, folks! Please do comment here (or in another issue and cc me) if this causes any other issues.