Wire Android cmdline-tools resolver API into maui CLI - #403
Conversation
d837fe6 to
e3d6d63
Compare
e3d6d63 to
21569ba
Compare
Expert Code Review — PR #403Methodology: 2 independent reviewers with adversarial consensus (1 reviewer failed to complete — reduced coverage, no tiebreaker; in this mode only findings both reviewers agreed on are included, and split findings are discarded). Findings: 4 posted as inline comments — 0 critical, 2 moderate, 2 minor. No crash-class or data-loss defects. Inline findings (all 2/2 consensus):
Discarded (flagged by only one reviewer, no consensus tiebreaker available):
CI status: pending (no checks reported yet at time of review). Test coverage: Good — the PR adds 6 targeted tests (command wiring, JSON invocation, dry-run no-op, resolver path/revision, null-SDK-path, empty-path throw) and a fake-provider implementation. Not covered: the "already up to date" (
|
There was a problem hiding this comment.
Expert Code Review: 4 findings posted inline. See the summary comment for methodology and details. No crash-class or data-loss defects found.
Generated by Expert Code Review (auto) for #403 · 323.6 AIC · ⌖ 18.2 AIC · ⊞ 12.3K
| formatter.Write(new CliCommandResult | ||
| { | ||
| Success = true, | ||
| Status = "updated", |
There was a problem hiding this comment.
🟡 MODERATE · consensus 2/2
null revision is overloaded as an "already up to date" sentinel, but JSON still reports Status = "updated". When the tools are already current, EnsureLatestSdkToolsAsync returns null, yet the JSON result here hardcodes Status = "updated". Machine consumers (IDE extensions, CI scripts) checking status == "updated" will incorrectly conclude an update happened. The human-readable path handles this correctly ("Command-line tools are up to date"), so only the machine-readable output is misleading. More broadly, a null revision conflates "no change" with "revision string unavailable."
Recommendation:
Status = revision is null ? "up_to_date" : "updated",Consider having the wrapper return an explicit updated/unchanged signal rather than inferring state from a nullable revision.
| <!-- Android platform tooling (managed via darc/maestro — see eng/Version.Details.xml) --> | ||
| <PropertyGroup Label="Android Tools"> | ||
| <XamarinAndroidToolsAndroidSdkVersion>1.0.189-preview.58</XamarinAndroidToolsAndroidSdkVersion> | ||
| <XamarinAndroidToolsAndroidSdkVersion>99.61.4269-preview.2131</XamarinAndroidToolsAndroidSdkVersion> |
There was a problem hiding this comment.
🟡 MODERATE · consensus 2/2
Rolling preview pin from a transport feed. 99.61.4269-preview.2131 (and the matching SHA in eng/Version.Details.xml) comes from the dotnet11-transport feed, which prunes old CI builds. A restore that works today can later fail with "package not found," breaking CI and consumers, and there is no Maestro subscription managing the pin so it must be bumped manually. The PR description also references a different build (99.61.4266-ci.main.2128) than what actually landed — reconcile that so provenance is auditable.
Recommendation: Prefer a version that flows through a durable channel (Maestro/darc), or at minimum add a comment in Version.Details.xml flagging this as a manual transport pin to review on each bump.
| "Complete" => "Command-line tools updated", | ||
| _ => msg | ||
| }; | ||
| task.Update(Math.Max(0, pct), label); |
There was a problem hiding this comment.
🟢 MINOR · consensus 2/2
Progress percentage is only lower-clamped. Math.Max(0, pct) floors at 0 but does not cap the upper bound, and it collapses indeterminate phases (e.g. a -1 sentinel during CheckingForUpdates) to a stuck 0% bar. The sibling TryParseInstallProgressLine in SdkManager.cs uses Math.Clamp(p, 0, 100) — this call site is inconsistent with that pattern.
Recommendation: Use Math.Clamp(pct, 0, 100), and consider routing negative/sentinel values to the indeterminate spinner path instead of flooring to 0.
| var dryRun = parseResult.GetValue(GlobalOptions.DryRunOption); | ||
| var formatter = Program.GetFormatter(parseResult); | ||
|
|
||
| var targetPath = androidProvider.SdkPath ?? PlatformDetector.Paths.DefaultAndroidSdkPath; |
There was a problem hiding this comment.
🟢 MINOR · consensus 2/2
Empty/unconfigured targetPath yields a poor error path. If both androidProvider.SdkPath and DefaultAndroidSdkPath resolve to an empty (non-null) string, the dry-run prints a blank path (...tools at: ) and the live path throws an opaque ArgumentNullException (surfaced generically via HandleCommandException). Neither AndroidProvider.EnsureLatestSdkToolsAsync nor the sibling InstallSdkToolsAsync guards early.
Recommendation: Guard for null/empty targetPath up front with an actionable message (e.g. "No Android SDK path configured; run maui android install"), for example via ArgumentException.ThrowIfNullOrWhiteSpace(targetPath) in the provider.
Bump Xamarin.Android.Tools.AndroidSdk 1.0.189-preview.58 -> 99.61.4266-ci.main.2128 to pick up the new command-line tools resolver API, and consume it in the CLI: - SdkManager: add FindCommandLineTools() and EnsureLatestCommandLineToolsAsync() wrappers over the upstream FindSdkManager() / EnsureLatestCommandLineToolsAsync(). - AndroidProvider/IAndroidProvider: surface the resolved cmdline-tools revision + path in the `maui doctor` SDK Manager check, and add an EnsureLatestSdkToolsAsync passthrough returning the revision. - Add `maui android sdk update-tools` command (--json / --dry-run with live progress) that ensures cmdline-tools;latest. - Tests: 6 new (SdkManager resolver + command-level); 766 total green. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: c7183fc6-bcda-4988-8a9b-da5fd84d2e3c
The pinned Xamarin.Android.Tools.AndroidSdk 99.61.4269-preview.2131 is published to both dotnet11-transport and dotnet11-workloads. The transport feed is a rolling window that will eventually prune the build, and the stable dotnet11 feed carries no 99.x versions. Adding the workloads feed provides a durable, release-signed source for the pinned package. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: c7183fc6-bcda-4988-8a9b-da5fd84d2e3c
ae3ea94 to
652c477
Compare
What & why
Bumps
Xamarin.Android.Tools.AndroidSdk1.0.189-preview.58→99.61.4266-ci.main.2128to pick up the new command-line tools resolver API, and actually consumes it in themauiCLI.Changes
FindCommandLineTools()andEnsureLatestCommandLineToolsAsync()over the upstreamFindSdkManager()/EnsureLatestCommandLineToolsAsync().maui doctor— the SDK Manager check now surfaces the resolved cmdline-tools revision + path.maui android sdk update-tools(--json/--dry-run, live progress) ensurescmdline-tools;latest.EnsureLatestSdkToolsAsyncadded toIAndroidProvider/AndroidProvider(+ test fake).Notes / risks
99.61.4266-ci.main.2128is a rolling main-channel CI build served only from thedotnet11-transportfeed (transport feeds prune old builds; there is no Maestro subscription, so it's a manual pin). The prior.58pin was also transport-only, so this isn't a new dependency class — but it's less stable than a preview-channel version. Kept as draft pending a decision on this pin.Single commit, based directly on
main. Local validation:Microsoft.Maui.Clibuilds clean;Microsoft.Maui.Cli.UnitTests757/757 pass.