[FastDev] Add faster FastDeploy2#11698
Conversation
Add a new FastDeploy2 strategy that uses a local manifest to push only changed files to temporary device storage and mirrors the app override directory with shell-created symlinks. Keep legacy FastDeploy selectable while making FastDeploy2 the default. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Remove existing override paths before copying changed files so Copy mode can safely recover from a symlink-based override tree. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR introduces a new manifest-driven fast deployment implementation (FastDeploy2) alongside the legacy FastDeploy, and makes FastDeploy2 the default strategy for app install fast deployment.
Changes:
- Register and invoke the new
FastDeploy2MSBuild task, with_AndroidFastDevStrategydefaulting toFastDeploy2. - Add
FastDeploy2Base(core orchestration + adb/run-as helpers) plus a manifest-basedFastDeploy2implementation that pushes only changed files and updates overrides via symlink (with copy fallback). - Add MSBuild-time validation for supported strategy/transfer-mode values, and a new adb compression algorithm property default (
any).
Show a summary per file
| File | Description |
|---|---|
| src/Xamarin.Android.Build.Debugging.Tasks/Xamarin.Android.Common.Debugging.targets | Registers FastDeploy2, selects strategy, sets defaults for strategy/transfer mode/compression, and wires task invocation into _Upload. |
| src/Xamarin.Android.Build.Debugging.Tasks/Tasks/FastDeploy2.cs | Adds FastDeploy2Base task implementation: device/package orchestration, adb invocation, file staging/stat/diff/copy paths, diagnostics logging, and error handling. |
| src/Xamarin.Android.Build.Debugging.Tasks/Tasks/FastDeploy2.Manifest.cs | Implements manifest-driven incremental upload, remote “ready” marker handling, stale cleanup, and shell-symlink override updates with copy fallback. |
Copilot's findings
- Files reviewed: 3/3 changed files
- Comments generated: 8
simonrozsival
changed the title
Fix manifest reset handling, device-scoped manifest state, adb batching edge cases, symlink/copy recovery, and diagnostics logging concurrency. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Use shell variables and cd to avoid repeating long staging and override paths in generated run-as symlink scripts. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Drop leftover experimental staging and symlink helper methods that are no longer referenced by the manifest-driven direct-push implementation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Skip missing FastDev inputs before manifest creation and clear symlink-managed override state before Copy mode so stale symlinks cannot survive mode switches or fallback. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
simonrozsival
changed the title
|
/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.
⚠️ Needs Changes
Thorough review of the new FastDeploy2 implementation. The architecture is well thought out — manifest-based incremental diffing, a clean strategy/mode split in the targets with condition-first <Error> validation, careful shell-argument quoting (ShellQuote single-quote escaping) and package-name sanitization (IsSafePackageNameForShell) to prevent shell injection, ADB cancellation handling with process kill + output draining, and reuse of the existing XA012x/XA013x error codes. Nice work.
A few things to address before merge:
Issues by severity
⚠️ 2 warnings- Order-dependent shell globbing (
FastDeploy2.Manifest.cs:132): the full-refreshrm -f ./*/ln -sf "$s"/* .globs also match subdirectories such as the staged{abi}/environment. Becausedirectoriesis iterated in nondeterministicHashSetorder, this can either delete real staged files through a directory symlink or silently fall back to copy. Please handle nested directories explicitly. - Dead/duplicated base implementation (
FastDeploy2.cs:549): thevirtual DeployFastDevFilesWithAdbPushand its exclusive helpers are fully superseded by the override and never run — including a duplicatedCreatePushArgsPrefixthat still uses--sync. Recommend deleting it (or making the methodabstract).
- Order-dependent shell globbing (
- 💡 2 suggestions
MarkRemoteReadyignores the adb exit code (FastDeploy2.Manifest.cs:447).ManifestEntry.LocalPathis persisted but never read (FastDeploy2.Manifest.cs:496).
See the inline comments for mechanisms and concrete fixes.
CI: license/cla passed; the Azure DevOps builds (Linux/Mac/Windows) are still in progress at review time, so green CI is not yet confirmed — please ensure the internal pipeline passes before merge.
The symlink-glob behavior is the main correctness concern; the rest are maintainability cleanups. I couldn't reproduce on-device, so if I've misread the staging layout for the glob case, a quick confirmation of how {abi}/environment is handled during a full refresh would resolve it.
Generated by Android PR Reviewer for issue #11698 · 1.4K AIC · ⌖ 48.9 AIC · ⊞ 37.8K
Comment /review to run again
|
|
||
| async Task MarkRemoteReady (string remoteStagingPath) | ||
| { | ||
| await RunAdbCommand ("shell", "touch", $"{remoteStagingPath}/{RemoteReadyMarker}"); |
There was a problem hiding this comment.
🤖 💡 Error handling — MarkRemoteReady discards the RunAdbCommand result, so a failed touch of the ready marker goes unnoticed. Every other adb call in this file checks result.ExitCode (e.g. line 305). If the marker isn't written, IsRemoteReady returns false next time and the deploy silently degrades to a full push — correct, but a hidden perf cliff. Consider checking the exit code and logging a diagnostic on failure.
{Rule: Check command exit codes consistently}
| LogDebugMessage ($"{PackageName} Terminated."); | ||
| } | ||
|
|
||
| protected virtual async Task<bool> DeployFastDevFilesWithAdbPush (string overridePath) |
There was a problem hiding this comment.
🤖 FastDeploy2Base.DeployFastDevFilesWithAdbPush is virtual and fully overridden by FastDeploy2 (in FastDeploy2.Manifest.cs), which never calls base.. Since FastDeploy2Base is abstract and FastDeploy2 is its only subclass, this entire base implementation is unreachable — along with the helpers only it reaches: RemoveStaleRemoteStagingFiles (line 729), UploadFiles (947), BatchPushFiles (972), CreatePushArgs (999), CreatePushArgsPrefix (1007), EstimateCommandLength (1018), and SetAdbPushFileCounts (1042, which has no callers at all).
Three of these (CreatePushArgs / CreatePushArgsPrefix / EstimateCommandLength) are also duplicated in the live override, and the dead base CreatePushArgsPrefix still appends --sync — the exact behavior the PR description says FastDeploy2 no longer relies on. Recommend deleting the dead base implementation (or making the method abstract) so only the shipped manifest-based path remains, to avoid future readers maintaining or copying the abandoned --sync variant.
{Rule: Ship only what's needed (YAGNI / dead code)}
| public string RelativePath { get; set; } | ||
|
|
||
| [JsonPropertyName ("localPath")] | ||
| public string LocalPath { get; set; } |
There was a problem hiding this comment.
🤖 💡 Code organization — ManifestEntry.LocalPath is written in CreateManifest (line 255) and serialized to manifest.json, but never read back — GetChangedFiles only compares Size and LastWriteTimeUtcTicks, and the key is RelativePath. It bloats the on-disk manifest and persists machine-specific absolute paths to disk. Consider dropping the field and its localPath JSON property.
{Rule: Ship only what's needed (YAGNI / dead code)}
| string sourceDirectory = string.IsNullOrEmpty (directory) ? remoteStagingPath : $"{remoteStagingPath}/{directory}"; | ||
|
|
||
| if (currentInDirectory.Count > 0 && (previousManifest == null || newInDirectory.Count == currentInDirectory.Count)) { | ||
| string script = $"d={ShellQuote (targetDirectory)};s={ShellQuote (sourceDirectory)};mkdir -p \"$d\"&&cd \"$d\"&&rm -f ./*&&ln -sf \"$s\"/* ."; |
There was a problem hiding this comment.
🤖 rm -f ./* and ln -sf "$s"/* . glob everything in the directory, including subdirectories. The staged tree does contain subdirectories — the environment file is staged as {PrimaryCpuAbi}/environment (FastDeploy2.cs line 648) — so directories (line 119) holds both "" and "{abi}", iterated in nondeterministic HashSet<string> order.
- If root
""is processed first,ln -sf "$s"/* .creates a symlink{abi} -> staging/{abi}in the override dir; the later{abi}iteration thencds through that symlink andrm -f ./*deletes the real staged files under staging. - In the opposite order,
rm -f ./*hits the real{abi}subdirectory and errors withIs a directory, trippingIsShellError (output, "rm")→ silent fallback to copy.
Either way the result depends on iteration order. Consider excluding child directories from the root-level glob, or routing any directory that has nested subdirectories through the explicit per-file CreateShellSymlinkScripts path (which lists filenames rather than *).
{Rule: Avoid order-dependent shell globbing}
jonathanpeppers
left a comment
jonathanpeppers
left a comment
There was a problem hiding this comment.
I think I want to see a .binlog of a deploy to understand what it does -- I might have time to manually test some later.
Overall, I like what it does -- introduce a private property by default you can opt out of if something goes wrong.
| static string GetSafeFileName (string value) | ||
| { | ||
| if (string.IsNullOrEmpty (value)) { | ||
| return "_"; | ||
| } | ||
|
|
||
| var builder = new StringBuilder (value.Length); | ||
| foreach (char c in value) { | ||
| builder.Append (char.IsLetterOrDigit (c) || c == '.' || c == '-' || c == '_' ? c : '_'); | ||
| } | ||
| return builder.ToString (); | ||
| } |
There was a problem hiding this comment.
Is there an existing helper somewhere that does this? I can think of maybe things related to Java names, or Android resources.
|
@jonathanpeppers I captured a binlog from building a fresh https://gist.github.com/simonrozsival/c06cacff00c99ee3504c256205e24492 Command used for the successful binlog: ./dotnet-local.sh build JonMauiBinlog/JonMauiBinlog.csproj \
-f net11.0-android \
-p:TargetFrameworks=net11.0-android \
-p:SupportedOSPlatformVersion=24.0 \
-bl:JonMauiBinlog-net11.0-android.binlogThe TFM override keeps the template Android-only because I only installed |
Move FastDeploy2 diagnostic JSON helpers out of the main task, use System.Text.Json source generation for FastDeploy2 JSON, remove unused FastDeploy2 task inputs, and consolidate repeated path/grouping helpers. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Flatten FastDeploy2 into a single concrete task, remove the development diagnostics property bag and JSON payload, and keep only the functional manifest serialization state. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
I think your command needs |
|
@jonathanpeppers you're right — the earlier binlog was build-only. I updated the same gist with a new https://gist.github.com/simonrozsival/c06cacff00c99ee3504c256205e24492 New file: Command used: ./dotnet-local.sh build JonMauiBinlog/JonMauiBinlog.csproj \
-t:Install \
-f net11.0-android \
--no-restore \
-m:1 \
-nr:false \
-p:TargetFrameworks=net11.0-android \
-p:SupportedOSPlatformVersion=24.0 \
-p:AdbTarget=-s%20R58Y30HZ65V \
-p:_FastDeploymentDiagnosticLogging=False \
-bl:JonMauiBinlog-net11.0-android-install-fastdeploy2.binlog \
-clp:PerformanceSummary \
-v:minimalThe console log shows |
Avoid logging empty run-as command output and buffer optional missing-file messages behind FastDeploy2 diagnostics so normal install binlogs stay readable. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
There are many log messages that just say 
I think we should probably skip these messages in general unless But then I probably need to see a log where |
Stage FastDeploy2 files under /data/local/tmp instead of /tmp so Android emulators with read-only /tmp can install. Also remove existing override contents recursively before full symlink refreshes so resource/culture directories do not fail rm. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Summary
This is the cleaned-up follow-up to the experimentation PR:
This PR keeps legacy
FastDeployin place, adds a newFastDeploy2strategy, and makesFastDeploy2the default for app install fast deployment.FastDeployremains selectable as a fallback.Review follow-up
Jon's review feedback is addressed in the latest cleanup commit:
FastDeploy2Baseinheritance split.System.Text.Jsonsource-generated context./data/local/tmp/fastdeploy2instead of/tmp/fastdeploy2, because/tmpis read-only on CI emulators.A fresh
dotnet new mauiAndroid sample build binlog is available here:FastDeploy2 approach
FastDeploy2 uses the final direction from the experiment PR:
Build a local manifest of FastDev inputs:
Compare with the last successful deploy manifest.
Push only changed files to device temp storage, without
adb push --sync:Remove stale staged files for removed inputs.
Maintain
files/.__override__as symlinks to staged files with batchedrun-as sh -ccommands. The generated shell scripts use shortd/svariables pluscdso long source/target directories are not repeated for every file.Write the manifest only after a successful deploy.
Why this shape is viable now
The existing FastDeploy v2.0 design was added when Android 11 broke the older external-storage based fast deployment model. At that time, adb compression and multi-file push/sync behavior were still new: Android SDK Platform-Tools 30.0.0 (April 2020) introduced client-side compression for
adb {push,pull,sync}when used with Android 11 devices, and Platform-Tools 30.0.5 (November 2020) later fixedadb push --syncwith multiple inputs and improved pushing many files over high-latency links.Today it is more reasonable to depend on a modern Android SDK Platform-Tools package for developer inner-loop scenarios.
adbis supplied by the Android SDK Platform-Tools package, and developers using .NET for Android / MAUI can update it independently of end-user device support. With Platform-Tools 36.0.0,adb push -z anywas also tested against API 24 and API 29 emulators that did not advertisesendrecv_v2*compression features; both accepted the command and transferred the file, apparently negotiating down to uncompressed transfer.This PR also no longer relies on
adb push --sync: the local manifest tells us exactly which files changed, so FastDeploy2 pushes only those files and avoids an adb-side scan of the full directory.What else we tried but abandoned
From the experiment PR:
FastDeployis excellent for very small changes but very slow for full payloads.adb push --syncover the whole directory is much faster for full payloads, but still scans/probes too much for tiny changes.adb pushper file is too slow because every adb invocation has ~40ms median overhead even when skipped.maui.link), so this PR does not include a native helper.How legacy FastDeploy could be improved instead
If we wanted to keep the native FastDeploy tool model, the most promising improvement would be to make
xamarin.syncbatch-aware. Instead of invokingrun-as ... xamarin.synconce per changed file, the host could open a singlerun-as <package> xamarin.sync-batchstdin stream and write all changed files one after another using a small binary protocol:The device helper would create directories, write each file to a temporary path, set mtime/permissions, rename into place, and remove stale files. This would keep the existing app-private real-file model and avoid symlink assumptions, while removing most per-file adb/run-as process overhead.
That approach is intentionally not part of this PR because it keeps the custom native tool/protocol complexity. FastDeploy2 tries the simpler path first: use adb's built-in transfer/compression support, keep the changed-file decision local, and leave legacy
FastDeployavailable as a fallback while we validate the symlink approach across more devices.Preliminary benchmark from the experiment PR
Device: Samsung Galaxy A16 (
SM-A165F,R58Y30HZ65V)Project:
samples/HelloWorld/HelloWorld/HelloWorld.DotNet.csprojValidation
Focused task builds pass in this branch:
Device/emulator validation passed:
SM-A165F,R58Y30HZ65V)SM-A165F,R58Y30HZ65V)SM-A165F,R58Y30HZ65V)d/sshell-variable symlink scriptsObserved validation details:
Additional adb compatibility probes with Platform-Tools 36.0.0:
adb push -z anycmdonly, nosendrecv_v2*cmdonly, nosendrecv_v2*sendrecv_v2,brotli,lz4,zstdRisks
FastDeployis still present and selectable.files/.__override__to/data/local/tmp/fastdeploy2/...worked on the tested Samsung device.adb push -z any; older devices can still fall back to uncompressed transfer when driven by a modern adb.Future work
FastDeployimplementation once we are confident the new approach is better across a broader device matrix.FastDeployno longer needs them.xamarin.syncprotocol as the app-private real-file fallback design.