Refactor AppleProvider to delegate to EnvironmentChecker and add install command#196
Conversation
There was a problem hiding this comment.
Pull request overview
Refactors the CLI Apple provider to rely on the upstream Xamarin.Apple.Tools.MaciOS environment-check/install APIs, and adds a new maui apple install command to bootstrap Apple tooling on macOS.
Changes:
- Delegate Apple health checks to
EnvironmentChecker.Check()and add additional checks (Xcode license, SDK platforms). - Add
maui apple installcommand that wrapsAppleInstaller.Install()with--platformand--dry-run. - Introduce
AppleInstallResultand register it for JSON source generation; update fakes and bump Apple tools dependency version.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Cli/Microsoft.Maui.Cli/Providers/Apple/IAppleProvider.cs | Adds InstallEnvironmentAsync and introduces AppleInstallResult. |
| src/Cli/Microsoft.Maui.Cli/Providers/Apple/AppleProvider.cs | Delegates health checks to EnvironmentChecker and implements install via AppleInstaller. |
| src/Cli/Microsoft.Maui.Cli/Output/MauiCliJsonContext.cs | Registers AppleInstallResult for source-generated JSON serialization. |
| src/Cli/Microsoft.Maui.Cli/Errors/ErrorCodes.cs | Adds new Apple error codes E2205 and E2206. |
| src/Cli/Microsoft.Maui.Cli/Commands/AppleCommands.cs | Adds the new maui apple install subcommand and wiring. |
| src/Cli/Microsoft.Maui.Cli.UnitTests/Fakes/FakeAppleProvider.cs | Extends fake provider to support InstallEnvironmentAsync for tests. |
| eng/Versions.props | Bumps Xamarin.Apple.Tools.MaciOS version. |
| eng/Version.Details.xml | Updates dependency version/SHA for darc/maestro tracking. |
Expert Code Review — PR #196Methodology: 3 independent reviewers with adversarial consensus 6 findings posted as inline comments (1 🔴 critical, 3 🟡 moderate, 2 🟢 minor)
Discarded Findings (single reviewer, no consensus)
CI Status✅ macOS build passed · ✅ Ubuntu build passed · ✅ Windows (CLI) build passed · ⏳ Windows (DevFlow) still running Test CoverageThe
|
There was a problem hiding this comment.
Expert Code Review Summary
Reviewed for regressions, bugs, security, and data-loss risks. No critical issues found. Two moderate and three minor findings identified.
🟡 MODERATE
- Missing try-catch in install command (
AppleCommands.cs:151-197) — Every other CLI command usestry/catch → HandleCommandException. This one doesn't, risking raw stack traces on failure. E2206error code declared but unused (ErrorCodes.cs:46) — Install failures return exit code 1 without a structured error ID, reducing automation clarity.
🟢 MINOR
- Task.Run cancellation is superficial (
AppleProvider.cs:372-387) —CancellationTokenis only checked before/after the synchronousInstall()call; Ctrl+C is unresponsive during actual work. - License check called when Xcode is absent (
AppleProvider.cs:191) — Wastes an exception throw/catch cycle. Could skip whenresult.Xcode is null. - Direct list reference sharing (
AppleProvider.cs:383) —Platformsassigned by reference from upstream; should use.ToList()for defensive copying (matchingRuntimeson the next line).
Generated by Expert Code Review (auto) for issue #196 · ● 14.1M
…all command - Delegate CheckHealth() to Xamarin.MacDev.EnvironmentChecker.Check() instead of manually querying XcodeManager/CommandLineTools/RuntimeService separately - Add Xcode license acceptance check (new E2205 error code) - Add SDK Platforms health check showing discovered platform SDKs - Upgrade CLT-missing severity from Warning to Error (blocks development) - Add 'maui apple install' command wrapping AppleInstaller.Install() for one-command environment bootstrapping (consistent with 'maui android install') - Add --platform and --dry-run options to the install command - Register AppleInstallResult in JSON source generator context - Update FakeAppleProvider with InstallEnvironmentAsync support Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Remove local --dry-run option; use GlobalOptions.DryRunOption (finding #1) - Return exit code 0 for 'skipped' status on non-macOS (finding #2) - Wrap install action in try-catch using E2206 error code (finding #3) - Replace null-forgiving with null-conditional on license check (finding #4) - Defensive copy Platforms list with .ToList() (finding #5) - Clarify cancellation limitation comment (finding #6) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
0c18830 to
0a3064a
Compare
There was a problem hiding this comment.
Pull request overview
Refactors the CLI’s Apple provider to rely on the upstream Xamarin.Apple.Tools.MaciOS environment checking/install APIs and adds a new maui apple install command to bootstrap the Apple dev environment.
Changes:
- Delegate Apple health checks to
EnvironmentChecker, adding Xcode license + SDK platform reporting and adjusting CLT severity. - Add
maui apple installcommand backed byAppleInstaller.Install()with--platformselection and JSON output support. - Bump
Xamarin.Apple.Tools.MaciOSdependency version and add new Apple error codes + JSON model registration.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Cli/Microsoft.Maui.Cli/Providers/Apple/IAppleProvider.cs | Adds InstallEnvironmentAsync API and introduces AppleInstallResult DTO. |
| src/Cli/Microsoft.Maui.Cli/Providers/Apple/AppleProvider.cs | Refactors CheckHealth() to EnvironmentChecker and implements install delegation via AppleInstaller. |
| src/Cli/Microsoft.Maui.Cli/Output/MauiCliJsonContext.cs | Registers AppleInstallResult for JSON source generation. |
| src/Cli/Microsoft.Maui.Cli/Errors/ErrorCodes.cs | Adds new Apple-related error codes for license/install failures. |
| src/Cli/Microsoft.Maui.Cli/Commands/AppleCommands.cs | Adds the apple install subcommand and wires output/exit codes. |
| src/Cli/Microsoft.Maui.Cli.UnitTests/Fakes/FakeAppleProvider.cs | Extends the fake provider to support install calls for tests. |
| eng/Versions.props | Updates the Xamarin.Apple.Tools.MaciOS version property. |
| eng/Version.Details.xml | Updates dependency flow metadata (version + SHA) for Xamarin.Apple.Tools.MaciOS. |
Tests cover: - Command structure (install exists, --platform option, no local --dry-run) - Option parsing (multiple --platform values) - Handler invocation via FakeAppleProvider - Platform filter passthrough - Global --dry-run option propagation - Exit code 0 for 'ok' and 'skipped' status - Exit code 1 for 'failed' status Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Fix <returns> doc to describe AppleInstallResult (not 'environment check') - Fix XcodeVersion doc comment to say 'version and build number' (not path) - Return exit code 1 on non-macOS for consistent signaling - Default --platform to iOS; add 'all' option to install all runtimes - Add test for --platform all passing null filter to provider Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Create eng/smoke-tests/apple-cli-smoke-test.sh with 7 automated checks: xcode list, runtime list, simulator list, start/stop simulator, install dry-run (default iOS), install dry-run (all platforms) - Update copilot-instructions.md to run smoke tests after Apple provider changes or Xamarin.Apple.Tools.MaciOS version updates on macOS Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Add --no-open flag to suppress UI launch (for CI/headless usage) - Add OpenSimulatorApp() to IAppleProvider interface - Implement via 'open -a Simulator' in AppleProvider - Default behavior: boot device + open Simulator.app window Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The install command handler returns exit code 1 on non-macOS platforms (by design). Skip the handler-invocation tests when not running on macOS since they test provider interaction, not the platform guard. Command structure tests (Exists, HasPlatformOption, ParsesPlatformOption, ParsesPlatformDefault) remain cross-platform. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Adds two new subcommands to `maui apple simulator`: - `create <device-type> [--name NAME] [--runtime RUNTIME]` - `erase <name-or-udid>` Both wrap `Xamarin.MacDev.SimulatorService.Create()` / `Erase()` APIs. New error codes E2207/E2208 and JSON output models added. Renumbered from E2205/E2206 after rebase onto main, since #196 already took those codes for AppleXcodeLicenseNotAccepted / AppleSetupFailed. Closes #197 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Expose maui apple simulator create|erase Adds two new subcommands to `maui apple simulator`: - `create <device-type> [--name NAME] [--runtime RUNTIME]` - `erase <name-or-udid>` Both wrap `Xamarin.MacDev.SimulatorService.Create()` / `Erase()` APIs. New error codes E2207/E2208 and JSON output models added. Renumbered from E2205/E2206 after rebase onto main, since #196 already took those codes for AppleXcodeLicenseNotAccepted / AppleSetupFailed. Closes #197 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Address review feedback on simulator create/erase commands - Use MauiToolException(PlatformNotSupported) for non-macOS instead of plain WriteWarning so JSON output includes error.code/category - Use formatter.WriteError(ex) consistently for both JSON and non-JSON error paths instead of WriteWarning(ex.Message) - Add shutdown hint to erase failure message for actionable guidance - Use string.IsNullOrWhiteSpace for --name option to handle empty strings - Apply dash-to-space replacement on runtime segment for consistent names - Switch to short attribute names in JsonOutputModels (file already imports System.Text.Json.Serialization) - Add default interface implementation for EraseSimulator to avoid breaking external implementors Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * apple sim: idempotency probe + actionable erase diagnostics Addresses re-review on PR #203: - maui apple simulator create now probes GetSimulators() for an existing device with the same name BEFORE invoking simctl create. simctl does not dedupe by name; without this guard repeated invocations create multiple devices with the same name and subsequent name-keyed commands (boot/erase/delete) become ambiguous. - New --if-not-exists flag returns the existing UDID as success when the name collides, for idempotent script use. - maui apple simulator erase now probes GetSimulators() first to distinguish 'not found' (E2204 AppleSimulatorNotFound) from 'simulator is booted' (E2208 AppleSimulatorEraseFailed with shutdown guidance) rather than emitting a single generic 'failed to erase' message. Both branches include the resolved UDID in the error message. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Address second round of review feedback - Fix runtime version mangling: 'iOS-17-2' now becomes 'iOS 17.2' instead of 'iOS 17 2' by replacing first dash with space and remaining dashes with dots - Revert EraseSimulator default interface implementation back to abstract to match sibling methods and avoid silent failure risk - Add doc comment on SimulatorEraseResult clarifying Erased is always true on the success path Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: GitHub Copilot <copilot@github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Summary
Leverages new APIs from
Xamarin.Apple.Tools.MaciOSv1.0.0-preview.1.26228.1 (dotnet/macios-devtools) to simplify and enhance the Apple provider.Changes
Health Checks (CheckHealth)
EnvironmentChecker.Check()— removes manual Xcode/CLT/runtime querying that duplicated upstream logicE2205error code, callsIsXcodeLicenseAccepted()Error(wasWarning) since it blocks developmentNew Command:
maui apple installAppleInstaller.Install()for one-command environment bootstrappingmaui android installnaming--platform iOS tvOS(multi-value),--dry-runSupporting Changes
E2205(license),E2206(install failed)AppleInstallResultrecord + JSON source gen registrationFakeAppleProviderupdated withInstallEnvironmentAsyncsupportTesting
maui doctor --json,maui apple install --dry-run,maui apple xcode list