[Xamarin.Android.Build.Tasks] merge targetSdkVersion into existing <uses-sdk>

[jonathanpeppers]

Context

When a project's AndroidManifest.xml already declares a <uses-sdk /> element, ManifestDocument merges android:minSdkVersion into it if absent — but it never merged android:targetSdkVersion. Instead it computed the value purely for internal bookkeeping (targetSdkVersionValue, which drives <instrumentation>, permission, and screenOrientation codegen) and emitted a <!-- suppress UsesMinSdkAttributes --> lint comment:

string targetSdkVersion;
var tsv = uses.Attribute (androidNs + "targetSdkVersion");
if (tsv != null)
    targetSdkVersion = tsv.Value;
else {
    targetSdkVersion = TargetSdkVersionName;
    uses.AddBeforeSelf (new XComment ("suppress UsesMinSdkAttributes"));
}

aapt2 silently defaults targetSdkVersion to minSdkVersion when the attribute is missing. So any app with a partial hand-written <uses-sdk /> shipped with an unintentionally ancient targetSdk, with no warning anywhere in the build.

The motivating example

tests/CodeGen-Binding/Xamarin.Android.JcwGen-Tests/Properties/AndroidManifest.xml declared:

<uses-sdk android:minSdkVersion="24" />

$(TargetSdkVersion) was 37. Installing the resulting .apk on a physical Pixel 10 (API 36):

$ adb shell dumpsys package Xamarin.Android.JcwGen_Tests
# before
    minSdk=24 targetSdk=24
# after this change
    minSdk=24 targetSdk=37

Android 15+ shows the end user an "app built for an older version of Android" warning dialog on install when targetSdk is low enough — which is how this was noticed.

Why was it like this?

I looked for a deliberate "respect the user's <uses-sdk /> verbatim" policy and could not find one:

• git log -S 'suppress UsesMinSdkAttributes' -- .../ManifestDocument.cs returns exactly one commit: 59ec488b4 [Xamarin.Android.Build.Tasks] Import from monodroid/301e7238. The behavior predates this repo entirely.
• Every subsequent touch of this region (a66013b5c, 941e04c22, 3ab04df75) only re-pointed where the value comes from. None revisited the "don't write it" decision.
• UsesMinSdkAttributes is an Android lint check id ("Minimum SDK and target SDK attributes not defined"). The comment is a symptom of the omission, not an independent feature — and <!-- suppress ... --> is IntelliJ inspection syntax, whereas Android lint honours tools:ignore, so it was likely never effective for our Lint task anyway. The string appears exactly once in the whole repo: the line that writes it. Nothing reads it.
• Decisively, the immediately preceding branch already merges minSdkVersion into a user-authored element. The targetSdkVersion omission is an inconsistency, not a design.

The change

Write android:targetSdkVersion when the user did not specify one, and drop the now-dead suppression comment. User-specified values still win — both merges only fire when the attribute is null.

⚠️ Back-compat consideration for reviewers

This changes the produced manifest for any customer who hand-wrote a partial <uses-sdk /> (min only, or neither attribute). Those apps will now get an explicit targetSdkVersion equal to $(TargetSdkVersion) where aapt2 previously defaulted it to minSdkVersion.

That is the point of the fix — apps stop silently shipping with an ancient targetSdk — but it is a real behavioral change at runtime: platform behavior changes gated on targetSdk now apply to those apps. Customers who genuinely want a lower targetSdk can still set android:targetSdkVersion explicitly (or $(TargetSdkVersion)); those values continue to win.

Tests

New ManifestDocumentTest with fast in-process coverage of all four <uses-sdk /> shapes, plus an assertion that the suppression comment is gone:

case	input	expected minSdkVersion	expected targetSdkVersion
NoUsesSdkElement	element absent	computed	computed
MinSdkVersionOnly	android:minSdkVersion="24"	24	computed
TargetSdkVersionOnly	android:targetSdkVersion="30"	computed	30
MinAndTargetSdkVersion	both	24	30

Verified the tests actually catch the bug: with the ManifestDocument.cs change reverted, MinSdkVersionOnly fails with Expected: "37" But was: null. With the fix, all 4 pass, as does the existing ManifestTest.TargetSdkVersion_361In_36Out.

MockVersionResolver moved from a private nested class in ManifestTest to namespace scope so both fixtures can share it; the ManifestTest.cs diff is removal-only.

[Copilot]

Pull request overview

This PR fixes a long-standing manifest merge inconsistency in ManifestDocument: when a user-authored <uses-sdk /> element omits android:targetSdkVersion, the build now explicitly writes the computed target SDK instead of relying on aapt2’s silent defaulting behavior. This prevents apps from accidentally shipping with an unintended (often very low) targetSdkVersion when only minSdkVersion was specified.

Changes:

• Update ManifestDocument.Merge() to set android:targetSdkVersion on an existing <uses-sdk /> element when it’s missing, and remove the dead “suppress UsesMinSdkAttributes” comment emission.
• Add new in-process unit coverage (ManifestDocumentTest) validating all four <uses-sdk /> shapes and asserting the suppression comment is no longer produced.
• Refactor the MockVersionResolver helper so it can be shared (removing the nested copy from ManifestTest).

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
src/Xamarin.Android.Build.Tasks/Utilities/ManifestDocument.cs	Ensures missing android:targetSdkVersion is merged into an existing user <uses-sdk />, avoiding unintended aapt2 defaults.
src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/ManifestTest.cs	Removes the now-duplicated nested MockVersionResolver helper in favor of a shared one.
src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/ManifestDocumentTest.cs	Adds focused in-process tests covering <uses-sdk /> merge behavior for min/target combinations and verifies suppression comment removal.

Suppressed comments (1)

src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/ManifestDocumentTest.cs:31

• 💡 suggestion: Repo conventions prefer C# raw string literals (""") for multi-line strings. CreateManifest can use a raw string literal here to avoid the @-string and escaping, and it stays easier to edit.

		static string CreateManifest (string usesSdk) => $@"<?xml version=""1.0"" encoding=""utf-8""?>
<manifest xmlns:android=""http://schemas.android.com/apk/res/android"" package=""com.xamarin.usessdk"">
	{usesSdk}
	<application android:label=""UsesSdk"">
	</application>