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

Add NativeAOT build and runtime pack for linux-bionic #86781

Merged
merged 3 commits into from
May 27, 2023

Conversation

MichalStrehovsky
Copy link
Member

Build and runtime pack for linux-bionic (Android without the Java part).

I tried not to regress the existing CoreCLR Android build that is based on a crossrootfs (#56622) - the if's basically deal with that. Note that it's likely broken anyway (#66562).

Cc @dotnet/ilc-contrib

Build and runtime pack for linux-bionic (Android without the Java part).

I tried not to regress the existing CoreCLR Android build that is based on a crossrootfs (dotnet#56622) - the if's basically deal with that. Note that it's likely broken anyway (dotnet#66562).
@ghost
Copy link

ghost commented May 26, 2023

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

Issue Details

Build and runtime pack for linux-bionic (Android without the Java part).

I tried not to regress the existing CoreCLR Android build that is based on a crossrootfs (#56622) - the if's basically deal with that. Note that it's likely broken anyway (#66562).

Cc @dotnet/ilc-contrib

Author: MichalStrehovsky
Assignees: -
Labels:

area-NativeAOT-coreclr

Milestone: -

@steveisok
Copy link
Member

@MichalStrehovsky can you kick off an official build of this branch to make sure it's ok?

@MichalStrehovsky
Copy link
Member Author

@MichalStrehovsky can you kick off an official build of this branch to make sure it's ok?

Triggered this build: https://dnceng.visualstudio.com/internal/_build/results?buildId=2188707&view=results

I think it looks good? It failed at signing, but I assume it's because I triggered it and it's not the main branch.

Copy link
Member

@akoeplinger akoeplinger left a comment

Choose a reason for hiding this comment

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

One question but looks good to me otherwise.

@@ -84,6 +84,8 @@ build_native()
exit 1
fi

cmakeArgs="-C $__RepoRootDir/eng/native/tryrun.cmake $cmakeArgs"
Copy link
Member

Choose a reason for hiding this comment

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

why is this necessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

The same reason we had to add a tryrun for iDevices - cmake cannot execute these detectoids because it's a crossbuild. Normally the infra passes tryrun to crossbuild but this is not considered a crossbuild by the infra (see the first issue I linked in top post - we don't pass -cross to build.sh to build Bionic)

Copy link
Member

Choose a reason for hiding this comment

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

Yes, what I meant was why is this not necessary for the mono-based Android build today?

I looked a bit deeper now and I think it is because we're not using any of these cmake checks in the System.Native build so we don't hit this, e.g. we explicitly skip the checks that would cause binaries to be run during configure on mobile and manually set the results:

if(CLR_CMAKE_TARGET_IOS)
# Manually set results from check_c_source_runs() since it's not possible to actually run it during CMake configure checking
unset(HAVE_SHM_OPEN_THAT_WORKS_WELL_ENOUGH_WITH_MMAP)
unset(HAVE_ALIGNED_ALLOC) # only exists on iOS 13+
unset(HAVE_CLOCK_MONOTONIC) # only exists on iOS 10+
unset(HAVE_CLOCK_REALTIME) # only exists on iOS 10+
unset(HAVE_FORK) # exists but blocked by kernel
elseif(CLR_CMAKE_TARGET_MACCATALYST)
# Manually set results from check_c_source_runs() since it's not possible to actually run it during CMake configure checking
# TODO: test to see if these all actually hold true on Mac Catalyst
unset(HAVE_SHM_OPEN_THAT_WORKS_WELL_ENOUGH_WITH_MMAP)
unset(HAVE_ALIGNED_ALLOC) # only exists on iOS 13+
unset(HAVE_CLOCK_MONOTONIC) # only exists on iOS 10+
unset(HAVE_CLOCK_REALTIME) # only exists on iOS 10+
unset(HAVE_FORK) # exists but blocked by kernel
elseif(CLR_CMAKE_TARGET_TVOS)
# Manually set results from check_c_source_runs() since it's not possible to actually run it during CMake configure checking
unset(HAVE_SHM_OPEN_THAT_WORKS_WELL_ENOUGH_WITH_MMAP)
unset(HAVE_ALIGNED_ALLOC) # only exists on iOS 13+
unset(HAVE_CLOCK_MONOTONIC) # only exists on iOS 10+
unset(HAVE_CLOCK_REALTIME) # only exists on iOS 10+
unset(HAVE_FORK) # exists but blocked by kernel
elseif(CLR_CMAKE_TARGET_ANDROID)
# Manually set results from check_c_source_runs() since it's not possible to actually run it during CMake configure checking
unset(HAVE_SHM_OPEN_THAT_WORKS_WELL_ENOUGH_WITH_MMAP)
unset(HAVE_ALIGNED_ALLOC) # only exists on newer Android
set(HAVE_CLOCK_MONOTONIC 1)
set(HAVE_CLOCK_REALTIME 1)
elseif(CLR_CMAKE_TARGET_BROWSER OR CLR_CMAKE_TARGET_WASI)
set(HAVE_FORK 0)
else()
if(CLR_CMAKE_TARGET_OSX)
unset(HAVE_ALIGNED_ALLOC) # only exists on OSX 10.15+
else()

So this is fine since we'll override the values for e.g. HAVE_SHM_OPEN_THAT_WORKS_WELL_ENOUGH_WITH_MMAP but we should probably clean this up and move all of the detection into tryrun.cmake. Would you mind filing an issue for that?

Copy link
Member Author

Choose a reason for hiding this comment

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

Filed #86829.

@akoeplinger
Copy link
Member

Triggered this build: dnceng.visualstudio.com/internal/_build/results?buildId=2188707&view=results

I think it looks good? It failed at signing, but I assume it's because I triggered it and it's not the main branch.

It should work for non-main builds as well but I checked and I've seen the same error on some 8.0-preview5 branch builds as well so I think it is unrelated.

@MichalStrehovsky MichalStrehovsky merged commit 7173823 into dotnet:main May 27, 2023
@MichalStrehovsky MichalStrehovsky deleted the bionic branch May 27, 2023 07:38
@ghost ghost locked as resolved and limited conversation to collaborators Jun 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants