Skip to content

[XABT] Split up the <GeneratePackageManagerJava> task. #10070

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

Merged
merged 2 commits into from
Apr 24, 2025

Conversation

jpobst
Copy link
Contributor

@jpobst jpobst commented Apr 23, 2025

Today, the <GeneratePackageManagerJava> task performs three separate functions:

  • Generate MonoPackageManager_Resources.java
  • Generate native application config assemblies (ex: environment.arm64-v8a.ll)
  • Generate native marshal method assemblies (ex: marshal_methods.arm64-v8a.ll)

Break up the <GeneratePackageManagerJava> task into three separate tasks, each handling one of the above functions:

  • <GeneratePackageManagerJava>
  • <GenerateNativeApplicationConfigAssemblies>
  • <GenerateNativeMarshalMethodAssemblies>

This helps us know what each task is responsible for and what its inputs are, allowing us to move these tasks around independently in the future if needed.

@jpobst jpobst force-pushed the dev/jpobst/split-generatepackagemanagerjava branch from 9ee5ebe to c1b756b Compare April 23, 2025 21:28
@jpobst jpobst requested a review from Copilot April 23, 2025 21:30
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request splits the monolithic task into three focused tasks to improve maintainability and independence.

  • Updated the test to instantiate and execute separate tasks for package manager and application config assemblies.
  • Renamed method references and updated comments to reflect the new task structure.
  • Introduced a new task for generating native marshal method assemblies.

Reviewed Changes

Copilot reviewed 8 out of 9 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Tasks/GeneratePackageManagerJavaTests.cs Updated tests to validate execution of distinct tasks.
src/Xamarin.Android.Build.Tasks/Tasks/GenerateTypeMappings.cs Updated comment to refer to the new native application config assemblies task.
src/Xamarin.Android.Build.Tasks/Tasks/GenerateNativeMarshalMethodAssemblies.cs Added new task implementation for native marshal method assemblies.
src/Xamarin.Android.Build.Tasks/Tasks/GenerateMainAndroidManifest.cs Updated comment to reference the new native marshal method assemblies task.
src/Xamarin.Android.Build.Tasks/Tasks/GenerateJniRemappingNativeCode.cs Replaced reference to the old API with the updated native application config assemblies task method.
src/Xamarin.Android.Build.Tasks/Tasks/GenerateCompressedAssembliesNativeSourceFiles.cs Similar update as above for target architecture retrieval.
Files not reviewed (1)
  • src/Xamarin.Android.Build.Tasks/Xamarin.Android.Common.targets: Language not supported
Comments suppressed due to low confidence (2)

src/Xamarin.Android.Build.Tasks/Tasks/GenerateTypeMappings.cs:59

  • Update the comment to reference '' (plural) to match the new task naming.
        // Set for use by <GenerateNativeApplicationConfigAssembly> task later

src/Xamarin.Android.Build.Tasks/Tasks/GenerateMainAndroidManifest.cs:78

  • Update the task reference in the comment to '' (plural) to remain consistent with the new task naming convention.
        // If we still need the NativeCodeGenState in the <GenerateNativeMarshalMethodAssembly> task because we're using marshal methods,

@jpobst jpobst force-pushed the dev/jpobst/split-generatepackagemanagerjava branch from c1b756b to 87042a6 Compare April 23, 2025 21:33
@jpobst jpobst marked this pull request as ready for review April 23, 2025 23:10
@jpobst jpobst requested review from jonpryor and grendello April 23, 2025 23:11
Copy link
Contributor

@grendello grendello left a comment

Choose a reason for hiding this comment

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

The code changes are fine, but I think the new task names shouldn't end with Assemblies. The tasks generate native assembler source code, no managed assemblies are produced, so perhaps their names should end with SourceCode or Sources instead?

Copy link
Member

@jonathanpeppers jonathanpeppers left a comment

Choose a reason for hiding this comment

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

I went ahead and renamed *Assemblies -> *Sources.

Otherwise looks good. 👍

@jonathanpeppers jonathanpeppers enabled auto-merge (squash) April 24, 2025 13:39
@jonathanpeppers jonathanpeppers merged commit ee56aaa into main Apr 24, 2025
59 checks passed
@jonathanpeppers jonathanpeppers deleted the dev/jpobst/split-generatepackagemanagerjava branch April 24, 2025 18:05
@github-actions github-actions bot locked and limited conversation to collaborators May 25, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants