Conversation
Add RuntimeService for listing simulator runtimes and downloading platform runtimes via xcodebuild. Includes SimctlOutputParser for parsing xcrun simctl JSON output (shared with SimulatorService). Download pattern from ClientTools.Platform: xcodebuild -downloadPlatform. Runtime listing reuses SimctlOutputParser.ParseRuntimes with filtering by platform and availability. Adds System.Text.Json dependency for netstandard2.0 target. Closes #150 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add EnvironmentChecker that performs a comprehensive check of the Apple development environment by aggregating results from CommandLineTools, XcodeManager, and RuntimeService. Includes Xcode license validation (xcodebuild -license check) and first-launch support (xcodebuild -runFirstLaunch), patterns from ClientTools.Platform iOSSshCommandsExtensions. Also maps platform SDK directory names to friendly names (e.g. iPhoneOS -> iOS, XROS -> visionOS). Includes dependencies from PRs #156, #157, #159 which will merge cleanly when those PRs land first. Closes #148 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Address Sonnet 4.5 review finding. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR adds runtime management APIs to Xamarin.MacDev: a RuntimeService class for listing and downloading simulator platform runtimes, a SimctlOutputParser class for parsing xcrun simctl JSON output (shared with PR #158), and a System.Text.Json NuGet dependency for netstandard2.0.
Changes:
- New
SimctlOutputParser.cs: Static parser forxcrun simctl listJSON output — producesSimulatorDeviceInfoandSimulatorRuntimeInfomodel objects - New
RuntimeService.cs: High-level runtime management —List(),ListByPlatform(), andDownloadPlatform()viaxcrun simctlandxcodebuild -downloadPlatform - Updated
Xamarin.MacDev.csproj: AddsSystem.Text.Json9.0.4 conditionally fornetstandard2.0
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
Xamarin.MacDev/SimctlOutputParser.cs |
Pure JSON parser for simctl device and runtime listings; also exposes ParseCreateOutput for companion PR #158 |
Xamarin.MacDev/RuntimeService.cs |
Service wrapping xcrun simctl and xcodebuild -downloadPlatform for runtime listing/downloading |
Xamarin.MacDev/Xamarin.MacDev.csproj |
Adds System.Text.Json dependency for netstandard2.0 target |
| log.LogInfo ("Downloading {0} platform runtime via xcodebuild...", platform); | ||
|
|
||
| try { | ||
| var (exitCode, stdout, stderr) = ProcessUtils.Exec (xcodebuildPath, "-downloadPlatform", platform); |
There was a problem hiding this comment.
The stdout variable captured from ProcessUtils.Exec is never used. Since xcodebuild output during a download is for progress/diagnostic purposes only, the second element of the tuple should be discarded with _ (i.e., var (exitCode, _, stderr) = ProcessUtils.Exec(...)). Leaving an unused variable named stdout may cause a compiler warning and is misleading.
| var (exitCode, stdout, stderr) = ProcessUtils.Exec (xcodebuildPath, "-downloadPlatform", platform); | |
| var (exitCode, _, stderr) = ProcessUtils.Exec (xcodebuildPath, "-downloadPlatform", platform); |
| /// Lists runtimes for a specific platform (e.g. "iOS", "tvOS", "watchOS", "visionOS"). | ||
| /// </summary> | ||
| public List<SimulatorRuntimeInfo> ListByPlatform (string platform, bool availableOnly = false) | ||
| { |
There was a problem hiding this comment.
ListByPlatform takes a string platform parameter but performs no input validation. A null or empty platform would silently return an empty list rather than failing fast with a clear error. The project coding guidelines require input validation at public boundaries using ArgumentNullException.ThrowIfNull and string.IsNullOrWhiteSpace. This is inconsistent with DownloadPlatform, which does validate its platform parameter with string.IsNullOrEmpty.
| { | |
| { | |
| ArgumentNullException.ThrowIfNull (platform); | |
| if (string.IsNullOrWhiteSpace (platform)) | |
| throw new ArgumentException ("Platform must not be empty or whitespace.", nameof (platform)); |
| string? ResolveXcodebuild (string? xcodePath) | ||
| { | ||
| if (!string.IsNullOrEmpty (xcodePath)) { | ||
| var path = Path.Combine (xcodePath!, XcodebuildRelativePath); |
There was a problem hiding this comment.
The null-forgiving operator (!) on xcodePath! is unnecessary here. The !string.IsNullOrEmpty(xcodePath) check in the if guard already proves xcodePath is non-null within this block, so the compiler should not warn about a potential null dereference when calling Path.Combine. Removing the ! makes the code cleaner and avoids suppressing a safety check that doesn't actually apply here.
| var path = Path.Combine (xcodePath!, XcodebuildRelativePath); | |
| var path = Path.Combine (xcodePath, XcodebuildRelativePath); |
| public static List<SimulatorDeviceInfo> ParseDevices (string json) | ||
| { | ||
| var devices = new List<SimulatorDeviceInfo> (); | ||
| if (string.IsNullOrEmpty (json)) | ||
| return devices; | ||
|
|
||
| try { | ||
| using (var doc = JsonDocument.Parse (json, JsonOptions)) { | ||
| if (!doc.RootElement.TryGetProperty ("devices", out var devicesElement)) | ||
| return devices; | ||
|
|
||
| foreach (var runtimeProp in devicesElement.EnumerateObject ()) { | ||
| var runtimeId = runtimeProp.Name; | ||
|
|
||
| foreach (var device in runtimeProp.Value.EnumerateArray ()) { | ||
| var info = new SimulatorDeviceInfo { | ||
| RuntimeIdentifier = runtimeId, | ||
| Name = GetString (device, "name"), | ||
| Udid = GetString (device, "udid"), | ||
| State = GetString (device, "state"), | ||
| DeviceTypeIdentifier = GetString (device, "deviceTypeIdentifier"), | ||
| IsAvailable = GetBool (device, "isAvailable"), | ||
| }; | ||
|
|
||
| devices.Add (info); | ||
| } | ||
| } | ||
| } | ||
| } catch (JsonException) { | ||
| // Malformed simctl output — return whatever we parsed so far | ||
| } | ||
|
|
||
| return devices; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Parses the JSON output of <c>xcrun simctl list runtimes --json</c> | ||
| /// into a list of <see cref="SimulatorRuntimeInfo"/>. | ||
| /// </summary> | ||
| public static List<SimulatorRuntimeInfo> ParseRuntimes (string json) | ||
| { | ||
| var runtimes = new List<SimulatorRuntimeInfo> (); | ||
| if (string.IsNullOrEmpty (json)) | ||
| return runtimes; | ||
|
|
||
| try { | ||
| using (var doc = JsonDocument.Parse (json, JsonOptions)) { | ||
| if (!doc.RootElement.TryGetProperty ("runtimes", out var runtimesArray)) | ||
| return runtimes; | ||
|
|
||
| foreach (var rt in runtimesArray.EnumerateArray ()) { | ||
| var info = new SimulatorRuntimeInfo { | ||
| Name = GetString (rt, "name"), | ||
| Identifier = GetString (rt, "identifier"), | ||
| Version = GetString (rt, "version"), | ||
| BuildVersion = GetString (rt, "buildversion"), | ||
| Platform = GetString (rt, "platform"), | ||
| IsAvailable = GetBool (rt, "isAvailable"), | ||
| IsBundled = GetBool (rt, "isInternal"), | ||
| }; | ||
|
|
||
| runtimes.Add (info); | ||
| } | ||
| } | ||
| } catch (JsonException) { | ||
| // Malformed simctl output — return whatever we parsed so far | ||
| } | ||
|
|
||
| return runtimes; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Parses the UDID from the output of <c>xcrun simctl create</c>. | ||
| /// The command outputs just the UDID on a single line. | ||
| /// </summary> | ||
| public static string? ParseCreateOutput (string output) | ||
| { | ||
| if (string.IsNullOrEmpty (output)) | ||
| return null; | ||
|
|
||
| var udid = output.Trim (); | ||
| return udid.Length > 0 ? udid : null; |
There was a problem hiding this comment.
The SimctlOutputParser class introduces three public utility methods (ParseDevices, ParseRuntimes, ParseCreateOutput) with no test coverage in this PR. The custom coding guidelines for this project explicitly require test coverage for every new utility method. The companion PR #158 is described as having 16 tests for SimctlOutputParser, but those tests are not part of this PR. Test coverage for these parsing methods should be included here as well.
| BuildVersion = GetString (rt, "buildversion"), | ||
| Platform = GetString (rt, "platform"), | ||
| IsAvailable = GetBool (rt, "isAvailable"), | ||
| IsBundled = GetBool (rt, "isInternal"), |
There was a problem hiding this comment.
The IsBundled property is being populated from the isInternal JSON field of simctl output, but these two concepts are semantically different. The isInternal field marks Apple-internal/private (pre-release) runtime builds, not runtimes that are bundled with Xcode. Bundled runtimes in modern simctl output (Xcode 15+) are identified differently, typically via a "contentType" field with value "bundled". Mapping IsBundled to isInternal will incorrectly mark internal builds as "bundled" and fail to identify genuinely bundled runtimes.
| try { | ||
| var (exitCode, stdout, stderr) = ProcessUtils.Exec (xcodebuildPath, "-downloadPlatform", platform); | ||
| if (exitCode != 0) { | ||
| log.LogInfo ("xcodebuild -downloadPlatform {0} failed (exit {1}): {2}", platform, exitCode, stderr.Trim ()); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
DownloadPlatform uses ProcessUtils.Exec which is a synchronous, blocking wrapper that buffers all stdout and stderr in memory before returning. The issue specification for runtime install explicitly requires progress reporting for downloads that can be 5-10 GB large. Using ProcessUtils.Exec here means: no progress is reported to callers, and the entire xcodebuild output is buffered in memory for the full duration of the download. Also, the stdout variable captured at line 81 is never used. At minimum, the return value should be discard-documented or the variable removed.
| log.LogInfo ("xcodebuild -downloadPlatform {0} failed (exit {1}): {2}", platform, exitCode, stderr.Trim ()); | ||
| return false; | ||
| } | ||
|
|
||
| log.LogInfo ("Successfully downloaded {0} platform runtime.", platform); | ||
| return true; | ||
| } catch (System.ComponentModel.Win32Exception ex) { | ||
| log.LogInfo ("Could not run xcodebuild: {0}", ex.Message); | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Resolves the path to xcodebuild. If xcodePath is given, looks inside the Xcode bundle. | ||
| /// Otherwise falls back to /usr/bin/xcrun xcodebuild. | ||
| /// </summary> | ||
| string? ResolveXcodebuild (string? xcodePath) | ||
| { | ||
| if (!string.IsNullOrEmpty (xcodePath)) { | ||
| var path = Path.Combine (xcodePath!, XcodebuildRelativePath); | ||
| if (File.Exists (path)) | ||
| return path; | ||
| } | ||
|
|
||
| // Fall back to xcrun to find xcodebuild | ||
| if (File.Exists (XcrunPath)) { | ||
| try { | ||
| var (exitCode, stdout, _) = ProcessUtils.Exec (XcrunPath, "--find", "xcodebuild"); | ||
| if (exitCode == 0) { | ||
| var path = stdout.Trim (); | ||
| if (File.Exists (path)) | ||
| return path; | ||
| } | ||
| } catch (System.ComponentModel.Win32Exception) { | ||
| // fall through | ||
| } | ||
| } | ||
|
|
||
| return null; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Runs a simctl subcommand and returns stdout, or null on failure. | ||
| /// </summary> | ||
| string? RunSimctl (params string [] args) | ||
| { | ||
| if (!File.Exists (XcrunPath)) { | ||
| log.LogInfo ("xcrun not found at '{0}'.", XcrunPath); | ||
| return null; | ||
| } | ||
|
|
||
| var fullArgs = new string [args.Length + 1]; | ||
| fullArgs [0] = "simctl"; | ||
| Array.Copy (args, 0, fullArgs, 1, args.Length); | ||
|
|
||
| try { | ||
| var (exitCode, stdout, stderr) = ProcessUtils.Exec (XcrunPath, fullArgs); | ||
| if (exitCode != 0) { | ||
| log.LogInfo ("simctl {0} failed (exit {1}): {2}", args [0], exitCode, stderr.Trim ()); | ||
| return null; | ||
| } | ||
| return stdout; | ||
| } catch (System.ComponentModel.Win32Exception ex) { | ||
| log.LogInfo ("Could not run xcrun: {0}", ex.Message); | ||
| return null; |
There was a problem hiding this comment.
Multiple failure conditions in RuntimeService are logged at LogInfo level rather than LogError or LogWarning. This makes it difficult to distinguish successful informational messages from error conditions in the log output. The following messages represent actual failures and should be promoted to log.LogError or log.LogWarning:
"xcodebuild -downloadPlatform {0} failed (exit {1}): {2}"(line 83)"Could not run xcodebuild: {0}"(line 90)"xcrun not found at '{0}'."(line 130)"simctl {0} failed (exit {1}): {2}"(line 141)"Could not run xcrun: {0}"(line 146)
The codebase consistently uses LoggingService.LogError for error conditions (see AppleSdkSettings.cs, MonoTouchSdk.cs, XamMacSdk.cs).
| string? ResolveXcodebuild (string? xcodePath) | ||
| { | ||
| if (!string.IsNullOrEmpty (xcodePath)) { | ||
| var path = Path.Combine (xcodePath!, XcodebuildRelativePath); | ||
| if (File.Exists (path)) | ||
| return path; | ||
| } | ||
|
|
||
| // Fall back to xcrun to find xcodebuild | ||
| if (File.Exists (XcrunPath)) { | ||
| try { | ||
| var (exitCode, stdout, _) = ProcessUtils.Exec (XcrunPath, "--find", "xcodebuild"); | ||
| if (exitCode == 0) { | ||
| var path = stdout.Trim (); | ||
| if (File.Exists (path)) | ||
| return path; | ||
| } | ||
| } catch (System.ComponentModel.Win32Exception) { | ||
| // fall through | ||
| } | ||
| } |
There was a problem hiding this comment.
When xcodePath is explicitly provided but xcodebuild is not found at the expected relative path inside the bundle (Contents/Developer/usr/bin/xcodebuild), the method silently falls back to finding xcodebuild via xcrun. This may yield a different Xcode version than the caller intended. If the caller specifies xcodePath, it likely intends to use that specific Xcode installation. A warning log or early return with null would be more transparent here, rather than silently using a different Xcode.
Summary
Add
RuntimeServicefor listing simulator runtimes and downloading platform runtimes. IncludesSimctlOutputParser(shared with PR #158) for parsingxcrun simctlJSON output.Changes
New files
Xamarin.MacDev/RuntimeService.cs— Runtime management:List(),ListByPlatform(),DownloadPlatform()Xamarin.MacDev/SimctlOutputParser.cs— Shared parser (also in PR Add simulator management APIs #158, will merge cleanly)Modified files
Xamarin.MacDev/Xamarin.MacDev.csproj— System.Text.Json for netstandard2.0Design notes
DownloadPlatform()usesxcodebuild -downloadPlatform iOSpattern from ClientTools.PlatformRemoteSimulatorValidatorListByPlatform()filters by platform name (iOS, tvOS, watchOS, visionOS)ResolveXcodebuild()looks inside Xcode bundle first, falls back toxcrun --findCloses #150