Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent keyNotFound error with unknown simulators #261

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Prevent keyNotFound error with unknown simulators #261

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7aa5a38
Prevent keyNotFound error with unknown simulators
dnicolson Jan 4, 2023
9fb309c
Remove unusable runtimes from output
dnicolson Jan 6, 2023
01be5cf
Add comments for unusable runtime attributes
dnicolson Jan 6, 2023
dbb6855
Add test for unusable runtimes
dnicolson Jan 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions Sources/XcodesKit/Models+Runtimes.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,18 +108,18 @@ extension DownloadableRuntime {
}

public struct InstalledRuntime: Decodable {
let build: String
let build: String?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#picky can we add a comment on why these are optional

let deletable: Bool
let identifier: UUID
let kind: Kind
let lastUsedAt: Date?
let path: String
let platformIdentifier: Platform
let runtimeBundlePath: String
let runtimeIdentifier: String
let platformIdentifier: Platform?
let runtimeBundlePath: String?
let runtimeIdentifier: String?
let signatureState: String
let state: String
let version: String
let version: String?
let sizeBytes: Int?
}

Expand Down
15 changes: 8 additions & 7 deletions Sources/XcodesKit/RuntimeInstaller.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,18 +38,16 @@ public class RuntimeInstaller {
}
}




let unusableCount = installed.removeAll { $0.state == "Unusable" }.count
installed.forEach { runtime in
let resolvedBetaNumber = downloadablesResponse.sdkToSeedMappings.first {
$0.buildUpdate == runtime.build
$0.buildUpdate == runtime.build!
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we refactor this section a bit?

Can we not use force unwrap. As a rule, I don't use them. Totally agree that it should never happen but it's bitten me in the past. Perhaps making an overloaded PrintableRuntime() to allow optionals?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how best to achieve this, feel free to make any changes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just my 2 cents here, personally I also try to avoid them if I can, but in this case, I would prefer a force unwrap to fail than hiding the error and providing an empty string as a default value, in that case, we would know that xcrun simctl runtime list printed something that we didn't account for, and there is probably a new state of a runtime that we should write code for. I see it as a assert/precondition and not a code smell.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MattKiazyk can you please give an update on how you would like to proceed here?

}?.seedNumber

var result = PrintableRuntime(platform: runtime.platformIdentifier.asPlatformOS,
var result = PrintableRuntime(platform: runtime.platformIdentifier!.asPlatformOS,
betaNumber: resolvedBetaNumber,
version: runtime.version,
build: runtime.build,
version: runtime.version!,
build: runtime.build!,
state: runtime.kind)

mappedRuntimes.indices {
Expand Down Expand Up @@ -90,6 +88,9 @@ public class RuntimeInstaller {
}
}
Current.logging.log("\nNote: Bundled runtimes are indicated for the currently selected Xcode, more bundled runtimes may exist in other Xcode(s)")
if unusableCount > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this vs having the original Unknown 👍

Current.logging.log("Note: Found \(unusableCount) unknown downloaded runtime(s). For more info run `xcrun simctl runtime list -j`.")
}
}

public func downloadAndInstallRuntime(identifier: String, to destinationDirectory: Path, with downloader: Downloader, shouldDelete: Bool) async throws {
Expand Down