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

Fix build on arm64 machines #37374

Merged
merged 3 commits into from
Oct 8, 2021
Merged

Conversation

omajid
Copy link
Member

@omajid omajid commented Oct 8, 2021

Building ASP.NET Core on an arm64 machine using ./build.sh -arch arm64 leads to a dependency error because crossgen2 is not listed in eng/Dependencies.props.

Building ASP.NET Core on an arm64 machine leads to a dependency error
because crossgen2 is not listed in eng/Dependencies.props.
@omajid omajid requested a review from a team as a code owner October 8, 2021 04:07
@dougbu
Copy link
Member

dougbu commented Oct 8, 2021

This might be a valid problem to fix @omajid but I'm not seeing any difference in the crossgen2 output near the end of the Linux_arm64 job in your validation build 20211007.33 and a recent build of 'main' e.g. 20211007.27.

Could you please describe in more detail what fails without this fix❔

@dougbu
Copy link
Member

dougbu commented Oct 8, 2021

@davidwrighton

  1. Are the complications in
    <!--
    Determine the crossgen2 package path property name. Special case linux-musl-arm and linux-musl-arm64 because they
    are built on an Ubuntu container with cross compilation tools. linux-musl-x64 is built in an alpine container.
    Special case the crossgen2 package reference on Windows to avoid the x86 package when building in Visual Studio.
    -->
    <BuildOsName>$(TargetOsName)</BuildOsName>
    <BuildOsName Condition="'$(TargetOsName)' == 'linux-musl' and '$(TargetArchitecture)'!='x64'">linux</BuildOsName>
    <Crossgen2BuildArchitecture Condition=" '$(BuildOsName)' == 'win' ">x64</Crossgen2BuildArchitecture>
    <Crossgen2BuildArchitecture Condition=" '$(Crossgen2BuildArchitecture)' == '' ">$(BuildArchitecture)</Crossgen2BuildArchitecture>
    <Crossgen2PackageRootVariableName>PkgMicrosoft_NETCore_App_Crossgen2_$(BuildOsName)-$(Crossgen2BuildArchitecture)</Crossgen2PackageRootVariableName>
    still necessary and correct❔
  2. Can those complications break people building on platforms other than what we use in the CI❔
  3. Should we list all crossgen2 packages found in the dotnet7 feed in eng/Dependencies.props❔ Currently missing linux-arm, linux-arm64, linux-musl-arm, linux-musl-arm64, win-arm, and win-x86.

Copy link

@mesutozturk mesutozturk left a comment

Choose a reason for hiding this comment

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

ok

@omajid
Copy link
Member Author

omajid commented Oct 8, 2021

Could you please describe in more detail what fails without this ?

From the CI build log:

/home/vsts/work/1/s/eng//build.sh --ci --nobl --configuration Release --arch arm64 --all --pack --no-build-nodejs --no-build-java -p:OnlyPackPlatformSpecificPackages=true -p:AssetManifestFileName=aspnetcore-Linux_arm64.xml /p:SkipTestBuild=true /p:PostBuildSign=true  
========================== Starting Command Output ===========================
/bin/bash --noprofile --norc /home/vsts/work/_temp/0f217d4e-ea7b-4fab-b5ba-7d6737c60938.sh
warning: Some managed projects depend on NodeJS projects. Building NodeJS is disabled so the managed projects will fallback to using the output from previous builds. The output may not be correct or up to date.
Downloading 'https://dotnet.microsoft.com/download/dotnet/scripts/v1/dotnet-install.sh'
dotnet-install: Note that the intended use of this script is for Continuous Integration (CI) scenarios, where:
dotnet-install: - The SDK needs to be installed without user interaction and without admin rights.
dotnet-install: - The SDK installation doesn't need to persist across multiple CI runs.
dotnet-install: To set up a development environment or to run apps, use installers rather than this script. Visit https://dotnet.microsoft.com/download to get the installer.

dotnet-install: Downloading primary link https://dotnetcli.azureedge.net/dotnet/Sdk/7.0.100-alpha.1.21474.3/dotnet-sdk-7.0.100-alpha.1.21474.3-linux-x64.tar.gz

That means the CI leg is running on an x64 machine but cross compiling for arm64. So it ends up using the x64 crossgen (which is present in eng/Dependencies.props) for processing artifacts.

I am building on arm64 hardware and targeting arm64, not cross compiling on x64. Without this change, this is how the build fails on my machine:

/home/omajid/aspnetcore/eng/targets/ResolveReferences.targets(220,5): error MSB3245: Could not resolve this reference. Could not locate the package or project for "Microsoft.NETCore.App.Crossgen2.linux-arm64". Did you update baselines and dependencies lists? See docs/ReferenceResolution.md for more details. [/home/omajid/aspnetcore/src/Framework/App.Runtime/src/Microsoft.AspNetCore.App.Runtime.csproj]

That's because $(Crossgen2BuildArchitecture) is arm64 and the arm64 RID-specific crossgen2 nupkg is not listed as a dependency.

Are the complications in aspnetcore/src/Framework/App.Runtime/src/Microsoft.AspNetCore.App.Runtime.csproj , still necessary and correct ?

They seem necessary to support building for arm64 in CI (which runs on an x64 machine but builds for arm64). I am not sure if they are quite correct. They seem to mix CI limitations (eg, "when building for linux-musl, assume we are actually on linux") with normal build ("ignore if the build os is linux-musl, assume it's always linux"). Maybe these conditions should first check if target platform is not the same as the build platform?

