Fix BuildArchive updates for existing jar entries#11552
Conversation
Co-authored-by: simonrozsival <374616+simonrozsival@users.noreply.github.com>
|
@copilot The current fix prevents deletion of existing jar-sourced entries, but it still preserves stale contents forever when the source jar entry changes. Please update
A better shape is: var wasExistingOutputEntry = existingEntries.Remove (apk_path);
using (var stream = File.OpenRead (jar_file_path))
using (var jar = ZipArchive.Open (stream)) {
var jar_item = jar.ReadEntry (jar_entry_name);
if (apk.ContainsEntry (apk_path)) {
if (!wasExistingOutputEntry) {
// Current packaged resource or earlier archive item wins.
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;
}
if (apk.GetEntry (apk_path).CRC == jar_item.CRC) {
Log.LogDebugMessage ("Skipping {0} from {1} as it is up to date.", jar_entry_name, jar_file_path);
continue;
}
apk.DeleteEntry (apk_path);
}
// extract jar_item and add it as today
}Then update the regression test so the existing APK entry has stale contents and the jar has current contents, and assert that the APK ends with the current contents. That proves the incremental bug is fixed rather than only hiding the deletion symptom. The test can still keep an unrelated stale entry to verify cleanup behavior. |
Co-authored-by: simonrozsival <374616+simonrozsival@users.noreply.github.com>
Co-authored-by: simonrozsival <374616+simonrozsival@users.noreply.github.com>
Addressed in 95e7370 and 2c430e7. Jar-sourced entries now distinguish existing output entries from current-build duplicates, refresh stale existing APK contents from the jar, skip up-to-date CRC matches, and keep cleanup coverage for unrelated stale entries. |
|
/review |
|
✅ Android PR Reviewer completed successfully! |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
@copilot
Issues (1
⚠️ Test fieldtempDirectoryshould be nullable (string?) for[SetUp]-initialized fields- 💡 New test file missing
#nullable enableand file-scoped namespace - 💡 Minor perf trade-off: jar is now opened before the duplicate check (acceptable for correctness)
- 💡 Consider a test for duplicate jar items targeting the same
apk_path
Positive notes:
- CRC comparison for staleness detection is consistent with the existing pattern used for
ApkInputPathentries (line 120) - Good separation of the "existing output entry" vs "current build duplicate" cases
- Regression tests cover both stale-update and up-to-date-skip paths
CI: Public checks pass. Internal Xamarin.Android-PR pipeline status should be verified separately.
Generated by Android PR Reviewer for issue #11552 · ● 7M
|
|
||
| using (var stream = File.OpenRead (jar_file_path)) |
There was a problem hiding this comment.
🤖 💡 Performance — The jar is now opened unconditionally, whereas previously the apk.ContainsEntry check was before File.OpenRead/ZipArchive.Open, allowing duplicates from current-build items to be skipped without touching the jar at all. This is probably fine in practice (OS file cache, rare duplicate path), but worth noting as a trade-off for correctness.
Rule: Place cheap checks before expensive ones
| @@ -0,0 +1,96 @@ | |||
| using System.Collections.Generic; | |||
There was a problem hiding this comment.
🤖 💡 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
| [TestFixture] | ||
| public class BuildArchiveTests | ||
| { | ||
| string tempDirectory; |
There was a problem hiding this comment.
🤖 tempDirectory is declared as non-nullable string but is only assigned in Setup(). Without #nullable enable this compiles, but the intent is unclear and fragile. Declare as string? tempDirectory; (consistent with the nullable pattern for [SetUp]-initialized fields per repo conventions).
Rule: Nullable reference types
| archive.AssertDoesNotContainEntry (apk, "stale.txt"); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🤖 💡 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
|
@copilot address code review |
Co-authored-by: simonrozsival <374616+simonrozsival@users.noreply.github.com>
Co-authored-by: simonrozsival <374616+simonrozsival@users.noreply.github.com>
|
/review |
|
❌ Android PR Reviewer failed. Please review the logs for details. |
|
/review |
|
❌ Android PR Reviewer failed. Please review the logs for details. |
There was a problem hiding this comment.
Pull request overview
This PR improves incremental APK publishing behavior in BuildArchive for files sourced from jar entries, ensuring that existing APK entries are treated as “retained” build outputs (not as duplicates) and are updated when stale.
Changes:
- Update
BuildArchivejar-entry handling to distinguish existing output entries from same-build duplicates and to replace stale existing entries when contents change (CRC differs). - Add focused unit tests covering jar-sourced entries that already exist in the output APK (update, skip when up-to-date, and duplicate handling).
Show a summary per file
| File | Description |
|---|---|
| src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Tasks/BuildArchiveTests.cs | Adds regression tests validating update/skip/duplicate behavior for existing jar-sourced APK entries. |
| src/Xamarin.Android.Build.Tasks/Tasks/BuildArchive.cs | Adjusts jar-entry logic to retain existing entries, skip true duplicates, and replace stale entries based on CRC comparison. |
Copilot's findings
- Files reviewed: 2/2 changed files
- Comments generated: 2
|
@copilot address review comments |
…disk in duplicate test Co-authored-by: simonrozsival <374616+simonrozsival@users.noreply.github.com>
Both issues addressed in the latest commit:
All 3 |
|
/review |
|
✅ Android PR Reviewer completed successfully! |
There was a problem hiding this comment.
@copilot
✅ LGTM — solid incremental build fix
CI: All checks green (dotnet-android ✅, Xamarin.Android-PR ✅, license/cla ✅).
Summary
The core logic change correctly distinguishes "entry from a previous build output" vs "entry added by another current-build item," fixing the bug where jar-sourced APK entries were incorrectly treated as duplicates and then deleted during cleanup on every incremental build. The CRC-based staleness check and delete-then-re-add pattern follows the existing established pattern in the same method (lines 117–133).
Positive callouts
- 👍 The
jar_item == nullnull check (line 173) is a nice defensive addition — the original code would have crashed with aNullReferenceExceptiononjar_item.Extract()if the jar entry was missing. - 👍 Tests are well-structured, cover the three main scenarios (update, skip, duplicate), and use appropriate assertions including message verification.
Issues
| Severity | Count | Category |
|---|---|---|
| 💡 | 3 | Cleanup edge case, log format consistency, test coverage |
No blocking issues found — all three inline comments are optional suggestions.
Generated by Android PR Reviewer for issue #11552 · ● 17.3M
|
@copilot address code review |
…anup on missing jar entry, add null-jar-entry test Co-authored-by: simonrozsival <374616+simonrozsival@users.noreply.github.com>
Addressed in the latest commit:
|
|
/review |
|
✅ Android PR Reviewer completed successfully! |
There was a problem hiding this comment.
@copilot ✅ LGTM — Clean, well-tested fix.
What the PR does: Fixes incremental BuildArchive behavior so that jar-sourced APK entries from previous builds are properly updated (or removed) instead of being incorrectly treated as "already added" duplicates.
Analysis:
- The core logic change (
existingEntries.Remove+ CRC-based up-to-date check +DeleteEntrybefore re-add) is correct. All code paths converge correctly for stale, up-to-date, duplicate, and missing-entry scenarios. IZipArchive.GetEntrythrows on missing entries, so the unchecked call on line 181 is safe given thehasApkEntryguard.- The
existingEntriesre-add whenjar_item == nullcorrectly restores cleanup semantics — the stale APK entry will be removed during the end-of-task cleanup loop. - Test coverage is solid: 4 focused unit tests exercise the key incremental scenarios without needing a full build.
Issues: 0 ❌ · 0
| Severity | File | Summary |
|---|---|---|
| 💡 | BuildArchiveTests.cs |
Use [] collection expressions for ITaskItem[] array initialization (6 instances) |
| 💡 | BuildArchive.cs:181 |
Consider adding a brief comment explaining why CRC-only comparison is used here vs CRC+CompressedSize in the input-APK refresh path |
CI: ✅ All checks passing.
Generated by Android PR Reviewer for issue #11552 · ● 30M
| }; | ||
|
|
||
| Assert.IsTrue (task.RunTask (), "task should have succeeded"); | ||
|
|
There was a problem hiding this comment.
🤖 💡 Formatting — This repo prefers [] collection expressions over new T[] { ... } (C# 12 syntax). Several places in this file can be simplified:
| FilesToAddToArchive = [item], |
Same applies to lines 79, 103, 116, 135, and 145.
Rule: Use [] not Array.Empty<T>() / new T[]
| } | ||
|
|
||
| if (hasApkEntry) { | ||
| if (apk.GetEntry (apk_path).CRC == jar_item.CRC) { |
There was a problem hiding this comment.
🤖 💡 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.
| 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"
BuildArchivecould remove or preserve stale Java/JAR resources from incremental APK publishes when the previous APK already contained those entries. Duplicate jar-entry adds were skipped, but existing output entries were not distinguished from duplicates added by current build inputs.Archive retention and updates
FilesToAddToArchivepaths as retained before cleanup.FilesToAddToArchive.Regression coverage
BuildArchivetests for existing jar-sourced APK entries.