[build] Stabilize Android define constant ordering - #12378
[build] Stabilize Android define constant ordering#12378jonathanpeppers wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses incremental build churn caused by non-deterministic ordering of Android define constants by appending Android defines at a consistent point in the compile pipeline (after .NET SDK implicit defines). This helps prevent equivalent project configurations from producing different compile dependency hashes and triggering unnecessary recompiles and downstream APK repackaging.
Changes:
- Move the
@(AndroidDefineConstants)append into a dedicated_AddAndroidDefineConstantstarget scheduled before compilation. - Add a regression test that exercises opposite target execution orders to ensure
DefineConstantsordering stays stable. - Expand design-time build coverage to validate Android defines with implicit framework defines enabled/disabled.
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/Xamarin.Android.Common.targets | Adds a pre-compile target to append Android define constants in a stable stage after SDK implicit defines. |
| src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/IncrementalBuildTest.cs | Adds regression coverage to ensure define constant ordering is independent of target execution order. |
| src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/BuildTest2.cs | Extends design-time build test matrix to include disabled implicit framework defines. |
|
@dalexsoto review |
dalexsoto
left a comment
There was a problem hiding this comment.
The Android define append now runs at a stable compiler stage after implicit defines, with regression coverage proving target-order independence and design-time behavior for both implicit-define modes.
8b80614 to
3e2475f
Compare
dalexsoto
left a comment
There was a problem hiding this comment.
Adding $(_AndroidJniAddNativeMethodRegistrationAttributePresentFile) to _GenerateJavaStubs outputs breaks incremental skipping when an input changes but the boolean content does not: CopyIfStringChanged preserves the output timestamp, leaving it older than the changed input, so the target reruns on every subsequent build. Refresh that state file timestamp whenever the target runs or use a separate always-refreshed output sentinel (Microsoft.Android.Sdk.TypeMap.LlvmIr.targets:38-41).
3e2475f to
7b856a1
Compare
dalexsoto
left a comment
There was a problem hiding this comment.
The replacement still loses the scanned JNI attribute state across incremental MSBuild processes: when _GenerateJavaStubs skips but _GeneratePackageManagerJava runs in a fresh process, NativeCodeGenState.TemplateJniAddNativeMethodRegistrationAttributePresent defaults to false, so a detected attribute is incorrectly removed from the generated application configuration. Persist the detected value across incremental builds while using a separate refreshed sentinel (GenerateNativeApplicationConfigSources.cs:275 and the _GenerateJavaStubs/package-manager path).
Append Android define constants at a fixed point after the .NET SDK implicit defines so equivalent project configurations share the same compile dependency hash. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 47bf5092-15e1-4b15-9e69-e6d60fa06b85
7b856a1 to
d88489c
Compare
dalexsoto
left a comment
There was a problem hiding this comment.
_SkipJniAddNativeMethodRegistrationAttributeScan still does not preserve the scanned JNI attribute state in production. It defaults to False and is only passed through to the tasks; no production target sets it when _GenerateJavaStubs skips. The new regression test instead forces the property to true before the first build, so it manufactures the desired result rather than exercising detection followed by a fresh-process incremental skip. In the real path, NativeCodeGenState.TemplateJniAddNativeMethodRegistrationAttributePresent still starts as false, and a genuinely detected attribute is removed from the generated application config. Persist the actual detected boolean across builds, use a separate refreshed output stamp, and test both detected true and false without forcing the skip property (Xamarin.Android.Common.targets:285, GenerateNativeApplicationConfigSources.cs:272-285, IncrementalBuildTest.cs:160-176).
Summary
Testing
PATH="/opt/homebrew/bin:$PATH" make all./dotnet-local.sh test bin/TestDebug/net10.0/Xamarin.Android.Build.Tests.dll --filter 'Name~AndroidDefineConstantsAreOrderIndependent|Name~DesignTimeBuildHasAndroidDefines'Fixes #12304