[build] Remove LibZipSharp - #12449
Conversation
Move BuildArchive to the .NET 12 task assembly and replace remaining LibZipSharp usages with System.IO.Compression. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4a826a98-ebc1-4ff9-9fa8-4a71854812f4
Use ZipArchiveEntry.Crc32 directly and avoid opening malformed empty stored entries found in AndroidX archives. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4a826a98-ebc1-4ff9-9fa8-4a71854812f4
Keep stored class archive creation in the modern BuildArchive task, restore coded metadata diagnostics, read CRCs from central-directory metadata, and separate read-only archive access from updates. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4a826a98-ebc1-4ff9-9fa8-4a71854812f4
Add ZIP64 metadata support, dispose failed archive opens, use indexed ordinal lookups, and bound update-mode buffering with periodic archive commits. Keep the modern task on the repository target framework so the .NET 11 CI toolchain can build it and it advances automatically with the repository. Also restore creation of new Javadoc sample repositories and make their serialized XML encoding consistent. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4a826a98-ebc1-4ff9-9fa8-4a71854812f4
Use modern ZipArchiveEntry metadata in the out-of-process task and reduce the netstandard ZIP parser to the ordered name/CRC data required for incremental hashing. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4a826a98-ebc1-4ff9-9fa8-4a71854812f4
|
/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.
❌ Reject
Found 2 errors and 1 warning:
- The
tmtAPK path closes its archive before deferred managed assembly resolution. System.Text.Encoding.CodePages.dllis missing from the installer payload for the new BaseTasks dependency.- Two rented
MemoryStreaminstances are disposed instead of returned to their pool.
The ZIP64-aware metadata work, explicit ordinal entry lookup, constructor-failure disposal, and expanded archive regression coverage are strong improvements.
CI is not green: Azure build #1560846 currently reports failed Linux Build and Windows Build & Smoke Test checks, with remaining macOS checks still running or queued. Azure timeline access was blocked in this review environment, so I have not attributed those failures to this PR without evidence.
Generated by Android PR Reviewer for #12449 · gpt56 · 1.2K AIC · ⌖ 9.14 AIC · ⊞ 25.7K
Comment /review to run again
|
|
||
| var ret = new List<ITypemap> (); | ||
| ZipArchive zip = ZipArchive.Open (filePath, FileMode.Open); | ||
| using var zip = Utils.OpenZip (filePath); |
There was a problem hiding this comment.
🤖 ❌ Resource management — This disposes the APK before the returned typemaps are consumed. ApkManagedTypeResolver retains ZipArchiveEntry instances for individual assemblies, but main.cs calls typemap.Load() only after TryLoad() returns; resolving an assembly then attempts to extract from this closed archive and throws ObjectDisposedException. Please keep the archive alive through typemap loading, or eagerly cache the assembly data needed by the resolver.
Rule: Resource lifetime
|
|
||
| <ItemGroup> | ||
| <PackageReference Include="System.IO.Hashing" Version="$(SystemIOHashingPackageVersion)" /> | ||
| <PackageReference Include="System.Text.Encoding.CodePages" Version="$(SystemTextEncodingCodePagesVersion)" /> |
There was a problem hiding this comment.
🤖 ❌ MSBuild tasks — This adds a runtime dependency on System.Text.Encoding.CodePages, but build-tools/installers/create-installers.targets does not add its DLL to the explicit _MSBuildFiles payload (unlike System.IO.Hashing.dll). Installed full-framework MSBuild can therefore fail when ZipArchiveMetadataReader initializes CodePagesEncodingProvider; the broad hash fallback then treats ZIPs as changed and breaks incrementality. Please include System.Text.Encoding.CodePages.dll in the installer payload.
Rule: Runtime dependency closure
|
|
||
| currentEntry?.Delete (); | ||
|
|
||
| using var buffer = MemoryStreamPool.Shared.Rent (); |
There was a problem hiding this comment.
🤖 MemoryStreamPool.Shared.Rent() does not return it to the pool; callers must invoke MemoryStreamPool.Shared.Return() in finally. This path can retain or allocate large JAR-entry buffers repeatedly and add LOH/GC pressure. The same issue occurs in src/Microsoft.Android.Build.BaseTasks/ZipArchiveExtensions.cs:212; please fix both sites with the existing rent/try/finally/return pattern.
Rule: Return rented objects to their pool
1 participant
Description
Moves
BuildArchiveinto the out-of-process-capableMicrosoft.Android.Build.Tasksassembly targeting the repository's current .NET TFM and replaces LibZipSharp withSystem.IO.Compressionthroughout the build, bootstrap tooling, tests, and repository utilities. The task therefore builds for .NET 11 today and will advance to .NET 12 with the repository-wide target-framework update.The new ZIP helpers preserve stored/deflated compression when renaming entries, use ZIP64-aware central-directory metadata where
netstandard2.0lacks public compression metadata, and keep ZIP-content comparisons deterministic. LibZipSharp package references, native payload packaging/signing, installer entries, and third-party notices are removed. Existing archive flush limits remain supported and periodically commitZipArchiveMode.Updatechanges to bound retained memory.Validation
dotnet test tests/Microsoft.Android.Build.BaseTasks-Tests/Microsoft.Android.Build.BaseTasks-Tests.csproj— 130 passed, 4 skippeddotnet build src/Microsoft.Android.Build.Tasks/Microsoft.Android.Build.Tasks.csprojand focusedBuildArchiveTests— passed, including stored-entry rename, AAB manifest relocation, zero-byte uncompressed incrementality, ZIP64 metadata, constructor-failure disposal, and periodic archive commitsSampleRepositoryTests— passed, including creation and reopening of a previously missing sample archivemake jenkins CONFIGURATION=Release MSBUILD_ARGS=''— passed locally after restoring the repository-aligned task TFMdotnet build src/Xamarin.Android.Build.Tasks/Xamarin.Android.Build.Tasks.csproj— passeddotnet build build-tools/Xamarin.Android.Tools.BootstrapTasks/Xamarin.Android.Tools.BootstrapTasks.csproj— passeddotnet build src/Xamarin.Android.Tools.JavadocImporter/Xamarin.Android.Tools.JavadocImporter.csproj— passeddotnet build tools/tmt/tmt.csproj— passeddotnet build tests/MSBuildDeviceIntegration/MSBuildDeviceIntegration.csproj— passedLibZipSharp,Xamarin.Tools.Zip, orUseLibZipSharpreferencesMAUI build performance
Measured an identical
dotnet new maui --sample-content -f net11.0app against the exact parent commit and this branch. Both variants used the same .NET 11 RC host SDK, external runtime packs, MAUI packages, Android SDK, and source-built Android workload pack; builds used--no-restore, and variant order alternated. APK entry names and compression modes matched: 1,275 entries, with 1,080 deflated and 195 stored.BuildArchivetask, median (3 pairs)BuildArchivetask, median (10 pairs)The paired 95% confidence interval for the total raw-asset build delta is -0.01 to +1.36 seconds, so the end-to-end improvement is promising but not statistically conclusive on this noisy host. Clean total-build and no-op wall times were too variable to attribute a difference; no-op builds do not execute
BuildArchive.For .NET/Core MSBuild, the parent already selects its
System.IO.Compressionimplementation, so these measurements compare the oldnetstandard2.0reflection/wrapper path with the modern public APIs rather than native LibZipSharp directly. The actual LibZipSharp-to-built-in transition is the Visual Studio/full-framework MSBuild path, which this macOS benchmark cannot measure.The benchmark also found two compatibility issues now fixed in
a5f720a9a: malformed empty stored entries used by AndroidX archives are extracted without opening their invalid payload, andBuildArchiveusesZipArchiveEntry.Crc32directly instead of recomputing CRC throughSystem.IO.Hashing.Review follow-up
cc52ef22bkeeps storedclasses.zipcreation in the modernBuildArchivetask instead of relying on full-frameworkCompressionLevel.NoCompression, restores localizedXA4234metadata diagnostics, reads ZIP comparison CRCs directly from ordered central-directory metadata, and separates read-only archive access from update access. A clean MAUI sample build produced 557 stored class entries, and subsequent no-op and raw-asset update builds succeeded.Review follow-up
be5565cf4adds ZIP64 metadata support, avoids file-handle leaks when archive construction fails, uses indexed ordinal entry lookup, and restores bounded update-mode commits. It also fixes creation and UTF-8 index serialization for new Javadoc sample repositories. The previous CI failure wasNETSDK1045: the task and test projects hard-codednet12.0while the repository installs .NET 11. Both now use$(DotNetTargetFramework), preserving the modern out-of-process task design and automatically following the repository's eventual .NET 12 update.