From 12c437b45ff2a9f29e746aac8a8587352237968e Mon Sep 17 00:00:00 2001 From: Tony Eichelberger Date: Thu, 29 Oct 2020 16:02:54 -0500 Subject: [PATCH 1/4] SUS-7308 added some error support for interop with Objc so we can have a localized descripion and exitCode passed to NSError --- Sources/Subprocess/Errors.swift | 34 +++++++++++++++++- Sources/Subprocess/Shell.swift | 22 ++++++++---- Subprocess.xcodeproj/project.pbxproj | 8 ++--- ...hellTests.swift => ShellSystemTests.swift} | 2 +- Tests/UnitTests/ShellTests.swift | 36 ++++++++++++------- 5 files changed, 77 insertions(+), 25 deletions(-) rename Tests/SystemTests/{ShellTests.swift => ShellSystemTests.swift} (98%) diff --git a/Sources/Subprocess/Errors.swift b/Sources/Subprocess/Errors.swift index 187e99d..181f73a 100644 --- a/Sources/Subprocess/Errors.swift +++ b/Sources/Subprocess/Errors.swift @@ -31,7 +31,8 @@ import Foundation public enum SubprocessError: Error { /// The process completed with a non-zero exit code - case exitedWithNonZeroStatus(Int32) + /// and a custom error message. + case exitedWithNonZeroStatus(Int32, String) /// The property list object could not be cast to expected type case unexpectedPropertyListObject(String) @@ -45,3 +46,34 @@ public enum SubprocessError: Error { /// Output string could not be encoded case outputStringEncodingError } + +extension SubprocessError: LocalizedError { + public var errorDescription: String? { + switch self { + case .exitedWithNonZeroStatus(_, let errorMessage): + return "\(errorMessage)" + case .unexpectedPropertyListObject: + return "The property list object could not be cast to expected type" + case .unexpectedJSONObject: + return "The JSON object could not be cast to expected type" + case .inputStringEncodingError: + return "Input string could not be encoded" + case .outputStringEncodingError: + return "Output string could not be encoded" + } + } +} + +/// Gets us the NSError methods for when used by Objective-C +extension SubprocessError: CustomNSError { + public var errorCode: Int { + switch self { + case .exitedWithNonZeroStatus(let errorCode, _): + return Int(errorCode) + case .unexpectedPropertyListObject: return 10 + case .unexpectedJSONObject: return 20 + case .inputStringEncodingError: return 30 + case .outputStringEncodingError: return 40 + } + } +} diff --git a/Sources/Subprocess/Shell.swift b/Sources/Subprocess/Shell.swift index 625a114..59922b8 100644 --- a/Sources/Subprocess/Shell.swift +++ b/Sources/Subprocess/Shell.swift @@ -66,12 +66,17 @@ public class Shell { public func exec(input: Input? = nil, options: OutputOptions = .stdout, transformBlock: (_ process: Subprocess, _ data: Data) throws -> T) throws -> T { - var buffer = Data() + var stdoutBuffer = Data() + var stderrBuffer = Data() try process.launch(input: input, - outputHandler: options.contains(.stdout) ? { buffer.append($0) } : nil, - errorHandler: options.contains(.stderr) ? { buffer.append($0) } : nil) + outputHandler: options.contains(.stdout) ? { stdoutBuffer.append($0) } : nil, + errorHandler: options.contains(.stderr) ? { stderrBuffer.append($0) } : nil) process.waitForTermination() - return try transformBlock(process, buffer) + // doing this so we can consistently get stdout before stderr when using the combined option + var combinedBuffer = Data() + combinedBuffer.append(stdoutBuffer) + combinedBuffer.append(stderrBuffer) + return try transformBlock(process, combinedBuffer) } /// Executes shell command expecting exit code of zero and returning the output data @@ -84,7 +89,10 @@ public class Shell { public func exec(input: Input? = nil, options: OutputOptions = .stdout) throws -> Data { return try exec(input: input, options: options) { process, data in let exitCode = process.exitCode - guard exitCode == 0 else { throw SubprocessError.exitedWithNonZeroStatus(exitCode) } + guard exitCode == 0 else { + let message = String(data: data, encoding: .utf8) + throw SubprocessError.exitedWithNonZeroStatus(exitCode, message ?? "") + } return data } } @@ -124,7 +132,9 @@ public class Shell { encoding: String.Encoding) throws -> String { return try exec(input: input, options: options, encoding: encoding) { process, text in let exitCode = process.exitCode - guard exitCode == 0 else { throw SubprocessError.exitedWithNonZeroStatus(exitCode) } + guard exitCode == 0 else { + throw SubprocessError.exitedWithNonZeroStatus(exitCode, text) + } return text } } diff --git a/Subprocess.xcodeproj/project.pbxproj b/Subprocess.xcodeproj/project.pbxproj index 5b36e2e..9c03b88 100644 --- a/Subprocess.xcodeproj/project.pbxproj +++ b/Subprocess.xcodeproj/project.pbxproj @@ -20,7 +20,7 @@ 6EDDC6DA241037F500E171C6 /* MockSubprocessDependencyBuilder.swift in Sources */ = {isa = PBXBuildFile; fileRef = 6EDDC6AF2410378100E171C6 /* MockSubprocessDependencyBuilder.swift */; }; 6EDDC6DB241037F500E171C6 /* MockProcess.swift in Sources */ = {isa = PBXBuildFile; fileRef = 6EDDC6B12410378100E171C6 /* MockProcess.swift */; }; 6EDDC6E52410391500E171C6 /* Subprocess.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = 6EDDC6382410369400E171C6 /* Subprocess.framework */; }; - 6EDDC6F92410395800E171C6 /* ShellTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 6EDDC6B82410378100E171C6 /* ShellTests.swift */; }; + 6EDDC6F92410395800E171C6 /* ShellSystemTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 6EDDC6B82410378100E171C6 /* ShellSystemTests.swift */; }; 6EDDC6FB2410396000E171C6 /* SubprocessTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 6EDDC6BC2410378100E171C6 /* SubprocessTests.swift */; }; 6EDDC6FC2410396000E171C6 /* ShellTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 6EDDC6BD2410378100E171C6 /* ShellTests.swift */; }; 6EDDC6FD24105F8300E171C6 /* SubprocessMocks.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = 6EDDC6CB241037CE00E171C6 /* SubprocessMocks.framework */; }; @@ -61,7 +61,7 @@ 6EDDC6B02410378100E171C6 /* Info.plist */ = {isa = PBXFileReference; lastKnownFileType = text.plist.xml; path = Info.plist; sourceTree = ""; }; 6EDDC6B12410378100E171C6 /* MockProcess.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = MockProcess.swift; sourceTree = ""; }; 6EDDC6B22410378100E171C6 /* Subprocess.podspec */ = {isa = PBXFileReference; lastKnownFileType = text; path = Subprocess.podspec; sourceTree = ""; }; - 6EDDC6B82410378100E171C6 /* ShellTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = ShellTests.swift; sourceTree = ""; }; + 6EDDC6B82410378100E171C6 /* ShellSystemTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = ShellSystemTests.swift; sourceTree = ""; }; 6EDDC6B92410378100E171C6 /* Info.plist */ = {isa = PBXFileReference; lastKnownFileType = text.plist.xml; path = Info.plist; sourceTree = ""; }; 6EDDC6BC2410378100E171C6 /* SubprocessTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SubprocessTests.swift; sourceTree = ""; }; 6EDDC6BD2410378100E171C6 /* ShellTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = ShellTests.swift; sourceTree = ""; }; @@ -182,7 +182,7 @@ 6EDDC6B52410378100E171C6 /* SystemTests */ = { isa = PBXGroup; children = ( - 6EDDC6B82410378100E171C6 /* ShellTests.swift */, + 6EDDC6B82410378100E171C6 /* ShellSystemTests.swift */, 6EDDC6B92410378100E171C6 /* Info.plist */, ); path = SystemTests; @@ -452,7 +452,7 @@ isa = PBXSourcesBuildPhase; buildActionMask = 2147483647; files = ( - 6EDDC6F92410395800E171C6 /* ShellTests.swift in Sources */, + 6EDDC6F92410395800E171C6 /* ShellSystemTests.swift in Sources */, ); runOnlyForDeploymentPostprocessing = 0; }; diff --git a/Tests/SystemTests/ShellTests.swift b/Tests/SystemTests/ShellSystemTests.swift similarity index 98% rename from Tests/SystemTests/ShellTests.swift rename to Tests/SystemTests/ShellSystemTests.swift index f37f2a8..c3bb3b8 100644 --- a/Tests/SystemTests/ShellTests.swift +++ b/Tests/SystemTests/ShellSystemTests.swift @@ -1,7 +1,7 @@ import XCTest @testable import Subprocess -final class ShellTests: XCTestCase { +final class ShellSystemTests: XCTestCase { override func setUp() { SubprocessDependencyBuilder.shared = SubprocessDependencyBuilder() diff --git a/Tests/UnitTests/ShellTests.swift b/Tests/UnitTests/ShellTests.swift index 9c14479..2f7b177 100644 --- a/Tests/UnitTests/ShellTests.swift +++ b/Tests/UnitTests/ShellTests.swift @@ -28,14 +28,21 @@ final class ShellTests: XCTestCase { func testExecReturningDataWhenExitCodeIsNoneZero() { // Given let exitCode = Int32.random(in: 1...Int32.max) - Shell.expect(command, input: nil, standardError: Data(), exitCode: exitCode) + let stdoutData = "stdout example".data(using: .utf8) + let stderrData = "stderr example".data(using: .utf8) + Shell.expect(command, input: nil, standardOutput: stdoutData, standardError: stderrData, exitCode: exitCode) // When - XCTAssertThrowsError(_ = try Shell(command).exec()) { - switch ($0 as? SubprocessError) { - case .exitedWithNonZeroStatus(let status): + XCTAssertThrowsError(_ = try Shell(command).exec()) { error in + switch (error as? SubprocessError) { + case .exitedWithNonZeroStatus(let status, let errorMessage): XCTAssertEqual(status, exitCode) - default: XCTFail("Unexpected error type: \($0)") + let failMsg = "error message should have contained the results from only stdout but was \(errorMessage)" + XCTAssertTrue(errorMessage.contains("stdout example"), failMsg) + XCTAssertEqual(errorMessage, error.localizedDescription, "should have also set localizedDescription") + let nsError = error as NSError + XCTAssertEqual(Int(status), nsError.code, "should have also set the NSError exit code") + default: XCTFail("Unexpected error type: \(error)") } } @@ -92,8 +99,7 @@ final class ShellTests: XCTestCase { // Then let text = String(data: result, encoding: .utf8) - XCTAssertTrue(text?.contains(expectedStdout) ?? false) - XCTAssertTrue(text?.contains(expectedStderr) ?? false) + XCTAssertEqual("\(expectedStdout)\(expectedStderr)", text, "should have combined stdout and stderror") Shell.verify { XCTFail($0.message, file: $0.file, line: $0.line) } } @@ -102,14 +108,18 @@ final class ShellTests: XCTestCase { func testExecReturningStringWhenExitCodeIsNoneZero() { // Given let exitCode = Int32.random(in: 1...Int32.max) - Shell.expect(command, input: nil, stderr: "Some error occured", exitCode: exitCode) - + let stdoutText = "should not show up" + let stderrText = "should show up" + Shell.expect(command, input: nil, stdout: stdoutText, stderr: stderrText, exitCode: exitCode) // When - XCTAssertThrowsError(_ = try Shell(command).exec(encoding: .utf8)) { - switch ($0 as? SubprocessError) { - case .exitedWithNonZeroStatus(let status): + XCTAssertThrowsError(_ = try Shell(command).exec(options: .stderr, encoding: .utf8)) { error in + switch (error as? SubprocessError) { + case .exitedWithNonZeroStatus(let status, let errorMessage): XCTAssertEqual(status, exitCode) - default: XCTFail("Unexpected error type: \($0)") + let failMsg = "should have put just stderr in the error: \(errorMessage)" + XCTAssertEqual("should show up", errorMessage, failMsg) + XCTAssertEqual(errorMessage, error.localizedDescription, "also should have set localizedDescription") + default: XCTFail("Unexpected error type: \(error)") } } From 332a014d7de54cc1d4e50d923372e1912e276b41 Mon Sep 17 00:00:00 2001 From: Tony Eichelberger Date: Mon, 2 Nov 2020 16:00:44 -0600 Subject: [PATCH 2/4] Update Sources/Subprocess/Errors.swift Co-authored-by: Kyle Hammond --- Sources/Subprocess/Errors.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Sources/Subprocess/Errors.swift b/Sources/Subprocess/Errors.swift index 181f73a..0b85003 100644 --- a/Sources/Subprocess/Errors.swift +++ b/Sources/Subprocess/Errors.swift @@ -64,7 +64,7 @@ extension SubprocessError: LocalizedError { } } -/// Gets us the NSError methods for when used by Objective-C +/// Common NSError methods for better interop with Objective-C extension SubprocessError: CustomNSError { public var errorCode: Int { switch self { From 4f27160de776c936ea47933732f00182e7a61283 Mon Sep 17 00:00:00 2001 From: Tony Eichelberger Date: Tue, 3 Nov 2020 14:32:05 -0600 Subject: [PATCH 3/4] Added a changelog and bumped the version to 2.0.0 for whenever we decide to release the next version --- CHANGELOG.md | 33 ++++++++++++++++++++++++++++++ Sources/Subprocess/Info.plist | 2 +- Sources/SubprocessMocks/Info.plist | 2 +- Subprocess.podspec | 2 +- 4 files changed, 36 insertions(+), 3 deletions(-) create mode 100644 CHANGELOG.md diff --git a/CHANGELOG.md b/CHANGELOG.md new file mode 100644 index 0000000..ae53580 --- /dev/null +++ b/CHANGELOG.md @@ -0,0 +1,33 @@ +## Subprocess + +Subprocess is a Swift library for macOS providing interfaces for both synchronous and asynchronous process execution. + +All notable changes to this project will be documented in this file. + +The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), +and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). + +## [Unreleased] + +### Changed +- Breaking: added the output of the command to Shell's exception exitedWithNonZeroStatus error to better conform to objc interop and NSError + + +## 1.1.0 - 2020-05-15 + +### Added +- Added dynamic library targets to SPM + +### Fixed +- Fixed naming convention to match Jamf internal convention + +## 1.0.1 - 2020-03-13 + +### Added +- Added Cocoapods support + + +## 1.0.0 - 2020-03-13 + +### Added +- All support for the initial release diff --git a/Sources/Subprocess/Info.plist b/Sources/Subprocess/Info.plist index f741ff7..7c96b9f 100644 --- a/Sources/Subprocess/Info.plist +++ b/Sources/Subprocess/Info.plist @@ -15,7 +15,7 @@ CFBundlePackageType $(PRODUCT_BUNDLE_PACKAGE_TYPE) CFBundleShortVersionString - 1.0.1 + 2.0.0 CFBundleVersion $(CURRENT_PROJECT_VERSION) diff --git a/Sources/SubprocessMocks/Info.plist b/Sources/SubprocessMocks/Info.plist index f741ff7..7c96b9f 100644 --- a/Sources/SubprocessMocks/Info.plist +++ b/Sources/SubprocessMocks/Info.plist @@ -15,7 +15,7 @@ CFBundlePackageType $(PRODUCT_BUNDLE_PACKAGE_TYPE) CFBundleShortVersionString - 1.0.1 + 2.0.0 CFBundleVersion $(CURRENT_PROJECT_VERSION) diff --git a/Subprocess.podspec b/Subprocess.podspec index 9b340bd..ca7db4e 100644 --- a/Subprocess.podspec +++ b/Subprocess.podspec @@ -1,6 +1,6 @@ Pod::Spec.new do |s| s.name = 'Subprocess' - s.version = '1.0.1' + s.version = '2.0.0' s.summary = 'Wrapper for NSTask used for running processes and shell commands on macOS.' s.license = { :type => 'MIT', :text => "" } s.description = <<-DESC From 7e16471dbbe079ee6fde0031e410403a3de210f1 Mon Sep 17 00:00:00 2001 From: Tony Eichelberger Date: Tue, 3 Nov 2020 15:00:21 -0600 Subject: [PATCH 4/4] Added some comments explaining why we are not using the String param in the error messages --- Sources/Subprocess/Errors.swift | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/Sources/Subprocess/Errors.swift b/Sources/Subprocess/Errors.swift index 0b85003..b4dadf3 100644 --- a/Sources/Subprocess/Errors.swift +++ b/Sources/Subprocess/Errors.swift @@ -52,9 +52,11 @@ extension SubprocessError: LocalizedError { switch self { case .exitedWithNonZeroStatus(_, let errorMessage): return "\(errorMessage)" - case .unexpectedPropertyListObject: + case .unexpectedPropertyListObject(_): + // Ignoring the plist contents parameter as we don't want that in the error message return "The property list object could not be cast to expected type" - case .unexpectedJSONObject: + case .unexpectedJSONObject(_): + // Ignoring the json contents parameter as we don't want that in the error message return "The JSON object could not be cast to expected type" case .inputStringEncodingError: return "Input string could not be encoded"