[Android NDK] Make API level configurable#124461
Conversation
There was a problem hiding this comment.
Pull request overview
Makes the Android NDK API level used by NativeAOT’s Unix build integration configurable, addressing link failures caused by hardcoding API level 21 when the runtime is built against a higher NDK level.
Changes:
- Introduce
CrossCompileAndroidApiLevelMSBuild property with a default of24. - Replace hardcoded
android21/androideabi21target triple components withandroid$(CrossCompileAndroidApiLevel)/androideabi$(CrossCompileAndroidApiLevel).
|
Tagging subscribers to this area: @agocke, @dotnet/ilc-contrib |
|
@MichalStrehovsky I'll modify the PR to set it back to 21, there's no need to make the bump right now across the entire runtime, it was just a "by the way" change. With regards to syncing, we should in general keep the levels compatible between what dotnet/runtime uses and what's used by .NET for Android, but this is not to say they have to be set to the same value. API 21 and 24 aren't compatible on the binary level, so this is a situation where we need to be in agreement and use 24 on the NativeAOT linking side (CoreCLR host in .NET for Android will be fine, even though we build against 24 and libcoreclr.so is built against 21). I wouldn't make using the same API level on both sides a matter of policy, I'd recommend we keep the two compatible, even if they would differ. Tests we have on the Android side will be able to tell us if there are incompatibilities. |
Context: bef768d Context: dotnet/runtime#124461 Follow up to bef768d which puts the non-mono builds API level in several more locations throughout the source. This is necessary for NativeAOT builds to correctly link as they **must** use the same API level `.NET for Android` was built with. Failure to do so may result in missing native symbols when linking. NativeAOT builds now use full path to the linker (in our case `clang++`) and pass the `--target=${ARCH}-linux-android${API}` argument to it, just like the toolchain wrapper scripts in the NDK do. Without the parameter, the lowest supported API level will be used (currently 21).
|
Workflow state for the Holistic Review Orchestrator. {
"version": 5,
"last_dispatched_commit": "90cc37969e2eff619ad12a0707db11a3adcc7861",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "8ef41435fc56aff88fe90469c5747bf8175a970e",
"last_reviewed_commit": "90cc37969e2eff619ad12a0707db11a3adcc7861",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "8ef41435fc56aff88fe90469c5747bf8175a970e",
"last_recorded_worker_run_id": "29673723556",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "90cc37969e2eff619ad12a0707db11a3adcc7861",
"review_id": 4730105590
}
]
} |
There was a problem hiding this comment.
Holistic Review
Motivation: The problem is real and well-evidenced: .NET for Android moved its NDK API level from 21 to 24, but this NativeAOT target hardcodes android21/androideabi21, producing undefined symbol: __gnu_strerror_r / stderr link errors because the API 24 runtime references symbols absent from the API 21 libc stub. Making the level configurable is justified.
Approach: The fix is minimal and idiomatic MSBuild — it introduces a CrossCompileAndroidApiLevel property (settable by an outer build) and substitutes it into the android/androideabi ABI strings. This is the right, low-risk way to parameterize the value without changing behavior for consumers that don't set it.
Summary: 21), but the PR description says the change should "default to 24," which contradicts the hardcoded 21 default in the diff. A human should confirm whether the intended default is 21 (behavior-preserving, with .NET for Android supplying 24) or 24, and reconcile the description/comment accordingly. No correctness or build-breaking issues otherwise.
Detailed Findings
⚠️ Description/code mismatch on default value — see inline comment on line 35
The PR description states the change makes the level "configurable and default to 24," but the added property defaults to 21, preserving the prior hardcoded value. Both interpretations are defensible, but the description and code should agree. Details in the inline comment.
✅ Correctness & scope
The substitution is applied consistently to both the android$(CrossCompileAndroidApiLevel) and androideabi$(CrossCompileAndroidApiLevel) cases, which are the only two places the API level appeared. The property is guarded so an externally-set value wins, and no other NativeAOT target references a hardcoded Android API level (verified via grep). CrossCompileAbi feeds only into TargetTriple, so the change is contained to the linker target triple as intended.
💡 Comment clarity
The added comment ("API level specified here MUST be the same as the one used by .NET for Android") is slightly misleading since .NET for Android overrides the property; consider rewording to note that 21 is merely the fallback default when no consumer sets CrossCompileAndroidApiLevel.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 54.8 AIC · ⌖ 10.6 AIC · ⊞ 10K
| <PropertyGroup> | ||
| <!-- API level specified here MUST be the same as the one used by .NET for Android. .NET for Android targets will | ||
| take care to always set this property. --> | ||
| <CrossCompileAndroidApiLevel Condition="'$(CrossCompileAndroidApiLevel)' == ''">21</CrossCompileAndroidApiLevel> |
There was a problem hiding this comment.
21, but the PR description states the change "makes the API level configurable and default to 24." The code preserves the prior hardcoded 21 default. If keeping 21 is intentional (to avoid changing behavior for existing NativeAOT consumers that don't set the property, with .NET for Android supplying 24 via its targets), that's a reasonable choice — but please reconcile the PR description so it matches the code. If the intent really was to default to 24, this value should be updated. The inline comment above also says the value "MUST be the same as the one used by .NET for Android," which is confusing given that .NET for Android overrides it anyway; consider clarifying that 21 is only the fallback when no consumer sets the property.
Context: dotnet/android#9926
Context: dotnet/android@bef768d
.NET for Androidrecently switched CoreCLR and NativeAOT NDK API levels from 21 to 24, which uncovered an issue in the NativeAOTMicrosoft.NETCore.Native.Unix.targetsSetupOSSpecificPropstarget, where the target hardcodes the Android API level to21.This causes the following link error when trying to build an Android NativeAOT application:
The reason for this is that API 24
libc.socontains these two symbols, while the one from API 21 does not. Since.NET for Androidruntime is built with API 24, andNativeAOTbuild forces the API 21 level by passing the--target=${ARCH}-linux-android21argument to the linker, we get the above error.This PR makes the API level configurable and default to 24. The default value is merely the
current API level
.NET for Androidtargets, it doesn't need to be updated in the future - the.NET for Androidtargets will take care of setting the property to the appropriate value, should the default change.