[XABT] GeneratePackageManagerJava - Convert NativeCodeGenState to Cecil-less DTOs.
#10058
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jpobst
force-pushed
the
dev/jpobst/marshal-method-task-object
branch
from
54c9784 to
ed7e870
Compare
jpobst
force-pushed
the
dev/jpobst/marshal-method-task-object
branch
from
ed7e870 to
dfec04a
Compare
jpobst
requested review from
dellis1972,
grendello and
jonathanpeppers
as code owners
There was a problem hiding this comment.
Pull Request Overview
This pull request refactors the way marshal method data is stored by converting the Cecil-dependent NativeCodeGenState into Cecil‐less DTOs, improving task decoupling and assembly access.
- Updated various classes and method signatures in MarshalMethodsNativeAssemblyGenerator.cs to work with the new DTO types.
- Introduced MarshalMethodCecilAdapter.cs to convert Cecil objects into DTOs and updated related build task registrations in GeneratePackageManagerJava.cs and GenerateMainAndroidManifest.cs.
- Defined a new task constant in GenerateJavaStubs.cs to support the DTO-based state transfer.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Xamarin.Android.Build.Tasks/Utilities/MarshalMethodsNativeAssemblyGenerator.cs | Updated method signatures and type references to use DTO types instead of Cecil objects. |
| src/Xamarin.Android.Build.Tasks/Utilities/MarshalMethodCecilAdapter.cs | Added adapter methods to create DTOs; note issues with dictionary initializers. |
| src/Xamarin.Android.Build.Tasks/Tasks/GeneratePackageManagerJava.cs | Modified retrieval and usage of NativeCodeGenState to use the new DTO type. |
| src/Xamarin.Android.Build.Tasks/Tasks/GenerateMainAndroidManifest.cs | Transferred and disposed the Cecil-dependent state appropriately after conversion. |
| src/Xamarin.Android.Build.Tasks/Tasks/GenerateJavaStubs.cs | Added a new task key to facilitate registration of the DTO-based state. |
jonathanpeppers
approved these changes
Apr 23, 2025
Member
jonathanpeppers
left a comment
jonathanpeppers
left a comment
There was a problem hiding this comment.
This makes sense to me as the first step.
My comments aren't super important, especially if some of that code will be different in future PRs.
src/Xamarin.Android.Build.Tasks/Utilities/MarshalMethodCecilAdapter.cs
Outdated
Show resolved
Hide resolved
src/Xamarin.Android.Build.Tasks/Utilities/MarshalMethodCecilAdapter.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Jonathan Peppers <[email protected]>
jpobst
force-pushed
the
dev/jpobst/marshal-method-task-object
branch
from
e06feb7 to
dd1d12b
Compare
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Context: #10062
One issue with our desire to move around LLVM Marshal Method build code so that it can work with Ready2Run is that our current implementation relies on holding Cecil
AssemblyDefinitionobjects open in global state across multiple targets. While these objects are open, no other process can access our assemblies, making it harder to reorder targets and tasks.As a first step towards refactoring this process, let's move the data into a DTO that does not require Cecil. Additionally, this makes it clear what data is actually consumed by
<GeneratePackageManagerJava>rather than "entire assemblies" so we can focus on how to properly obtain and persist the data.This still relies on
BuildEngine4.RegisterTaskObjectto store the data, but a likely future step would be to persist it to a file instead.