[Xamarin.Android.Build.Tasks] Reduce BuildDependsOn duplication

[pjcollins]

I was looking to extend the BuildDependsOn target ordering used to
structure application and library builds and noticed a fair amount of
target duplication that may benefit from some consolidation.

A new _AndroidBuildDependsOn property has been added to include the
core targets that builds of both project types depend on.

The _CheckNonIdealBindingConfigurations target has been merged with
_CheckNonIdealConfigurations.

The _CheckProjectItems target will now run for both app and library
projects, as the corresponding CheckProjectItems task is already
conditional based on the value of $(AndroidApplication).

The _AddAndroidDefines target has been removed in favor of the more
widely used AndroidPrepareForBuild target, both of which are empty
targets that depend on $(_OnResolveMonoAndroidSdks).

The _RemoveLegacyDesigner target has been removed from the library
specific target list, it should already run for each project type as part
of the UpdateGeneratedFiles target.

[jonathanpeppers]

It looks like previously we had a different value for $(BuildDependsOn) for class libraries vs apps. Is this now sharing the same set of targets for both?

I think there were a few differences in there that were important.

[dellis1972]

There were differences I can't remember what though.

[pjcollins]

We do still have two groups for libraries vs apps. Those groups are now much smaller, as the bulk of the targets declared in both groups were duplicated. The new AndroidBuildDependsOn group contains the targets used by both project types.

The PR description has info about the project type specific targets that were modified, removed, or now run for both project types as part of these changes. The only potential behavior changes here would be with respect to _CheckProjectItems , and _RemoveLegacyDesigner as described in the PR. The rest of the changes are cosmetic.

The reason behind these changes is that I was looking to add a new target for all project types, and I wasn't sure where the best place to put it would be. It also seemed strange to add that target to two lists.

I'm happy to abandon this approach and go with something else if this seems unnecessary or too risky, but I think the consolidation makes the build easier to extend. That is assuming that adding to $(BuildDependsOn) is generally the recommended approach for extending the build process.

[pjcollins]

For reference, here's a diff of the two targets groups as they exist on main currently. This does not demonstrate the changes made in this PR, but it should highlight the few differences between the two:

--- App-BuildDependsOn.targets
+++ Lib-BuildDependsOn.targets
@@ -1,21 +1,22 @@
-<PropertyGroup Condition=" '$(AndroidApplication)' == 'True' ">
+<PropertyGroup Condition=" '$(AndroidApplication)' != 'True' ">
     <BuildDependsOn>
       _ValidateLinkMode;
-      _CheckNonIdealConfigurations;
+      _CheckNonIdealBindingConfigurations;
       _SetupMSBuildAllProjects;
       _SetupDesignTimeBuildForBuild;
       _CategorizeAndroidLibraries;
       _CreatePropertiesCache;
       _CleanIntermediateIfNeeded;
-      _CheckProjectItems;
+      _AddAndroidDefines;
       _CheckForContent;
       _CheckForObsoleteFrameworkAssemblies;
+      _CalculateAssetsWithAssetPackMetaData;
+      _RemoveLegacyDesigner;
       _ValidateAndroidPackageProperties;
       AddLibraryJarsToBind;
       _CollectJavaSourceForBinding;
       _CompileBindingJava;
       $(BuildDependsOn);
-      _CompileDex;
-      $(_AfterCompileDex);
+      _CreateAar;
     </BuildDependsOn>
 </PropertyGroup>

[jonathanpeppers]

We don't want _CreateAar running for apps, and we wouldn't want _CompileDex running for class libraries.

Adding $(AndroidBuildDependsOn) is a good idea, but I think we should try to keep the appropriate targets for both project types.

Maybe we move the condition to the targets?

[pjcollins]

_CreateAar and _CompileDex are still separated. The GitHub diff doesn't show this well... Looking at the file itself through here are the new splits:

Shared AndroidBuildDependsOn:

android/src/Xamarin.Android.Build.Tasks/Microsoft.Android.Sdk/targets/Microsoft.Android.Sdk.BuildOrder.targets

