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

[XABT] Refactor manifest merging and ACW map generation out of GenerateJavaStubs. #9827

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jpobst
Copy link
Contributor

@jpobst jpobst commented Feb 21, 2025

Begin breaking down the <GenerateJavaStubs> task into smaller, more manageable pieces by moving the manifest merging and ACW map generation into separate tasks. Once this is complete, we should be able to start moving these steps that require Cecil assembly scanning into linker steps (or equivalent), eventually saving a Cecil assembly scan per build.

Additionally, create a new GetProjectBuildSpecificTaskObjectKey to be used with GetRegisteredTaskObject. This version additional uses $(IntermediateOutpuPath) in the key to help ensure data from multi-targeted builds (net8.0-android,net9.0-android) is not overwritten.

@dotnet dotnet deleted a comment from azure-pipelines bot Feb 21, 2025
@jpobst jpobst marked this pull request as ready for review February 24, 2025 18:18
@jpobst jpobst requested a review from jonpryor February 24, 2025 18:18

namespace Xamarin.Android.Tasks;

public class GenerateMergedAndroidManifest : AndroidTask
Copy link
Contributor

Choose a reason for hiding this comment

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

Some of this can be trimmed down since we use the android manifestmerger these days.

AndroidManifestMerger` == `legacy is what this is keyed off.
But I guess this will be a staged approach.
Maybe we can rename this task a bit? Because unless we use legacy we use another tool to generate the final manifest. Perhaps g ?

Copy link
Contributor Author

@jpobst jpobst Feb 24, 2025

Choose a reason for hiding this comment

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

I don't see anything that prevents this logic from running if $(AndroidManifestMerger) != 'legacy'. If that is the intention, then that is definitely something we should look into in the future.

I can rename the task GenerateLegacyMergedAndroidManifest to help make the purpose clearer?

Copy link
Contributor

Choose a reason for hiding this comment

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

Well it is partially used in the new system. The main difference is the _MergedManifestDocuments ItemGroup is not populated in the new system. So that will be empty.

Copy link
Contributor

Choose a reason for hiding this comment

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

See

<_MergedManifestDocuments Condition=" '$(AndroidManifestMerger)' == 'legacy' " Include="@(ExtractedManifestDocuments)" />

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, then calling it GenerateLegacyMergedAndroidManifest is probably not a good name. Do you have a suggestion?

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe GenerateIntermediateAndroidManifest ?

Copy link
Contributor

Choose a reason for hiding this comment

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

or GenerateMainAndroidManifest since its the "root" manifest when we finally do the merging later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants