From 22dc57c04459f3702553eb156bd3b549184f6a13 Mon Sep 17 00:00:00 2001 From: Brandon Evans Date: Wed, 2 Oct 2019 20:30:14 -0600 Subject: [PATCH 1/2] Provide better error messages for underlying shell commands When shell commands failed a helpful error message wasn't always provided to the user. This change improves the specific XcodeInstaller.Error cases and their error messages. Not all Current.shell command failures are caught and turned into our own error type, so this also improves the fallback behaviour by printing output from Process.PMKError.execution, which is omitted by default. --- Sources/XcodesKit/XcodeInstaller.swift | 49 ++++++++++++++++++++--- Sources/xcodes/main.swift | 9 ++--- Tests/XcodesKitTests/XcodesKitTests.swift | 4 +- 3 files changed, 49 insertions(+), 13 deletions(-) diff --git a/Sources/XcodesKit/XcodeInstaller.swift b/Sources/XcodesKit/XcodeInstaller.swift index ef2d1d3..cb13734 100644 --- a/Sources/XcodesKit/XcodeInstaller.swift +++ b/Sources/XcodesKit/XcodeInstaller.swift @@ -8,11 +8,42 @@ public final class XcodeInstaller { static let XcodeTeamIdentifier = "59GAB85EFG" static let XcodeCertificateAuthority = ["Software Signing", "Apple Code Signing Certification Authority", "Apple Root CA"] - public enum Error: Swift.Error, Equatable { + public enum Error: LocalizedError, Equatable { case failedToMoveXcodeToApplications case failedSecurityAssessment(xcode: InstalledXcode, output: String) - case codesignVerifyFailed + case codesignVerifyFailed(output: String) + case unexpectedCodeSigningIdentity(identifier: String, certificateAuthority: [String]) case unsupportedFileFormat(extension: String) + + public var errorDescription: String? { + switch self { + case .failedToMoveXcodeToApplications: + return "Failed to move Xcode to the /Applications directory." + case .failedSecurityAssessment(let xcode, let output): + return """ + Xcode \(xcode.version) failed its security assessment with the following output: + \(output) + It remains installed at \(xcode.path) if you wish to use it anyways. + """ + case .codesignVerifyFailed(let output): + return """ + The downloaded Xcode failed code signing verification with the following output: + \(output) + """ + case .unexpectedCodeSigningIdentity(let identity, let certificateAuthority): + return """ + The downloaded Xcode doesn't have the expected code signing identity. + Got: + \(identity) + \(certificateAuthority) + Expected: + \(XcodeInstaller.XcodeTeamIdentifier) + \(XcodeInstaller.XcodeCertificateAuthority) + """ + case .unsupportedFileFormat(let fileExtension): + return "xcodes doesn't (yet) support installing Xcode from the \(fileExtension) file format." + } + } } public init() {} @@ -51,7 +82,7 @@ public final class XcodeInstaller { let destinationURL = Path.root.join("Applications").join("Xcode-\(xcode.version.descriptionWithoutBuildMetadata).app").url switch archiveURL.pathExtension { case "xip": - return try unarchiveAndMoveXIP(at: archiveURL, to: destinationURL).map { xcodeURL in + return unarchiveAndMoveXIP(at: archiveURL, to: destinationURL).map { xcodeURL in guard let path = Path(url: xcodeURL), Current.files.fileExists(atPath: path.string), @@ -90,7 +121,7 @@ public final class XcodeInstaller { } } - func unarchiveAndMoveXIP(at source: URL, to destination: URL) throws -> Promise { + func unarchiveAndMoveXIP(at source: URL, to destination: URL) -> Promise { return firstly { () -> Promise in return Current.shell.unxip(source) } @@ -122,7 +153,13 @@ public final class XcodeInstaller { func verifySigningCertificate(of url: URL) -> Promise { return Current.shell.codesignVerify(url) - .recover { _ -> Promise in throw Error.codesignVerifyFailed } + .recover { error -> Promise in + var output = "" + if case let Process.PMKError.execution(_, possibleOutput, possibleError) = error { + output = [possibleOutput, possibleError].compactMap { $0 }.joined(separator: "\n") + } + throw Error.codesignVerifyFailed(output: output) + } .map { output -> CertificateInfo in // codesign prints to stderr return self.parseCertificateInfo(output.err) @@ -131,7 +168,7 @@ public final class XcodeInstaller { guard cert.teamIdentifier == XcodeInstaller.XcodeTeamIdentifier, cert.authority == XcodeInstaller.XcodeCertificateAuthority - else { throw Error.codesignVerifyFailed } + else { throw Error.unexpectedCodeSigningIdentity(identifier: cert.teamIdentifier, certificateAuthority: cert.authority) } } } diff --git a/Sources/xcodes/main.swift b/Sources/xcodes/main.swift index ef5effb..860d03f 100644 --- a/Sources/xcodes/main.swift +++ b/Sources/xcodes/main.swift @@ -19,7 +19,7 @@ try? configuration.load() let xcodesUsername = "XCODES_USERNAME" let xcodesPassword = "XCODES_PASSWORD" -enum XcodesError: Swift.Error, LocalizedError { +enum XcodesError: LocalizedError { case missingUsernameOrPassword case missingSudoerPassword case invalidVersion(String) @@ -271,11 +271,10 @@ let install = Command(usage: "install ", flags: [urlFlag]) { flags, arg } .catch { error in switch error { - case XcodeInstaller.Error.failedSecurityAssessment(let xcode, let output): + case Process.PMKError.execution(let process, let standardOutput, let standardError): print(""" - Xcode \(xcode.version) failed its security assessment with the following output: - \(output) - It remains installed at \(xcode.path) if you wish to use it anyways. + Failed executing: `\(process)` (\(process.terminationStatus)) + \([standardOutput, standardError].compactMap { $0 }.joined(separator: "\n")) """) default: print(error.legibleLocalizedDescription) diff --git a/Tests/XcodesKitTests/XcodesKitTests.swift b/Tests/XcodesKitTests/XcodesKitTests.swift index 2266cc6..711a031 100644 --- a/Tests/XcodesKitTests/XcodesKitTests.swift +++ b/Tests/XcodesKitTests/XcodesKitTests.swift @@ -89,7 +89,7 @@ final class XcodesKitTests: XCTestCase { let xcode = Xcode(version: Version("0.0.0")!, url: URL(fileURLWithPath: "/"), filename: "mock") installer.installArchivedXcode(xcode, at: URL(fileURLWithPath: "/Xcode-0.0.0.xip"), archiveTrashed: { _ in }, passwordInput: { Promise.value("") }) - .catch { error in XCTAssertEqual(error as! XcodeInstaller.Error, XcodeInstaller.Error.codesignVerifyFailed) } + .catch { error in XCTAssertEqual(error as! XcodeInstaller.Error, XcodeInstaller.Error.codesignVerifyFailed(output: "")) } } func test_InstallArchivedXcode_VerifySigningCertificateDoesntMatch_Throws() { @@ -97,7 +97,7 @@ final class XcodesKitTests: XCTestCase { let xcode = Xcode(version: Version("0.0.0")!, url: URL(fileURLWithPath: "/"), filename: "mock") installer.installArchivedXcode(xcode, at: URL(fileURLWithPath: "/Xcode-0.0.0.xip"), archiveTrashed: { _ in }, passwordInput: { Promise.value("") }) - .catch { error in XCTAssertEqual(error as! XcodeInstaller.Error, XcodeInstaller.Error.codesignVerifyFailed) } + .catch { error in XCTAssertEqual(error as! XcodeInstaller.Error, XcodeInstaller.Error.unexpectedCodeSigningIdentity(identifier: "", certificateAuthority: [])) } } func test_InstallArchivedXcode_TrashesXIPWhenFinished() { From 3a94285aa8bb3135e9b7faa67db8b27f7876033c Mon Sep 17 00:00:00 2001 From: Brandon Evans Date: Wed, 2 Oct 2019 20:33:16 -0600 Subject: [PATCH 2/2] Use cauterize in tests to silence warnings --- Tests/XcodesKitTests/XcodesKitTests.swift | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/Tests/XcodesKitTests/XcodesKitTests.swift b/Tests/XcodesKitTests/XcodesKitTests.swift index 711a031..d4e7c3b 100644 --- a/Tests/XcodesKitTests/XcodesKitTests.swift +++ b/Tests/XcodesKitTests/XcodesKitTests.swift @@ -56,6 +56,7 @@ final class XcodesKitTests: XCTestCase { XCTAssertEqual(value, Path.applicationSupport.join("com.robotsandpencils.xcodes").join("Xcode-0.0.0.xip").url) XCTAssertNil(xcodeDownloadURL) } + .cauterize() } func test_DownloadOrUseExistingArchive_DownloadsArchive() { @@ -73,6 +74,7 @@ final class XcodesKitTests: XCTestCase { XCTAssertEqual(value, Path.applicationSupport.join("com.robotsandpencils.xcodes").join("Xcode-0.0.0.xip").url) XCTAssertEqual(xcodeDownloadURL, URL(string: "https://apple.com/xcode.xip")!) } + .cauterize() } func test_InstallArchivedXcode_SecurityAssessmentFails_Throws() { @@ -111,6 +113,7 @@ final class XcodesKitTests: XCTestCase { let xipURL = URL(fileURLWithPath: "/Xcode-0.0.0.xip") installer.installArchivedXcode(xcode, at: xipURL, archiveTrashed: { _ in }, passwordInput: { Promise.value("") }) .ensure { XCTAssertEqual(trashedItemAtURL, xipURL) } + .cauterize() } func test_UninstallXcode_TrashesXcode() { @@ -123,6 +126,7 @@ final class XcodesKitTests: XCTestCase { let installedXcode = InstalledXcode(path: Path("/Applications/Xcode-0.0.0.app")!)! installer.uninstallXcode(installedXcode) .ensure { XCTAssertEqual(trashedItemAtURL, installedXcode.path.url) } + .cauterize() } func test_VerifySecurityAssessment_Fails() { @@ -131,6 +135,7 @@ final class XcodesKitTests: XCTestCase { let installedXcode = InstalledXcode(path: Path("/Applications/Xcode-0.0.0.app")!)! installer.verifySecurityAssessment(of: installedXcode) .tap { result in XCTAssertFalse(result.isFulfilled) } + .cauterize() } func test_VerifySecurityAssessment_Succeeds() { @@ -139,6 +144,7 @@ final class XcodesKitTests: XCTestCase { let installedXcode = InstalledXcode(path: Path("/Applications/Xcode-0.0.0.app")!)! installer.verifySecurityAssessment(of: installedXcode) .tap { result in XCTAssertTrue(result.isFulfilled) } + .cauterize() } func test_MigrateApplicationSupport_NoSupportFiles() {