Skip to content
21 changes: 20 additions & 1 deletion src/Xamarin.Android.Build.Tasks/Tasks/BuildArchive.cs
Original file line number Diff line number Diff line change
Expand Up @@ -158,16 +158,35 @@ public override bool RunTask ()
// ItemSpec for these will be "<jarfile>#<entrypath>
// eg: "obj/myjar.jar#myfile.txt"
var jar_file_path = disk_path.Substring (0, disk_path.Length - (jar_entry_name.Length + 1));
var wasExistingOutputEntry = existingEntries.Remove (apk_path);
var hasApkEntry = apk.ContainsEntry (apk_path);

if (apk.ContainsEntry (apk_path)) {
if (hasApkEntry && !wasExistingOutputEntry) {
Log.LogDebugMessage ("Failed to add jar entry {0} from {1}: the same file already exists in the apk", jar_entry_name, Path.GetFileName (jar_file_path));
continue;
}

using (var stream = File.OpenRead (jar_file_path))
using (var jar = ZipArchive.Open (stream)) {
if (!jar.ContainsEntry (jar_entry_name)) {
Log.LogDebugMessage ("Failed to add jar entry {0} from {1}: entry not found in jar.", jar_entry_name, jar_file_path);
if (wasExistingOutputEntry)
existingEntries.Add (apk_path);
continue;
Comment thread
simonrozsival marked this conversation as resolved.
}

var jar_item = jar.ReadEntry (jar_entry_name);

if (hasApkEntry) {
// CRC is computed on uncompressed data — matching CRC means identical content regardless of compression settings.
if (apk.GetEntry (apk_path).CRC == jar_item.CRC) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🤖 💡 Documentation — The existing input-APK refresh check above (line 120) compares both CRC and CompressedSize, while this new path compares CRC only. CRC-only is actually correct here — CRC checksums the uncompressed data, so matching CRCs mean identical content regardless of compression settings. A brief inline comment explaining the intentional difference would help future readers who might wonder about the inconsistency.

Suggested change
if (apk.GetEntry (apk_path).CRC == jar_item.CRC) {
if (apk.GetEntry (apk_path).CRC == jar_item.CRC) { // CRC is computed on uncompressed data — matching CRC means identical content

Rule: Comments explain "why", not "what"

Log.LogDebugMessage ("Skipping {0} from {1} as it is up to date.", jar_entry_name, jar_file_path);
Comment thread
simonrozsival marked this conversation as resolved.
continue;
}

apk.DeleteEntry (apk_path);
}

byte [] data;
var d = MemoryStreamPool.Shared.Rent ();

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,168 @@
#nullable enable
using System;
using System.Collections.Generic;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🤖 💡 Formatting — New files should have #nullable enable at the top and use file-scoped namespaces (namespace Xamarin.Android.Build.Tests;). Several other test fixtures in this directory follow those conventions.

Rule: Nullable reference types / File-scoped namespaces

using System.IO;
using System.Text;
using Microsoft.Android.Build.Tasks;
using Microsoft.Build.Framework;
using Microsoft.Build.Utilities;
using NUnit.Framework;
using Xamarin.Android.Tasks;
using Xamarin.Tools.Zip;

namespace Xamarin.Android.Build.Tests;

[TestFixture]
public class BuildArchiveTests
{
string? tempDirectory;

string TempDirectory => tempDirectory ?? throw new InvalidOperationException ("Setup has not run.");

[SetUp]
public void Setup ()
{
tempDirectory = Path.Combine (Path.GetTempPath (), Path.GetRandomFileName ());
Directory.CreateDirectory (tempDirectory);
}

[TearDown]
public void TearDown ()
{
if (!tempDirectory.IsNullOrEmpty () && Directory.Exists (tempDirectory))
Directory.Delete (tempDirectory, recursive: true);
}

[Test]
public void ExistingJavaArchiveEntriesAreUpdated ()
{
var apk = Path.Combine (TempDirectory, "app.apk");
var jar = Path.Combine (TempDirectory, "classes.jar");

CreateArchive (apk, ("commonMain/default/manifest", "existing"), ("stale.txt", "stale"));
CreateArchive (jar, ("commonMain/default/manifest", "current"));

var item = new TaskItem ($"{jar}#commonMain/default/manifest");
item.SetMetadata ("ArchivePath", "commonMain/default/manifest");
item.SetMetadata ("JavaArchiveEntry", "commonMain/default/manifest");

var task = new BuildArchive {
BuildEngine = new MockBuildEngine (TestContext.Out),
ApkOutputPath = apk,
FilesToAddToArchive = [item],
};

Assert.IsTrue (task.RunTask (), "task should have succeeded");

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🤖 💡 Formatting — This repo prefers [] collection expressions over new T[] { ... } (C# 12 syntax). Several places in this file can be simplified:

Suggested change
FilesToAddToArchive = [item],

Same applies to lines 79, 103, 116, 135, and 145.

Rule: Use [] not Array.Empty<T>() / new T[]

using (var archive = ZipArchive.Open (apk, FileMode.Open)) {
archive.AssertEntryContents (apk, "commonMain/default/manifest", "current");
archive.AssertDoesNotContainEntry (apk, "stale.txt");
}
}

[Test]
public void ExistingJavaArchiveEntriesAreSkippedWhenUpToDate ()
{
var apk = Path.Combine (TempDirectory, "app.apk");
var jar = Path.Combine (TempDirectory, "classes.jar");

CreateArchive (apk, ("commonMain/default/manifest", "current"));
CreateArchive (jar, ("commonMain/default/manifest", "current"));

var item = new TaskItem ($"{jar}#commonMain/default/manifest");
item.SetMetadata ("ArchivePath", "commonMain/default/manifest");
item.SetMetadata ("JavaArchiveEntry", "commonMain/default/manifest");
var messages = new List<BuildMessageEventArgs> ();

var task = new BuildArchive {
BuildEngine = new MockBuildEngine (TestContext.Out, messages: messages),
ApkOutputPath = apk,
FilesToAddToArchive = [item],
};

Assert.IsTrue (task.RunTask (), "task should have succeeded");

Assert.That (messages, Has.Some.Property (nameof (BuildMessageEventArgs.Message)).EqualTo ($"Skipping commonMain/default/manifest from {jar} as it is up to date."));

using (var archive = ZipArchive.Open (apk, FileMode.Open)) {
archive.AssertEntryContents (apk, "commonMain/default/manifest", "current");
}
}

[Test]
public void DuplicateJavaArchiveEntriesKeepFirstCurrentBuildItem ()
{
var apk = Path.Combine (TempDirectory, "app.apk");
var firstJar = Path.Combine (TempDirectory, "first.jar");
var secondJar = Path.Combine (TempDirectory, "second.jar");

CreateArchive (apk, ("stale.txt", "stale"));
CreateArchive (firstJar, ("commonMain/default/manifest", "first"));
CreateArchive (secondJar, ("commonMain/default/manifest", "second"));

Comment thread
simonrozsival marked this conversation as resolved.
var firstItem = new TaskItem ($"{firstJar}#commonMain/default/manifest");
firstItem.SetMetadata ("ArchivePath", "commonMain/default/manifest");
firstItem.SetMetadata ("JavaArchiveEntry", "commonMain/default/manifest");
var secondItem = new TaskItem ($"{secondJar}#commonMain/default/manifest");
secondItem.SetMetadata ("ArchivePath", "commonMain/default/manifest");
secondItem.SetMetadata ("JavaArchiveEntry", "commonMain/default/manifest");
var messages = new List<BuildMessageEventArgs> ();

var task = new BuildArchive {
BuildEngine = new MockBuildEngine (TestContext.Out, messages: messages),
ApkOutputPath = apk,
FilesToAddToArchive = [firstItem, secondItem],
};

Assert.IsTrue (task.RunTask (), "task should have succeeded");

Assert.That (messages, Has.Some.Property (nameof (BuildMessageEventArgs.Message)).EqualTo ("Failed to add jar entry commonMain/default/manifest from second.jar: the same file already exists in the apk"));

using (var archive = ZipArchive.Open (apk, FileMode.Open)) {
archive.AssertEntryContents (apk, "commonMain/default/manifest", "first");
archive.AssertDoesNotContainEntry (apk, "stale.txt");
}
}
Comment thread
simonrozsival marked this conversation as resolved.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🤖 💡 Testing — Consider adding a test for the case where multiple jar items target the same apk_path — this exercises the preserved duplicate-handling logic (!wasExistingOutputEntry branch). The current tests only cover the existing-output-entry path.

Rule: Test edge cases

[Test]
public void MissingJarEntryIsSkippedAndExistingOutputEntryIsRemoved ()
{
var apk = Path.Combine (TempDirectory, "app.apk");
var jar = Path.Combine (TempDirectory, "classes.jar");

CreateArchive (apk, ("commonMain/default/manifest", "existing"));
CreateArchive (jar, ("other-entry.txt", "contents"));

var item = new TaskItem ($"{jar}#commonMain/default/manifest");
item.SetMetadata ("ArchivePath", "commonMain/default/manifest");
item.SetMetadata ("JavaArchiveEntry", "commonMain/default/manifest");
var messages = new List<BuildMessageEventArgs> ();

var task = new BuildArchive {
BuildEngine = new MockBuildEngine (TestContext.Out, messages: messages),
ApkOutputPath = apk,
FilesToAddToArchive = [item],
};

Assert.IsTrue (task.RunTask (), "task should have succeeded");

Assert.That (messages, Has.Some.Property (nameof (BuildMessageEventArgs.Message)).EqualTo ($"Failed to add jar entry commonMain/default/manifest from {jar}: entry not found in jar."));

// The entry should be removed. If the APK itself no longer exists, all entries were cleared (also satisfies the assertion).
if (File.Exists (apk)) {
using (var archive = ZipArchive.Open (apk, FileMode.Open)) {
archive.AssertDoesNotContainEntry (apk, "commonMain/default/manifest");
}
}
}

static void CreateArchive (string path, params (string name, string contents) [] entries)
{
using (var stream = File.Create (path))
using (var archive = ZipArchive.Create (stream)) {
foreach (var entry in entries) {
archive.AddEntry (entry.name, entry.contents, encoding: Encoding.UTF8);
}
}
}
}
Loading