Skip to content

Add resources at end of APK #4659

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.

Sign up for GitHub

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

Draft
wants to merge 13 commits into
base: main
Choose a base branch
from
Draft

Add resources at end of APK #4659

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
0cdd106
Write resources at the end of the APK, to optimize delta install
May 6, 2020
f53a5c7
Fixup resource update
May 7, 2020
5fc6087
Fix to get the file timestamps before output is modified
May 7, 2020
968f4eb
Add braces, per code style conventions
May 11, 2020
abd6e73
Add test to verify updated resources at end of zip
May 12, 2020
bbadcf2
Added resource delete test
May 12, 2020
6ddd5e3
Removed TODO comments; it's expected
May 12, 2020
47e2583
Make use of the LibZipSharp fix to skip deleted entries
May 15, 2020
37adaa0
Make use of the LibZipSharp fix to skip deleted entries
May 15, 2020
86d468a
Merge branch 'add-resources-at-end-of-apk' of github.com:xamarin/xama…
May 15, 2020
24cc3a0
Bump to fixed LibZipSharp 0.0.13
May 20, 2020
63e83c6
Move updating from input Apk to separate method
May 20, 2020
5946d0c
Change back to not bump LibZipSharp (yet) and use workaround code ins…
May 20, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
102 changes: 66 additions & 36 deletions src/Xamarin.Android.Build.Tasks/Tasks/BuildApk.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,46 +122,23 @@ void ExecuteWithAbi (string [] supportedAbis, string apkInputPath, string apkOut
}

ArchiveFileList files = new ArchiveFileList ();
bool refresh = true;
if (apkInputPath != null && File.Exists (apkInputPath) && !File.Exists (apkOutputPath)) {
Log.LogDebugMessage ($"Copying {apkInputPath} to {apkInputPath}");
File.Copy (apkInputPath, apkOutputPath, overwrite: true);
refresh = false;
}

