Skip to content

Restore and consolidate the app bundle copy on BundleFileManager - #217

Merged
shanerbaner82 merged 7 commits into
NativePHP:mainfrom
trailhead-labs:feat/bundle-copy-consolidation
Jul 27, 2026
Merged

shanerbaner82 merged 7 commits into
NativePHP:mainfrom
trailhead-labs:feat/bundle-copy-consolidation

Conversation

@gwleuverink

Copy link
Copy Markdown
Contributor

The SuperNative squash (#173) carried a pre-#30 copy of BuildIosAppCommand.php, reverting the iOS app copy to an inline directory iterator that ignores config('nativephp.cleanup_exclude_files').

BundleFileManager and BundleExclusions were 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:

  1. 11f5704 anchors VENDOR_PATTERNS under vendor/**/. Unanchored, *.md/*.yaml/docs matched at any depth and silently stripped an app's own files from the bundle. Verified against openrsync (macOS default)
  2. e29a320 restores the iOS BundleFileManager wiring 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.
  3. f405078 routes the Android bundle prep through BundleFileManager too, 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, /XF for files, separator normalisation).
  4. f6db135 reduces platformOptimizedCopy to a plain full-tree copy. Its exclude branches had no remaining callers and duplicated the robocopy logic now owned by the manager.
  5. ac7196a prunes VENDOR_PATTERNS from the copied tree after robocopy, which cannot express rsync's multi-level vendor/** wildcards. With this, BundleFileManager::copy() produces the same result on every platform.
  6. 061d255 removes a source-grep test whose claims are covered by the Process::fake tests 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:

  • Small privacy fix: Unix-built bundles shipped storage/logs/laravel.log; Windows already stripped it. Now stripped everywhere.
  • Dropping the seeded database.sqlite is safe by inspection: the Android runtime creates its own at persisted_data/database/database.sqlite and runs migrate --force on first boot (LaravelEnvironment.kt), it never reads a bundled one.

Vendor slimming survives composer install because vendor/composer/installed.json and the lock file are copied intact, so composer treats packages as installed and does not restore stripped files.

Tests

  • Behavioral copy test staging a real tree: app files survive, vendor non-runtime files are stripped at every depth. String-matching rsync flags is what let the anchoring bug hide, so the outcome is asserted directly.
  • Regression guards asserting both platforms route copy and cleanup through BundleFileManager (the failure mode that let SuperNative #173 revert feat(ios): replace PHP iterator with rsync for app copy step #30 silently).
  • Behavioral zip assertions for Android: config excludes shape the archive, artisan/composer.lock are copied for composer then cleaned, artisan.php/.version survive, runtime dirs are re-added empty.
  • Robocopy command-shape tests (any-depth names, wildcard expansion, /XF vs /XD, export-ignore patterns, failure exit codes), faked so they run on any platform.
  • Behavioral prune test: a destination staged as robocopy leaves it comes out matching what rsync produces, vendor root files and runtime code untouched.

Worth checking before merge

  • A Windows-hosted Android build end to end. The robocopy path is only string-asserted in CI.

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

@shanerbaner82

Copy link
Copy Markdown
Contributor

@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:

  • The anchoring bug was real and user-visible. Staged a tree and ran actual rsync: an app's own docs/guide.md, app/Support/NOTES.md and config.yaml were being silently stripped from bundles, and now survive, while vendor's .md/LICENSE/docs/.yml still go and vendor/composer/installed.json stays. Worth a line in the release notes — people were losing their own files.
  • The robocopy/rsync parity claim holds. Staged a destination the way robocopy leaves it, ran pruneVendorPatterns, diffed against the rsync output: identical apart from the root README.md, which robocopy excludes at copy time via its own glob → /XF branch.

Also ran a full BundleFileManager::copy() over a real app — every runtime-critical path survives (bootstrap, config, routes, vendor/composer/installed.json, framework, views, migrations, .env), 640 MB → 81 MB. And the generated robocopy command comes to ~2.9k characters against cmd.exe's 8191 limit, so there's comfortable headroom there.

The conflicts

#224 landed first, which moved src/Traits → src/Concerns, so this branch has to absorb the rename. Three things, the last one being the one to watch:

  1. BuildIosAppCommand — keep BundleFileManager, take the renamed Concerns\* imports, and drop Illuminate\Support\Str: this branch removed its last use along with the inline iterator, so it's now unused.
  2. ReleaseBuildBundleTest — keep BundleFileManager, take Concerns\PreparesBuild, drop BuildIosAppCommand and ReflectionClass (the source-grep test that used them was removed here).
  3. AndroidBundleCopyTest ⚠️ — git can't help with this one. It's a new file on this branch, so there's no rename for it to follow, and it keeps importing Native\Mobile\Traits\PreparesBuild. The merge comes out clean and the diff looks fine, but it fatals. Needs changing to Native\Mobile\Concerns\PreparesBuild by hand.

I've pushed my resolution to merge/bundle-copy-consolidation on this repo if it's useful to diff against — 748 tests green, Pint clean, and I re-checked afterwards that the vendor/**/ anchoring and the BundleFileManager wiring both survived the merge. Your PR branch is on a fork with maintainer-edit off, so I couldn't push it to you directly.

Give @shanerbaner82 a mention once you've merged main in and we'll get it landed.

Two small things for the release notes rather than the code, neither blocking:

  • Android now drops a seeded database/database.sqlite. Your reasoning is sound and it matches the documented "use migrations for seeding" guidance, but it's a visible change for anyone shipping a pre-seeded DB.
  • storage/logs is now stripped everywhere, which is a genuine privacy improvement worth surfacing — on this machine that was ~19 MB of Jump logs (edge-nav.log alone was 11.7 MB) that would previously have gone into the bundle. It isn't in $requiredDirs, so it's not recreated empty; Monolog's StreamHandler::createDir() will mkdir it on first write so it should self-heal, but adding it there would remove the doubt.

🤖 Generated with Claude Code

@shanerbaner82 shanerbaner82 added the enhancement New feature or request label Jul 26, 2026
…nsolidation

# Conflicts:
#	src/Commands/BuildIosAppCommand.php
#	tests/Feature/ReleaseBuildBundleTest.php
@gwleuverink

Copy link
Copy Markdown
Contributor Author

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.

@shanerbaner82
shanerbaner82 merged commit 7e7365f into NativePHP:main Jul 27, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants