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

Linux-bionic (Android libc w/o Android interop) initial implementation #66147

Merged
merged 209 commits into from
May 19, 2022

Conversation

directhex
Copy link
Contributor

@directhex directhex commented Mar 3, 2022

Implement support for "Linux Bionic", a RID which builds a Linux runtime against an Android libc but without Android OS functions like JNI, Dalvik, Instrumentation, etc. This configuration has been requested by a team working on an IoT scenario.

  • The new RID is linux-bionic-arm64/linux-bionic-x64.
  • The build reuses the existing Android build dockerfiles and Helix queues
  • Since we don't have JNI, we don't have Android Crypto - instead, use an Android build of OpenSSL. As an errata, it is incumbent upon the end user to supply this in their OS environment (we download one ad-hoc for testing purposes, which was compiled by Google)
  • Some tests cannot be completed due to SELinux policies affecting the shell user executing tests via adb shell in the world-writable /data/local/tmp directory. We expect those tests would pass if they pass on Android, in a real-world IoT scenario with a custom OS (rather than recycling Google's OS images intended for phones)

Closes: #66027

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@ghost ghost assigned directhex Mar 3, 2022
@ghost
Copy link

ghost commented Mar 3, 2022

Tagging subscribers to this area: @directhex
See info in area-owners.md if you want to be subscribed.

Issue Details

I expect I broke something along the way, so let's see just how badly.

Closes: #66027

Author: directhex
Assignees: directhex
Labels:

area-Infrastructure-mono

Milestone: -

@directhex
Copy link
Contributor Author

Urgh, horrible merge failure means I spent hours cleaning up unneeded changes, and they reappeared anyway

@directhex
Copy link
Contributor Author

New regression: SSL doesn't work because an Android System.Net.Security is being used instead of a Linux one, which causes it to forcibly load Android SSL instead of OpenSSL.

@steveisok
Copy link
Member

steveisok commented Mar 7, 2022

New regression: SSL doesn't work because an Android System.Net.Security is being used instead of a Linux one, which causes it to forcibly load Android SSL instead of OpenSSL.

I don't think parts of System.Net.Security are going to work on Android anyway. At least not in .NET 6.

@directhex
Copy link
Contributor Author

New regression: SSL doesn't work because an Android System.Net.Security is being used instead of a Linux one, which causes it to forcibly load Android SSL instead of OpenSSL.

I don't think System.Net.Security is going to work on Android anyway. At least not in .NET 6.

That's kinda the point. It worked yesterday when the build system assumed Bionic was Linux, and it's broken today now the build system thinks Bionic is Android, and I'm trying to figure out how to massage that in the build system to behave better (msbuild log viewer shows none of my properties in System.Net.Security.csproj are getting resolved, something odd is going on)

@directhex
Copy link
Contributor Author

Tweak this and you should be ok: https://github.com/directhex/runtime/blob/78111b553b2546356130a1d7ed33c944f3a94e46/src/libraries/System.Net.Security/src/System.Net.Security.csproj#L14

You'd think! And yet

Screenshot from 2022-03-07 15-11-40

Hence "and something odd is going on"

@directhex
Copy link
Contributor Author

Looks like the problem is with the structured log viewer, I see more via /v:diag

@directhex
Copy link
Contributor Author

Okay, so, need help from someone who understands the libraries build a little better here (@ericstj?)

  • The build doesn't really support OS subgroups (i.e. linux-musl, linux-bionic), it's a gross hack where the system thinks it's actually the OSGroup OS (Linux for musl), some magic autodetection code in a few shell scripts tells it otherwise and causes changes in build behaviour, and the RID gets overridden post-hoc via /p:OutputRID. I've removed this assumption in a few places and parsed /p:RuntimeOS, because it's not possible to autodetect the difference between Android and linux-bionic based on anything in the rootfs.
  • We still need a TargetOS (--os). The linux-musl build defaults to linux; in our case there's a case to be made for both linux and android. And we kinda need to be both, in a few places - System.Net.Security and System.Security.Cryptography we need the Linux version NOT the Android version, whereas System.Console we want the Android version. Do we want to add a separate TFM for linux-bionic? Use RuntimeOS conditionals in the relevant csproj? What's the least painful way to do this?

@directhex
Copy link
Contributor Author

Okay, there's a few small bits & pieces to clean up, but the broad strokes are there (e.g. the on-device build is green and the tests are published), so I'm gonna mark this ready for review. Hopefully we can nail down any lingering issues by the end of the week, and clean up any questionable conditions to everyone's satisfaction.

@directhex directhex marked this pull request as ready for review May 11, 2022 19:52
@directhex directhex requested a review from am11 May 12, 2022 14:55
Copy link
Member

@am11 am11 left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you.

src/native/libs/CMakeLists.txt Show resolved Hide resolved
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.

Great jobs, I left a couple comments but this is really close :)

Directory.Build.props Outdated Show resolved Hide resolved
eng/Subsets.props Outdated Show resolved Hide resolved
eng/Subsets.props Outdated Show resolved Hide resolved
eng/native/naming.props Outdated Show resolved Hide resolved
eng/pipelines/common/global-build-job.yml Outdated Show resolved Hide resolved
@@ -83,6 +83,8 @@
#define FALLBACK_HOST_RID _X("solaris")
#elif defined(TARGET_LINUX_MUSL)
#define FALLBACK_HOST_RID _X("linux-musl")
#elif defined(TARGET_ANDROID)
Copy link
Member

Choose a reason for hiding this comment

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

it'd probably be better to introduce a TARGET_LINUX_BIONIC define instead.

<_MonoRunInitCompiler>false</_MonoRunInitCompiler>
</PropertyGroup>
<ItemGroup Condition="'$(TargetsAndroid)' == 'true'">
<ItemGroup Condition="'$(TargetsLinuxBionic)' == 'true'">
<_MonoCPPFLAGS Include="-DANDROID_FORCE_ICU_DATA_DIR" />
Copy link
Member

Choose a reason for hiding this comment

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

nit: call this FORCE_ANDROID_ICU_DATA_DIR to make it consistent with FORCE_ANDROID_OPENSSL (or change the other one, I don't have a preference)

Copy link
Member

Choose a reason for hiding this comment

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

probably better to move this to the src/native CMakeFiles anyway though.

src/native/libs/build-native.proj Outdated Show resolved Hide resolved
@ghost ghost added the needs-author-action An issue or pull request that requires more info or actions from the author. label May 17, 2022
@ghost ghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label May 19, 2022
Directory.Build.props Outdated Show resolved Hide resolved
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.

Thanks, the rest of my comments can be addressed in followup-PRs

@directhex directhex merged commit b5f9c22 into dotnet:main May 19, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Jun 19, 2022
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.

Add support for Android Bionic runtime