using (var notice = Assembly.GetExecutingAssembly ().GetManifestResourceStream ("NOTICE.txt"))
using (var apk = new ZipArchiveEx (apkOutputPath, File.Exists (apkOutputPath) ? FileMode.Open : FileMode.Create )) {
if (refresh) {
for (long i = 0; i < apk.Archive.EntryCount; i++) {
ZipEntry e = apk.Archive.ReadEntry ((ulong) i);
Log.LogDebugMessage ($"Registering item {e.FullName}");
existingEntries.Add (e.FullName);
}
bool apkInputPathExists = apkInputPath != null && File.Exists (apkInputPath);

DateTime lastWriteOutput = DateTime.MinValue;
DateTime lastWriteInput = DateTime.MinValue;
if (apkInputPathExists) {
lastWriteOutput = File.Exists (apkOutputPath) ? File.GetLastWriteTimeUtc (apkOutputPath) : DateTime.MinValue;
lastWriteInput = File.GetLastWriteTimeUtc (apkInputPath);
}
if (apkInputPath != null && File.Exists (apkInputPath) && refresh) {
var lastWriteOutput = File.Exists (apkOutputPath) ? File.GetLastWriteTimeUtc (apkOutputPath) : DateTime.MinValue;
var lastWriteInput = File.GetLastWriteTimeUtc (apkInputPath);
using (var packaged = new ZipArchiveEx (apkInputPath, FileMode.Open)) {
foreach (var entry in packaged.Archive) {
Log.LogDebugMessage ($"Deregistering item {entry.FullName}");
existingEntries.Remove (entry.FullName);
if (lastWriteInput <= lastWriteOutput)
continue;
if (apk.Archive.ContainsEntry (entry.FullName)) {
ZipEntry e = apk.Archive.ReadEntry (entry.FullName);
// check the CRC values as the ModifiedDate is always 01/01/1980 in the aapt generated file.
if (entry.CRC == e.CRC) {
Log.LogDebugMessage ($"Skipping {entry.FullName} from {apkInputPath} as its up to date.");
continue;
}
}
var ms = new MemoryStream ();
entry.Extract (ms);
Log.LogDebugMessage ($"Refreshing {entry.FullName} from {apkInputPath}");
apk.Archive.AddStream (ms, entry.FullName, compressionMethod: entry.CompressionMethod);
}
}

for (long i = 0; i < apk.Archive.EntryCount; i++) {
ZipEntry e = apk.Archive.ReadEntry ((ulong) i);
Log.LogDebugMessage ($"Registering item {e.FullName}");
existingEntries.Add (e.FullName);
}
apk.FixupWindowsPathSeparators ((a, b) => Log.LogDebugMessage ($"Fixing up malformed entry `{a}` -> `{b}`"));
string noticeName = RootPath + "NOTICE";
existingEntries.Remove (noticeName);
if (!apk.Archive.ContainsEntry (noticeName))
Expand Down Expand Up @@ -248,6 +225,16 @@ void ExecuteWithAbi (string [] supportedAbis, string apkInputPath, string apkOut
count = 0;
}
}

// For now, we keep track of deleted entry indices & skip them ourselves when iterating zip contents in FixupWindowsPathSeparators.
// For this to work, no one shoud call Flush on the zip before calling FixupWindowsPathSeparators, since Flush changes all the indices
// (and gets rid of the deleted stuff). Later when we move to a newer LibZipSharp, with the fix to skip deleted entries itself,
// we can remove this workaround. See FixupWindowsPathSeparators for more info.
HashSet<ulong> deletedEntries = new HashSet<ulong> ();
Copy link
Contributor

@jonpryor jonpryor May 13, 2020

Choose a reason for hiding this comment

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

Why is this a ulong instead of a long, when .EntryCount is long?

Copy link
Author

@BretJohnson BretJohnson May 13, 2020

Choose a reason for hiding this comment

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

@grendello - do you know if there's a rationale for that? maybe EntryCount can be -1 in some case?

Copy link
Contributor

@grendello grendello May 14, 2020

Choose a reason for hiding this comment

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

As far as I remember, the rationale for EntryCount being long was some sort of compatibility with BCL's ZipArchive (which doesn't have EntryCount but has a collection which has a signed integer Count property). At the same time, I always avoid using signed integers whenever a size or an index is concerned - on the notion that we have no negative indices and we have no negative sizes (or at least - they make no sense). Thus using an unsigned integer limits the size of the error domain (index can be too big, but can't be too small - one less thing that can go wrong)

if (apkInputPathExists)
UpdateEntriesFromInputApk (apkInputPath, apk, lastWriteOutput, lastWriteInput, deletedEntries);
apk.FixupWindowsPathSeparators (deletedEntries, (a, b) => Log.LogDebugMessage ($"Fixing up malformed entry `{a}` -> `{b}`"));

// Clean up Removed files.
foreach (var entry in existingEntries) {
Log.LogDebugMessage ($"Removing {entry} as it is not longer required.");
Expand All @@ -258,6 +245,49 @@ void ExecuteWithAbi (string [] supportedAbis, string apkInputPath, string apkOut
}
}

private void UpdateEntriesFromInputApk (string apkInputPath, ZipArchiveEx apk, DateTime lastWriteOutput, DateTime lastWriteInput, HashSet<ulong> deletedEntries)
{
using (var packaged = new ZipArchiveEx (apkInputPath, FileMode.Open)) {
foreach (var entry in packaged.Archive) {
Log.LogDebugMessage ($"Deregistering item {entry.FullName}");
existingEntries.Remove (entry.FullName);
if (lastWriteInput <= lastWriteOutput)
continue;

long entryIndexInOutput;
if (apk.Archive.ContainsEntry (entry.FullName, out entryIndexInOutput)) {
ZipEntry e = apk.Archive.ReadEntry (entry.FullName);
// check the CRC values as the ModifiedDate is always 01/01/1980 in the aapt generated file.
if (entry.CRC == e.CRC) {
Copy link
Contributor

@jonpryor jonpryor May 28, 2020

Choose a reason for hiding this comment

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

@grendello: what's the "likelihood" of CRC collisions? This is "only" a 32-bit value. Should we be relying on it for change "identicality" checks?

Copy link
Contributor

@grendello grendello May 28, 2020

Choose a reason for hiding this comment

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

For ZIP purposes it's not likely that we'd have collisions. Adler-32 (used to calculate the CRC) is weak for short messages, however, so there is a possibility of collision on small files. How big a likelihood? Depends on the data set... I wouldn't use it alone to see if the data has changed or not, but combined with the file name check I think it's good enough.

Log.LogDebugMessage ($"Skipping {entry.FullName} from {apkInputPath} as its up to date.");
continue;
}
} else {
entryIndexInOutput = -1;
}

var ms = new MemoryStream ();
entry.Extract (ms);
Copy link
Contributor

@jonpryor jonpryor May 28, 2020

Choose a reason for hiding this comment

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

Is this advisable or a good idea?

@grendello: is there a "good" libZipSharp method which can copy data between .zip files without decompressing them?

My fear is that entry may refer to e.g. a 2GB resource (.mp4 file?), at which point this entry.Extract(ms) invocation is going to need to contain said 2GB of data, which very probably isn't going to happen, which will in turn cause "obscure" corner cases in .zip handling.

Maybe the "proper" thing to do is check entry.Size and use an intermediate file if it's "too big", though that has implications with Windows Defender not liking file writes.

Copy link
Contributor

@grendello grendello May 28, 2020

Choose a reason for hiding this comment

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

e.g. zip_fopen_index can be used to open compressed data and copy it around as needed (you need to pass OperationFlags.Compressed to do that, here are the upstream docs)

Copy link
Contributor

@jonpryor jonpryor May 28, 2020

Choose a reason for hiding this comment

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

which is only publicly exposed via ZipEntry.Extract(), so there's no public way to do this at present.

Copy link
Contributor

@grendello grendello May 28, 2020

Choose a reason for hiding this comment

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

It was never needed, so there was no reason to make it public (or, rather, wrapped in a method to call it)


if (entryIndexInOutput != -1) {
Log.LogDebugMessage ($"Refreshing {entry.FullName} from {apkInputPath}");

// Force the modified resource to move to the end of the file, by deleting it first so that AddStream adds it
// back with a new index. Keeping modified resources toward the end optimizes the delta install for the typical
// dev scenario where the user is editing a few resources but most of the APK contents (e.g. the native libs)
// don't change and we want to keep their byte offset in the APK fixed. Delta install need not update APK
// contents that don't change and don't move.
apk.Archive.DeleteEntry ((ulong) entryIndexInOutput);
deletedEntries.Add ((ulong) entryIndexInOutput);
} else {
Log.LogDebugMessage ($"Adding {entry.FullName} from {apkInputPath}");
}

apk.Archive.AddStream (ms, entry.FullName, compressionMethod: entry.CompressionMethod);
}
}
}

public override bool RunTask ()
{
Aot.TryGetSequencePointsMode (AndroidSequencePointsMode, out sequencePointsMode);
Expand Down
107 changes: 102 additions & 5 deletions src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/IncrementalBuildTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
using System.Text;
using Xamarin.Android.Tasks;
using Xamarin.ProjectTools;
using Xamarin.Tools.Zip;

namespace Xamarin.Android.Build.Tests
{
Expand Down Expand Up @@ -1088,23 +1089,119 @@ public void AndroidResourceChange ()
using (var builder = CreateApkBuilder ()) {
Assert.IsTrue (builder.Build (proj), "first build should succeed");

// AndroidResource change
proj.LayoutMain += $"{Environment.NewLine}<!--comment-->";
// Change the AndroidResource body; just adding a comment isn't enough to trigger an APK update
string orientationTag = "android:orientation=\"vertical\"";
proj.LayoutMain = proj.LayoutMain.Replace (orientationTag, $"{orientationTag} android:paddingLeft=\"16dp\"");
proj.Touch ("Resources\\layout\\Main.axml");
Assert.IsTrue (builder.Build (proj), "second build should succeed");

var targets = new [] {
AssertNonResourceTargetsSkipped (builder);
}
}

void AssertNonResourceTargetsSkipped(ProjectBuilder builder)
{
var targets = new [] {
"_ResolveLibraryProjectImports",
"_GenerateJavaStubs",
"_CompileJava",
"_CompileToDalvik",
};
foreach (var target in targets) {
Assert.IsTrue (builder.Output.IsTargetSkipped (target), $"`{target}` should be skipped!");
foreach (var target in targets) {
Assert.IsTrue (builder.Output.IsTargetSkipped (target), $"`{target}` should be skipped!");
}
}

[Test]
public void AndroidChangedResourcesAtEnd ()
{
var path = Path.Combine (Root, "temp", TestName);

var proj = new XamarinAndroidApplicationProject ();
using (var builder = CreateApkBuilder ()) {
Assert.IsTrue (builder.Build (proj), "first build should succeed");

string signedApkPath = Path.Combine (path, proj.OutputPath, "UnnamedProject.UnnamedProject-Signed.apk");

// Initial resources should at the end of the zip
using (var zip = ZipArchive.Open (signedApkPath, FileMode.Open)) {
Assert.AreEqual (37, zip.EntryCount, "Zip entry count");
AssertZipEntryHasPosition (zip, "res/layout/main.xml", 32);
AssertZipEntryHasPosition (zip, "resources.arsc", 33);
}

AndroidItem.AndroidResource layout2Resource = AddLayout2 (proj);
Assert.IsTrue (builder.Build (proj), "second build should succeed");

// Added resources should go at the end of the zip
using (var zip = ZipArchive.Open (signedApkPath, FileMode.Open)) {
Assert.AreEqual (38, zip.EntryCount, "Zip entry count");
AssertZipEntryHasPosition (zip, "res/layout/main.xml", 32);
AssertZipEntryHasPosition (zip, "res/layout/layout2.xml", 33);
AssertZipEntryHasPosition (zip, "resources.arsc", 34);
}

string orientationTag = "android:orientation=\"vertical\"";
proj.LayoutMain = proj.LayoutMain.Replace (orientationTag, $"{orientationTag} android:paddingLeft=\"16dp\"");
proj.Touch ("Resources\\layout\\Main.axml");
Assert.IsTrue (builder.Build (proj), "third build should succeed");
AssertNonResourceTargetsSkipped (builder);

// Updated resources should go at the end of the zip
using (var zip = ZipArchive.Open (signedApkPath, FileMode.Open)) {

Assert.AreEqual (38, zip.EntryCount, "Zip entry count");
AssertZipEntryHasPosition (zip, "res/layout/layout2.xml", 32);
AssertZipEntryHasPosition (zip, "resources.arsc", 33);
AssertZipEntryHasPosition (zip, "res/layout/main.xml", 34);
}

proj.AndroidResources.Remove (layout2Resource);
Assert.IsTrue (builder.Build (proj), "fourth build should succeed");

// Removed resources should go away
using (var zip = ZipArchive.Open (signedApkPath, FileMode.Open)) {
Assert.AreEqual (37, zip.EntryCount, "Zip entry count");
AssertZipEntryHasPosition (zip, "res/layout/main.xml", 32);
AssertZipEntryHasPosition (zip, "resources.arsc", 33);
}
}
}

AndroidItem.AndroidResource AddLayout2 (XamarinAndroidApplicationProject proj)
{
const string layout2 = @"<?xml version=""1.0"" encoding=""utf-8""?>
<LinearLayout xmlns:android=""http://schemas.android.com/apk/res/android""
android:orientation=""vertical""
android:layout_width=""fill_parent""
android:layout_height=""fill_parent""
>
<Button
android:id=""@+id/myButton2""
android:layout_width=""fill_parent""
android:layout_height=""wrap_content""
android:text=""@string/hello""
/>
</LinearLayout>
";

var androidResource = new AndroidItem.AndroidResource ("Resources\\layout\\Layout2.axml") { TextContent = () => layout2 };
proj.AndroidResources.Add (androidResource);

return androidResource;
}

void AssertZipEntryHasPosition (ZipArchive zipArchive, string name, int expectedPosition) {
foreach (ZipEntry entry in zipArchive) {
if (entry.FullName.Equals (name, StringComparison.Ordinal)) {
Assert.AreEqual (expectedPosition, entry.Index, $"Zip entry {name} should be at expected position");
return;
}
}

Assert.Fail ($"Zip entry ${name} not found");
}

[Test]
[NonParallelizable]
public void AndroidXMigrationBug ()
Expand Down
22 changes: 19 additions & 3 deletions src/Xamarin.Android.Build.Tasks/Utilities/ZipArchiveEx.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using System;
using System;
using System.Collections.Generic;
using System.IO;
using System.Linq;
using Xamarin.Tools.Zip;
Expand Down Expand Up @@ -113,10 +114,25 @@ public void AddDirectory (string folder, string folderInArchive, CompressionMeth
/// <summary>
/// HACK: aapt2 is creating zip entries on Windows such as `assets\subfolder/asset2.txt`
/// </summary>
public void FixupWindowsPathSeparators (Action<string, string> onRename)
public void FixupWindowsPathSeparators (HashSet<ulong> deletedEntries, Action<string, string> onRename)
{
bool modified = false;
foreach (var entry in zip) {

long entryCount = zip.EntryCount;
if (entryCount < 0)
return;

for (ulong i = 0; i < (ulong) entryCount; ++i) {
// If an entry index has been deleted then ReadEntry will throw a "Entry has been deleted" ZipException.
// So we skip deleted entries instead. Later, when we move to a newer LibZipSharp (>= 1.0.13) with this fix
// https://github.com/xamarin/LibZipSharp/pull/53 we can remove this workaround and go back to using
// a foreach enumerator to enumerate the contents of the zip. With the fix, deleted entries are
// skipped on the LibZipSharp side when enumerating. But we want to keep the LibZipSharp bump as
// a separate change, with the other changes that come with it, and not depenend on that here.
if (deletedEntries.Contains (i))
continue;

var entry = zip.ReadEntry (i);
if (entry.FullName.Contains ('\\')) {
var name = entry.FullName.Replace ('\\', '/');
onRename?.Invoke (entry.FullName, name);
Expand Down