-
Notifications
You must be signed in to change notification settings - Fork 538
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
[XABT] Split BuildArchive
from BuildApk
task.
#9556
Conversation
3905d61
to
32ae6a9
Compare
32ae6a9
to
7b9412e
Compare
BuildApkArchive
from BuildApk
task.BuildArchive
from BuildApk
task.
7b9412e
to
069e80e
Compare
069e80e
to
8dc5cc7
Compare
How does this impact |
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.
Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 suggestion.
Files not reviewed (1)
- src/Xamarin.Android.Build.Tasks/Xamarin.Android.Common.targets: Language not supported
It is largely unchanged. Updated the PR description to include FastDev timing. |
var ms = new MemoryStream (); | ||
entry.Extract (ms); | ||
Log.LogDebugMessage ($"Refreshing {entryName} from {ApkInputPath}"); | ||
apk.Archive.AddStream (ms, entryName, compressionMethod: entry.CompressionMethod); |
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.
Don't we need to set ms.Position=0
? Or is that somehow done automatically?
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.
It is copied from our existing code, so I assume it must be done automatically:
android/src/Xamarin.Android.Build.Tasks/Tasks/BuildApk.cs
Lines 198 to 201 in 2fa7954
var ms = new MemoryStream (); | |
entry.Extract (ms); | |
Log.LogDebugMessage ($"Refreshing {entryName} from {apkInputPath}"); | |
apk.Archive.AddStream (ms, entryName, compressionMethod: entry.CompressionMethod); |
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.
It's automatic when returned:
And then the MemoryStream
should reuse its underlying byte[]
when Length
is set to 0.
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.
I think the concern is that we pass the MemoryStream
to Archive.AddStream
(libzip.zip_file_add
) without rewinding it first. I assume the marshaling code that passes it to the native library as an IntPtr
is automatically passing a pointer to the beginning of the MemoryStream's underlying byte[]
and not pointing to the MemoryStream's current Position
.
* main: (25 commits) [CI] Break "Linux Tests" into 2 parallel jobs. (#9642) Fix `WorkloadDependencies.proj` build. (#9648) [CI] Set "WearOS Tests" parallelization to 2 agents. (#9639) [CI] Break "Package Tests" into 2 parallel jobs. (#9638) Bump to DevDiv/android-platform-support@3b4e16f1 (#9632) [NativeAOT] improve build logic, part 2 (#9631) Bump to dotnet/java-interop@2c06b3c2 (#9633) [NativeAOT] improve build logic, part 1 (#9614) [build] Generate `WorkloadDependencies.json` (#9613) [monodroid] remove `monodroid_get_log_categories()` (#9625) [monodroid] remove `_monodroid_get_identity_hash_code` (#9622) Bump to dotnet/java-interop@f800ea52 (#9607) [XABT] Break BuildApk into individual tasks for each content type. (#9612) [Mono.Android] Bind Android API-Baklava DP1 (#9594) [Xamarin.Android.Build.Tasks] Extract `BuildArchive` from `BuildApk` (#9556) [NativeAOT] MSBuild-related logic to get projects to build (#9583) [build] remove remnants of `OpenTK-1.0.dll` (#9610) [build] remove `Xamarin.Android.CSharp.targets` (#9609) [build] runtime "flavors" part 2 (#9598) Bump com.android.tools.build:manifest-merger to 31.7.3 (#9600) ...
* dev/grendel/use-libc++: (25 commits) [CI] Break "Linux Tests" into 2 parallel jobs. (#9642) Fix `WorkloadDependencies.proj` build. (#9648) [CI] Set "WearOS Tests" parallelization to 2 agents. (#9639) [CI] Break "Package Tests" into 2 parallel jobs. (#9638) Bump to DevDiv/android-platform-support@3b4e16f1 (#9632) [NativeAOT] improve build logic, part 2 (#9631) Bump to dotnet/java-interop@2c06b3c2 (#9633) [NativeAOT] improve build logic, part 1 (#9614) [build] Generate `WorkloadDependencies.json` (#9613) [monodroid] remove `monodroid_get_log_categories()` (#9625) [monodroid] remove `_monodroid_get_identity_hash_code` (#9622) Bump to dotnet/java-interop@f800ea52 (#9607) [XABT] Break BuildApk into individual tasks for each content type. (#9612) [Mono.Android] Bind Android API-Baklava DP1 (#9594) [Xamarin.Android.Build.Tasks] Extract `BuildArchive` from `BuildApk` (#9556) [NativeAOT] MSBuild-related logic to get projects to build (#9583) [build] remove remnants of `OpenTK-1.0.dll` (#9610) [build] remove `Xamarin.Android.CSharp.targets` (#9609) [build] runtime "flavors" part 2 (#9598) Bump com.android.tools.build:manifest-merger to 31.7.3 (#9600) ...
Break down the mega-task
BuildApk
into smaller separation of concerns:BuildApk
- Generate files that need to be placed in the APK.BuildArchive
- Place files in the APK.This works via the
@(FilesToAddToArchive)
item group which is defined as:The
%(Compression)
metadata is optional and is really only needed for handling the assembly compression information stored inRegisterTaskObject
(source). The valid values are from here. Hopefully this will not be needed in the final version.This change will allow future PR(s) to break down the
BuildApk
task into separate tasks for each type of files that belong in the APK, facilitating better incremental builds. For example, if only the Dalvik file changes we don't need to reprocess every managed assembly, etc.Implementation
To keep the diff more manageable at this point, create the temporary
ZipArchiveFileListBuilder
as a drop-in replacement forZipArchiveEx
. Instead of adding files to the.apk
it collects the file name and apk path so that theBuildArchive
task can do the actual adding.Additionally, there is code that looks in every referenced
.jar
file and copies non-code files into the final apk. This is currently handled by specifying theInclude
aspath/to/my.jar#ExtraFile.txt
. In a final future state we would probably unzip the.jar
s to disk and add the files from there. This would allow us to avoid unzipping the.jar
s on every build.Performance
Although performance isn't the focus of this PR, there is a nice win from eliminating extraneous
ZipArchiveEx.Flush ()
calls. (Note the AutoFlush logic still exists.)