Repository navigation
Restore and consolidate the app bundle copy on BundleFileManager - #217
Conversation
Already covered by the Process::fake tests that run the real backends
|
@gwleuverink this one's good to go from our side — a Windows-hosted Android build has been run and checks out, which was the last open item from your own pre-merge list. I went through it fairly carefully before that. Rather than take the claims on trust I re-ran the two that matter:
Also ran a full The conflicts#224 landed first, which moved
I've pushed my resolution to Give @shanerbaner82 a mention once you've merged Two small things for the release notes rather than the code, neither blocking:
🤖 Generated with Claude Code |
…nsolidation # Conflicts: # src/Commands/BuildIosAppCommand.php # tests/Feature/ReleaseBuildBundleTest.php
|
Thanks for the thorough review. Main is merged in. I resolved the same way as your merge/bundle-copy-consolidation branch (src and tests come out identical), the only difference is that mine includes the commits that landed on main after you cut yours. One heads-up from the merge: phpstan reports 17 errors on current main itself. Looks like #224's Traits → Concerns rename broke the path contexts in phpstan.neon's ignoreErrors, so previously ignored errors resurfaced. Not related to this branch, but you'll probably want a small follow-up for it. (I'm not sure why it comes out green in CI here, it. explodes locally) On storage/logs in $requiredDirs: agreed it removes the doubt, happy to add it here if you want it in this PR, otherwise I'll do it as a follow-up right after. @shanerbaner82 ready for another look. |
The SuperNative squash (#173) carried a pre-#30 copy of
BuildIosAppCommand.php, reverting the iOS app copy to an inline directory iterator that ignoresconfig('nativephp.cleanup_exclude_files').BundleFileManagerandBundleExclusionswere left on disk unused, and nothing asserted the wiring, so the suite stayed green. This only affects the 4.0 RC, but it should be fixed before stable.While restoring that, two more things surfaced: the vendor slimming patterns were unanchored (a bug), and Android still had its own parallel copy implementation. This PR fixes all three, split into commits for review:
VENDOR_PATTERNSundervendor/**/. Unanchored,*.md/*.yaml/docsmatched at any depth and silently stripped an app's own files from the bundle. Verified against openrsync (macOS default)BundleFileManagerwiring from feat(ios): replace PHP iterator with rsync for app copy step #30 and deletes the dead iterator. User-configured excludes work again and iOS bundles shrink back to their pre-4.0 size. A regression-guard test asserts the wiring, the exact assertion whose absence let SuperNative #173 revert feat(ios): replace PHP iterator with rsync for app copy step #30 unnoticed.BundleFileManagertoo, replacing the inline platform-specific exclude list, and adds the post-install cleanup pass iOS already had. The robocopy backend picks up the Windows fixes from fix: Windows and Linux compatibility across all CLI commands #214 (junction guard,/XFfor files, separator normalisation).platformOptimizedCopyto a plain full-tree copy. Its exclude branches had no remaining callers and duplicated the robocopy logic now owned by the manager.VENDOR_PATTERNSfrom the copied tree after robocopy, which cannot express rsync's multi-levelvendor/**wildcards. With this,BundleFileManager::copy()produces the same result on every platform.Process::faketests that run the real backends, and which would forbid the streaming iterator the prune legitimately uses.Why we trust the Android change
The old and new pipelines were run against the same staged app tree and the outputs diffed, including a simulation of the zip step's own filter. Nothing the old pipeline excluded ships now, and everything newly stripped (vendor tests/docs/licenses, the 15 MB Pint binary, seeded sqlite, storage caches) is exactly what iOS already strips. #31 measured this consolidation at roughly 35% smaller Android bundles.
The per-platform differences in the old exclude list turned out not to be design: the zip step's hardcoded filter equalised the final artifacts anyway, so the platform arms only decided at which stage filtering happened. Two genuine behavioral notes:
storage/logs/laravel.log; Windows already stripped it. Now stripped everywhere.database.sqliteis safe by inspection: the Android runtime creates its own atpersisted_data/database/database.sqliteand runsmigrate --forceon first boot (LaravelEnvironment.kt), it never reads a bundled one.Vendor slimming survives
composer installbecausevendor/composer/installed.jsonand the lock file are copied intact, so composer treats packages as installed and does not restore stripped files.Tests
BundleFileManager(the failure mode that let SuperNative #173 revert feat(ios): replace PHP iterator with rsync for app copy step #30 silently).artisan/composer.lockare copied for composer then cleaned,artisan.php/.versionsurvive, runtime dirs are re-added empty./XFvs/XD, export-ignore patterns, failure exit codes), faked so they run on any platform.Worth checking before merge
Relationship to #31
#31 attempted the Android consolidation right after #30 and stalled on test failures; the squash then rewrote the code underneath it. This PR redoes the work on the current codebase. Three of the four test files that blocked it no longer exist. Credit to @benjam-es for the original approach and the benchmarks.
Follow-ups, intentionally not here
PlatformFileOperationsintoBundleFileManagerand retire the trait's file ops, finishing the migration feat(ios): replace PHP iterator with rsync for app copy step #30/feat(android): port iOS rsync optimisations to Android build pipeline #31 started.createZipBundle's hardcoded 7-Zip and ZipArchive safety lists fromBundleExclusionsinstead of parallel copies.addDirectoryToZip, which dropsiterator_to_arrayand measured 10x lower peak memory.