Can those complications break people building on platforms other than what we use in the CI?

Looking at the lines in aspnetcore/src/Framework/App.Runtime/src/Microsoft.AspNetCore.App.Runtime.csproj , I can see some failure scenarios:

  • Building on arm64 (or any another non-x64 architecture) OS that's using musl libc will use linux instead of linux-musl RID, probably ending up with a crossgen that doesn't work.
  • Building on Windows on any non-x64 architecture will lead to x64 assets being used, which will end up with a crossgen that doesn't work.

Should we list all crossgen2 packages found in the dotnet7 feed in eng/Dependencies.props ?

Seems like the right thing to do, if we want to support building ASP.NET Core on those platforms.

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.

Thanks very much for the added information @omajid❔ Let's at least cover all available variants of the crossgen2 package in this PR.

I'm not positive how best to handle the code in Microsoft.AspNetCore.App.Runtime.csproj but that can wait. Likely we should make some of that understand the actual OS architecture. Could you please open an issue listing the potential problems you see there❔ (mention me in it so I don't miss it 😄)

eng/Dependencies.props Show resolved Hide resolved
eng/Dependencies.props Show resolved Hide resolved
omajid and others added 2 commits October 8, 2021 13:00
Add all arm/arm64 variants

Co-authored-by: Doug Bunting <[email protected]>
Add all win variants for crossgen2

Co-authored-by: Doug Bunting <[email protected]>
@dougbu dougbu enabled auto-merge (squash) October 8, 2021 17:09
@dougbu dougbu merged commit 64f0642 into dotnet:main Oct 8, 2021
@ghost ghost added this to the 7.0-preview1 milestone Oct 8, 2021
@Pilchie
Copy link
Member

Pilchie commented Oct 8, 2021

Is this interesting to consider for release/6.0 or is main sufficient?

@dougbu
Copy link
Member

dougbu commented Oct 8, 2021

Up to you @Pilchie. The fix here and however we address #37400 will have minimal impact on our pipeline builds but will reduce barriers for contributors building the shared Fx on other systems. Let me know if we should backport this.

@Pilchie
Copy link
Member

Pilchie commented Oct 8, 2021

Let's do it. ARM64 hardware is only going to get more common during the life of .NET 6, and I can imagine people building the branch/tags and being confused.

@dougbu
Copy link
Member

dougbu commented Oct 8, 2021

/backport to release/6.0

@github-actions
Copy link
Contributor

github-actions bot commented Oct 8, 2021

Started backporting to release/6.0: https://github.com/dotnet/aspnetcore/actions/runs/1321679215

@github-actions
Copy link
Contributor

github-actions bot commented Oct 8, 2021

@dougbu an error occurred while backporting to release/6.0, please check the run log for details!

Error: @dougbu is not a repo collaborator, backporting is not allowed.

@dougbu
Copy link
Member

dougbu commented Oct 8, 2021

@Pilchie can you do the needful please❔ GitHub hasn't fixed their collaborator lookups

@Pilchie
Copy link
Member

Pilchie commented Oct 8, 2021

/backport to release/6.0

@github-actions
Copy link
Contributor

github-actions bot commented Oct 8, 2021

Started backporting to release/6.0: https://github.com/dotnet/aspnetcore/actions/runs/1321947322

@github-actions
Copy link
Contributor

github-actions bot commented Oct 8, 2021

@Pilchie backporting to release/6.0 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: Fix build on an arm64 machine
Applying: Update eng/Dependencies.props
Applying: Update eng/Dependencies.props
error: sha1 information is lacking or useless (eng/Dependencies.props).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0003 Update eng/Dependencies.props
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

@dougbu
Copy link
Member

dougbu commented Oct 8, 2021

Please backport manually!

I'll take this.

dougbu pushed a commit to dougbu/aspnetcore that referenced this pull request Oct 8, 2021
- backport of dotnet#37374, `cherry-pick` of 64f0642

Add more `crossgen2` dependencies (dotnet#37374)

  * Fix build on an arm64 machine
    * Building ASP.NET Core on an arm64 machine leads to a dependency error
      because `crossgen2` is not listed in eng/Dependencies.props.
  * Add all arm/arm64 variants
  * Add all win variants for `crossgen2`

  Co-authored-by: Doug Bunting <[email protected]>
dougbu added a commit that referenced this pull request Oct 8, 2021
- backport of #37374, `cherry-pick` of 64f0642

Add more `crossgen2` dependencies (#37374)

  * Fix build on an arm64 machine
    * Building ASP.NET Core on an arm64 machine leads to a dependency error
      because `crossgen2` is not listed in eng/Dependencies.props.
  * Add all arm/arm64 variants
  * Add all win variants for `crossgen2`

  Co-authored-by: Doug Bunting <[email protected]>

Co-authored-by: Omair Majid <[email protected]>
@omajid
Copy link
Member Author

omajid commented Oct 9, 2021

A little late to the party here, but thanks for backporting it to release/6.0! I ran into this bug while building source-build (including ASP.NET Core) on arm64, and it's definitely affecting us (Red Hat) for 6.0. We are carrying a patch locally for 6.0 to bypass this, so it's great to see an official fix landing.

@ghost
Copy link

ghost commented Oct 9, 2021

Hi @omajid. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants