Improve Destination.sdkPlatformFrameworkPaths()

We should not ignore errors or an empty SDK platform path since that means XCTest imports and test execution might silently fail on macOS with no indication to what the problem is. I am speculating that this may be contributing to the recent surge of tests failing by not seeing the XCTest Swift library. I am also re-enabling the disabled tests here to see if this has any practical effect, but regardless it doesn't seem reasonable to silently ignore some failure states here.

rdar://101868275

Author: neonichu

Sources/Commands/Utilities/TestingSupport.swift

@@ -82,13 +82,13 @@ enum TestingSupport {
                 ),
                 sanitizers: sanitizers
             )
-            // Add the sdk platform path if we have it. If this is not present, we
-            // might always end up failing.
-            if let sdkPlatformFrameworksPath = try Destination.sdkPlatformFrameworkPaths() {
-                // appending since we prefer the user setting (if set) to the one we inject
-                env.appendPath("DYLD_FRAMEWORK_PATH", value: sdkPlatformFrameworksPath.fwk.pathString)
-                env.appendPath("DYLD_LIBRARY_PATH", value: sdkPlatformFrameworksPath.lib.pathString)
-            }
+
+            // Add the sdk platform path if we have it. If this is not present, we might always end up failing.
+            let sdkPlatformFrameworksPath = try Destination.sdkPlatformFrameworkPaths()
+            // appending since we prefer the user setting (if set) to the one we inject
+            env.appendPath("DYLD_FRAMEWORK_PATH", value: sdkPlatformFrameworksPath.fwk.pathString)
+            env.appendPath("DYLD_LIBRARY_PATH", value: sdkPlatformFrameworksPath.lib.pathString)
+
             try TSCBasic.Process.checkNonZeroExit(arguments: args, environment: env)
             // Read the temporary file's content.
             return try swiftTool.fileSystem.readFileContents(tempFile.path)

Sources/PackageLoading/ManifestJSONParser.swift

@@ -165,7 +165,7 @@ enum ManifestJSONParser {
         do {
             path = try AbsolutePath(validating: location)
         } catch PathValidationError.invalidAbsolutePath(let path) {
-            throw ManifestParseError.invalidManifestFormat("'\(path)' is not a valid path for path-based dependencies; use relative or absolute path instead.", diagnosticFile: nil)
+            throw ManifestParseError.invalidManifestFormat("'\(path)' is not a valid path for path-based dependencies; use relative or absolute path instead.", diagnosticFile: nil, compilerCommandLine: nil)
         }
         let identity = try identityResolver.resolveIdentity(for: path)
         return .fileSystem(identity: identity,
@@ -224,11 +224,11 @@ enum ManifestJSONParser {
             guard hostnameComponent.isEmpty else {
               if hostnameComponent == ".." {
                 throw ManifestParseError.invalidManifestFormat(
-                  "file:// URLs cannot be relative, did you mean to use '.package(path:)'?", diagnosticFile: nil
+                  "file:// URLs cannot be relative, did you mean to use '.package(path:)'?", diagnosticFile: nil, compilerCommandLine: nil
                 )
               }
               throw ManifestParseError.invalidManifestFormat(
-                "file:// URLs with hostnames are not supported, are you missing a '/'?", diagnosticFile: nil
+                "file:// URLs with hostnames are not supported, are you missing a '/'?", diagnosticFile: nil, compilerCommandLine: nil
               )
             }
             return try AbsolutePath(validating: location).pathString

Sources/PackageLoading/ManifestLoader.swift

@@ -22,6 +22,8 @@ public enum ManifestParseError: Swift.Error, Equatable {
     /// The manifest contains invalid format.
     case invalidManifestFormat(String, diagnosticFile: AbsolutePath?)
     // TODO: Test this error.
+    case invalidManifestFormat(String, diagnosticFile: AbsolutePath?, compilerCommandLine: [String]?)
+
     /// The manifest was successfully loaded by swift interpreter but there were runtime issues.
     case runtimeManifestErrors([String])
 }
@@ -32,8 +34,14 @@ extension ManifestParseError: CustomStringConvertible {
         switch self {
         case .emptyManifest(let manifestPath):
             return "'\(manifestPath)' is empty"
-        case .invalidManifestFormat(let error, _):
-            return "invalid manifest\n\(error)"
+        case .invalidManifestFormat(let error, _, let compilerCommandLine):
+            let suffix: String
+            if let compilerCommandLine = compilerCommandLine {
+                suffix = " (compiled with: \(compilerCommandLine))"
+            } else {
+                suffix = ""
+            }
+            return "Invalid manifest\(suffix)\n\(error)"
         case .runtimeManifestErrors(let errors):
             return "invalid manifest (evaluation failed)\n\(errors.joined(separator: "\n"))"
         }
@@ -283,7 +291,7 @@ public final class ManifestLoader: ManifestLoaderProtocol {
         // Throw now if we weren't able to parse the manifest.
         guard let manifestJSON = result.manifestJSON, !manifestJSON.isEmpty else {
             let errors = result.errorOutput ?? result.compilerOutput ?? "Missing or empty JSON output from manifest compilation for \(packageIdentity)"
-            throw ManifestParseError.invalidManifestFormat(errors, diagnosticFile: result.diagnosticFile)
+            throw ManifestParseError.invalidManifestFormat(errors, diagnosticFile: result.diagnosticFile, compilerCommandLine: result.compilerCommandLine)
         }
 
         // We should not have any fatal error at this point.
@@ -583,6 +591,8 @@ public final class ManifestLoader: ManifestLoaderProtocol {
                     let compiledManifestFile = tmpDir.appending(component: "\(packageIdentity)-manifest\(executableSuffix)")
                     cmd += ["-o", compiledManifestFile.pathString]
 
+                    evaluationResult.compilerCommandLine = cmd
+
                     // Compile the manifest.
                     TSCBasic.Process.popen(arguments: cmd, environment: self.toolchain.swiftCompilerEnvironment, queue: callbackQueue) { result in
                         dispatchPrecondition(condition: .onQueue(callbackQueue))
@@ -821,6 +831,9 @@ extension ManifestLoader {
         /// The manifest in JSON format.
         var manifestJSON: String?
 
+        /// The command line used to compile the manifest
+        var compilerCommandLine: [String]?
+
         /// Any non-compiler error that might have occurred during manifest loading.
         ///
         /// For e.g., we could have failed to spawn the process or create temporary file.

Sources/PackageModel/Destination.swift

@@ -195,12 +195,11 @@ public struct Destination: Encodable, Equatable {
         var extraCCFlags: [String] = []
         var extraSwiftCFlags: [String] = []
 #if os(macOS)
-        if let sdkPaths = try Destination.sdkPlatformFrameworkPaths(environment: environment) {
-            extraCCFlags += ["-F", sdkPaths.fwk.pathString]
-            extraSwiftCFlags += ["-F", sdkPaths.fwk.pathString]
-            extraSwiftCFlags += ["-I", sdkPaths.lib.pathString]
-            extraSwiftCFlags += ["-L", sdkPaths.lib.pathString]
-        }
+        let sdkPaths = try Destination.sdkPlatformFrameworkPaths(environment: environment)
+        extraCCFlags += ["-F", sdkPaths.fwk.pathString]
+        extraSwiftCFlags += ["-F", sdkPaths.fwk.pathString]
+        extraSwiftCFlags += ["-I", sdkPaths.lib.pathString]
+        extraSwiftCFlags += ["-L", sdkPaths.lib.pathString]
 #endif
 
 #if !os(Windows)
@@ -217,26 +216,29 @@ public struct Destination: Encodable, Equatable {
     /// Returns macosx sdk platform framework path.
     public static func sdkPlatformFrameworkPaths(
         environment: EnvironmentVariables = .process()
-    ) throws -> (fwk: AbsolutePath, lib: AbsolutePath)? {
+    ) throws -> (fwk: AbsolutePath, lib: AbsolutePath) {
         if let path = _sdkPlatformFrameworkPath {
             return path
         }
-        let platformPath = try? TSCBasic.Process.checkNonZeroExit(
+        let platformPath = try TSCBasic.Process.checkNonZeroExit(
             arguments: ["/usr/bin/xcrun", "--sdk", "macosx", "--show-sdk-platform-path"],
             environment: environment).spm_chomp()
 
-        if let platformPath = platformPath, !platformPath.isEmpty {
-            // For XCTest framework.
-            let fwk = try AbsolutePath(validating: platformPath).appending(
-                components: "Developer", "Library", "Frameworks")
+        guard !platformPath.isEmpty else {
+            throw StringError("could not determine SDK platform path")
+        }
+
+        // For XCTest framework.
+        let fwk = try AbsolutePath(validating: platformPath).appending(
+            components: "Developer", "Library", "Frameworks")
 
-            // For XCTest Swift library.
-            let lib = try AbsolutePath(validating: platformPath).appending(
-                components: "Developer", "usr", "lib")
+        // For XCTest Swift library.
+        let lib = try AbsolutePath(validating: platformPath).appending(
+            components: "Developer", "usr", "lib")
 
-            _sdkPlatformFrameworkPath = (fwk, lib)
-        }
-        return _sdkPlatformFrameworkPath
+        let sdkPlatformFrameworkPath = (fwk, lib)
+        _sdkPlatformFrameworkPath = sdkPlatformFrameworkPath
+        return sdkPlatformFrameworkPath
     }
     /// Cache storage for sdk platform path.
     private static var _sdkPlatformFrameworkPath: (fwk: AbsolutePath, lib: AbsolutePath)? = nil

Sources/SPMTestSupport/Toolchain.swift

@@ -33,18 +33,6 @@ private func resolveBinDir() throws -> AbsolutePath {
 #endif
 }
 
-extension UserToolchain {
-
-#if os(macOS)
-    public var sdkPlatformFrameworksPath: AbsolutePath {
-        get throws {
-            return try Destination.sdkPlatformFrameworkPaths()!.fwk
-        }
-    }
-#endif
-
-}
-
 extension Destination {
     public static var `default`: Self {
         get throws {

Tests/CommandsTests/BuildToolTests.swift

@@ -280,8 +280,6 @@ final class BuildToolTests: CommandsTestCase {
     }
 
     func testBuildCompleteMessage() throws {
-        throw XCTSkip("This test fails to match the 'Compiling' regex; rdar://101815761")
-
         try fixture(name: "DependencyResolution/Internal/Simple") { fixturePath in
             do {
                 let result = try execute([], packagePath: fixturePath)

Tests/FunctionalTests/SwiftPMXCTestHelperTests.swift

@@ -49,7 +49,7 @@ class SwiftPMXCTestHelperTests: XCTestCase {
 
     func XCTAssertXCTestHelper(_ bundlePath: AbsolutePath, testCases: NSDictionary) throws {
         #if os(macOS)
-        let env = ["DYLD_FRAMEWORK_PATH": try UserToolchain.default.sdkPlatformFrameworksPath.pathString]
+        let env = ["DYLD_FRAMEWORK_PATH": try Destination.sdkPlatformFrameworkPaths().fwk.pathString]
         let outputFile = bundlePath.parentDirectory.appending(component: "tests.txt")
         let _ = try SwiftPMProduct.XCTestHelper.execute([bundlePath.pathString, outputFile.pathString], env: env)
         guard let data = NSData(contentsOfFile: outputFile.pathString) else {

Tests/PackageLoadingTests/PD_4_0_LoadingTests.swift

@@ -311,7 +311,7 @@ class PackageDescription4_0LoadingTests: PackageDescriptionLoadingTests {
 
         let observability = ObservabilitySystem.makeForTesting()
         XCTAssertThrowsError(try loadAndValidateManifest(content, observabilityScope: observability.topScope), "expected error") { error in
-            if case ManifestParseError.invalidManifestFormat(let error, _) = error {
+            if case ManifestParseError.invalidManifestFormat(let error, _, _) = error {
                 XCTAssert(error.contains("error: 'package(url:version:)' is unavailable: use package(url:exact:) instead"), "\(error)")
                 XCTAssert(error.contains("error: 'package(url:range:)' is unavailable: use package(url:_:) instead"), "\(error)")
             } else {

Tests/PackageLoadingTests/PD_4_2_LoadingTests.swift

@@ -103,7 +103,7 @@ class PackageDescription4_2LoadingTests: PackageDescriptionLoadingTests {
 
             let observability = ObservabilitySystem.makeForTesting()
             XCTAssertThrowsError(try loadAndValidateManifest(content, observabilityScope: observability.topScope), "expected error") { error in
-                if case ManifestParseError.invalidManifestFormat(let message, _) = error {
+                if case ManifestParseError.invalidManifestFormat(let message, _, _) = error {
                     XCTAssertMatch(
                         message,
                             .and(
@@ -166,7 +166,7 @@ class PackageDescription4_2LoadingTests: PackageDescriptionLoadingTests {
 
             let observability = ObservabilitySystem.makeForTesting()
             XCTAssertThrowsError(try loadAndValidateManifest(content, observabilityScope: observability.topScope), "expected error") { error in
-                if case ManifestParseError.invalidManifestFormat(let message, _) = error {
+                if case ManifestParseError.invalidManifestFormat(let message, _, _) = error {
                     XCTAssertMatch(message, .contains("is unavailable"))
                     XCTAssertMatch(message, .contains("was introduced in PackageDescription 5"))
                 } else {
@@ -188,7 +188,7 @@ class PackageDescription4_2LoadingTests: PackageDescriptionLoadingTests {
 
             let observability = ObservabilitySystem.makeForTesting()
             XCTAssertThrowsError(try loadAndValidateManifest(content, observabilityScope: observability.topScope), "expected error") { error in
-                if case ManifestParseError.invalidManifestFormat(let message, _) = error {
+                if case ManifestParseError.invalidManifestFormat(let message, _, _) = error {
                     XCTAssertMatch(message, .contains("is unavailable"))
                     XCTAssertMatch(message, .contains("was introduced in PackageDescription 5"))
                 } else {
@@ -208,7 +208,7 @@ class PackageDescription4_2LoadingTests: PackageDescriptionLoadingTests {
 
             let observability = ObservabilitySystem.makeForTesting()
             XCTAssertThrowsError(try loadAndValidateManifest(content, observabilityScope: observability.topScope), "expected error") { error in
-                if case ManifestParseError.invalidManifestFormat(let message, _) = error {
+                if case ManifestParseError.invalidManifestFormat(let message, _, _) = error {
                     XCTAssertMatch(message, .contains("is unavailable"))
                     XCTAssertMatch(message, .contains("was introduced in PackageDescription 5"))
                 } else {
@@ -239,7 +239,7 @@ class PackageDescription4_2LoadingTests: PackageDescriptionLoadingTests {
 
         let observability = ObservabilitySystem.makeForTesting()
         XCTAssertThrowsError(try loadAndValidateManifest(content, observabilityScope: observability.topScope), "expected error") { error in
-            if case ManifestParseError.invalidManifestFormat(let message, _) = error {
+            if case ManifestParseError.invalidManifestFormat(let message, _, _) = error {
                 XCTAssertMatch(message, .contains("is unavailable"))
                 XCTAssertMatch(message, .contains("was introduced in PackageDescription 5"))
             } else {
@@ -499,7 +499,7 @@ class PackageDescription4_2LoadingTests: PackageDescriptionLoadingTests {
 
         let observability = ObservabilitySystem.makeForTesting()
         XCTAssertThrowsError(try loadAndValidateManifest(content, observabilityScope: observability.topScope), "expected error") { error in
-            if case ManifestParseError.invalidManifestFormat(let message, let diagnosticFile) = error {
+            if case ManifestParseError.invalidManifestFormat(let message, let diagnosticFile, _) = error {
                 XCTAssertNil(diagnosticFile)
                 XCTAssertEqual(message, "'https://someurl.com' is not a valid path for path-based dependencies; use relative or absolute path instead.")
             } else {
@@ -519,9 +519,9 @@ class PackageDescription4_2LoadingTests: PackageDescriptionLoadingTests {
             case .invalidAbsolutePath:
               return nil
             case .relativePath:
-              return .invalidManifestFormat("file:// URLs cannot be relative, did you mean to use '.package(path:)'?", diagnosticFile: nil)
+              return .invalidManifestFormat("file:// URLs cannot be relative, did you mean to use '.package(path:)'?", diagnosticFile: nil, compilerCommandLine: nil)
             case .unsupportedHostname:
-              return .invalidManifestFormat("file:// URLs with hostnames are not supported, are you missing a '/'?", diagnosticFile: nil)
+              return .invalidManifestFormat("file:// URLs with hostnames are not supported, are you missing a '/'?", diagnosticFile: nil, compilerCommandLine: nil)
             }
           }