Lines 14 to 31 in a535c4f

	<AndroidBuildDependsOn>
	_ValidateLinkMode;
	_CheckNonIdealConfigurations;
	_SetupMSBuildAllProjects;
	_SetupDesignTimeBuildForBuild;
	_CategorizeAndroidLibraries;
	_CreatePropertiesCache;
	_CleanIntermediateIfNeeded;
	_CheckProjectItems;
	AndroidPrepareForBuild;
	_CheckForContent;
	_CheckForObsoleteFrameworkAssemblies;
	_ValidateAndroidPackageProperties;
	AddLibraryJarsToBind;
	_CollectJavaSourceForBinding;
	_CompileBindingJava;
	$(BuildDependsOn);
	</AndroidBuildDependsOn>

App BuildDependsOn:

android/src/Xamarin.Android.Build.Tasks/Microsoft.Android.Sdk/targets/Microsoft.Android.Sdk.BuildOrder.targets

Lines 34 to 39 in a535c4f

	<PropertyGroup Condition=" '$(AndroidApplication)' == 'True' ">
	<BuildDependsOn>
	$(AndroidBuildDependsOn);
	_CompileDex;
	$(_AfterCompileDex);
	</BuildDependsOn>

Library BuildDependsOn:

android/src/Xamarin.Android.Build.Tasks/Microsoft.Android.Sdk/targets/Microsoft.Android.Sdk.BuildOrder.targets

Lines 90 to 94 in a535c4f

	<PropertyGroup Condition=" '$(AndroidApplication)' != 'True' ">
	<BuildDependsOn>
	$(AndroidBuildDependsOn);
	_CalculateAssetsWithAssetPackMetaData;
	_CreateAar;

[jonathanpeppers]

Ok, I think the changes look right after another review. It doesn't look like any targets have changed (run for different projects), just refactoring.

Should we add the new public properties to this doc?

• https://github.com/dotnet/android/blob/main/Documentation/docs-mobile/building-apps/build-properties.md

[pjcollins]

Ok, I think the changes look right after another review. It doesn't look like any targets have changed (run for different projects), just refactoring.

Should we add the new public properties to this doc?

• https://github.com/dotnet/android/blob/main/Documentation/docs-mobile/building-apps/build-properties.md

Sorry I'm not sure what we'd want to document here, the new AndroidBuildDependsOn target list is internal to our build process, and I don't think anything public is changing that needs documentation

[jonathanpeppers]

Oh, should we prefix $(_AndroidBuildDependsOn) with an underscore, if customers aren't meant to use it?

[pjcollins]

Oh, should we prefix $(_AndroidBuildDependsOn) with an underscore, if customers aren't meant to use it?

I'm not entirely sure what the right conventions are, I was trying to follow suit with $(BuildDependsOn). I guess customers could try to use $(AndroidBuildDependsOn) in a similar way if they wanted to extend the build? Maybe I should add a comment about it to https://github.com/dotnet/android/blob/main/Documentation/guides/MSBuildBestPractices.md#should-i-use-beforetargets-or-aftertargets? It's not a traditional "project config" property like the others in the build-properties.md.

[jonathanpeppers]

$(BuildDependsOn) came from MSBuild, and they documented it as a public thing:

• https://learn.microsoft.com/en-us/visualstudio/msbuild/how-to-extend-the-visual-studio-build-process?view=vs-2022#extend-the-dependson-properties

We could do that for ours, or just put _ as a signal that it's internal.

[pjcollins]

$(BuildDependsOn) came from MSBuild, and they documented it as a public thing:

• https://learn.microsoft.com/en-us/visualstudio/msbuild/how-to-extend-the-visual-studio-build-process?view=vs-2022#extend-the-dependson-properties

We could do that for ours, or just put _ as a signal that it's internal.

After some more testing it looks like both $(BuildDependsOn) and $(_AndroidBuildDependsOn) can't easily be overridden in a customers project file due to our import ordering, so I've added the _ prefix to our new property.