-
Notifications
You must be signed in to change notification settings - Fork 556
[XABT] GeneratePackageManagerJava - Convert NativeCodeGenState
to Cecil-less DTOs.
#10058
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
Conversation
54c9784
to
ed7e870
Compare
ed7e870
to
dfec04a
Compare
There was a problem hiding this 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 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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]>
e06feb7
to
dd1d12b
Compare
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
AssemblyDefinition
objects 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.RegisterTaskObject
to store the data, but a likely future step would be to persist it to a file instead.