Skip to content

Commit 98e5054

Browse files
neonichuneonichu
committed
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
1 parent 3c692dc commit 98e5054

17 files changed

+76
-77
lines changed

‎Sources/Commands/Utilities/TestingSupport.swift

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -81,13 +81,13 @@ enum TestingSupport {
8181
),
8282
sanitizers: sanitizers
8383
)
84-
// Add the sdk platform path if we have it. If this is not present, we
85-
// might always end up failing.
86-
if let sdkPlatformFrameworksPath = try Destination.sdkPlatformFrameworkPaths() {
87-
// appending since we prefer the user setting (if set) to the one we inject
88-
env.appendPath("DYLD_FRAMEWORK_PATH", value: sdkPlatformFrameworksPath.fwk.pathString)
89-
env.appendPath("DYLD_LIBRARY_PATH", value: sdkPlatformFrameworksPath.lib.pathString)
90-
}
84+
85+
// Add the sdk platform path if we have it. If this is not present, we might always end up failing.
86+
let sdkPlatformFrameworksPath = try Destination.sdkPlatformFrameworkPaths()
87+
// appending since we prefer the user setting (if set) to the one we inject
88+
env.appendPath("DYLD_FRAMEWORK_PATH", value: sdkPlatformFrameworksPath.fwk.pathString)
89+
env.appendPath("DYLD_LIBRARY_PATH", value: sdkPlatformFrameworksPath.lib.pathString)
90+
9191
try TSCBasic.Process.checkNonZeroExit(arguments: args, environment: env)
9292
// Read the temporary file's content.
9393
return try swiftTool.fileSystem.readFileContents(tempFile.path)

‎Sources/PackageLoading/ManifestJSONParser.swift

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -165,7 +165,7 @@ enum ManifestJSONParser {
165165
do {
166166
path = try AbsolutePath(validating: location)
167167
} catch PathValidationError.invalidAbsolutePath(let path) {
168-
throw ManifestParseError.invalidManifestFormat("'\(path)' is not a valid path for path-based dependencies; use relative or absolute path instead.", diagnosticFile: nil)
168+
throw ManifestParseError.invalidManifestFormat("'\(path)' is not a valid path for path-based dependencies; use relative or absolute path instead.", diagnosticFile: nil, compilerCommandLine: nil)
169169
}
170170
let identity = try identityResolver.resolveIdentity(for: path)
171171
return .fileSystem(identity: identity,
@@ -224,11 +224,11 @@ enum ManifestJSONParser {
224224
guard hostnameComponent.isEmpty else {
225225
if hostnameComponent == ".." {
226226
throw ManifestParseError.invalidManifestFormat(
227-
"file:// URLs cannot be relative, did you mean to use '.package(path:)'?", diagnosticFile: nil
227+
"file:// URLs cannot be relative, did you mean to use '.package(path:)'?", diagnosticFile: nil, compilerCommandLine: nil
228228
)
229229
}
230230
throw ManifestParseError.invalidManifestFormat(
231-
"file:// URLs with hostnames are not supported, are you missing a '/'?", diagnosticFile: nil
231+
"file:// URLs with hostnames are not supported, are you missing a '/'?", diagnosticFile: nil, compilerCommandLine: nil
232232
)
233233
}
234234
return try AbsolutePath(validating: location).pathString

‎Sources/PackageLoading/ManifestLoader.swift

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ import enum TSCUtility.Diagnostics
1818

1919
public enum ManifestParseError: Swift.Error, Equatable {
2020
/// The manifest contains invalid format.
21-
case invalidManifestFormat(String, diagnosticFile: AbsolutePath?)
21+
case invalidManifestFormat(String, diagnosticFile: AbsolutePath?, compilerCommandLine: [String]?)
2222

2323
/// The manifest was successfully loaded by swift interpreter but there were runtime issues.
2424
case runtimeManifestErrors([String])
@@ -28,8 +28,14 @@ public enum ManifestParseError: Swift.Error, Equatable {
2828
extension ManifestParseError: CustomStringConvertible {
2929
public var description: String {
3030
switch self {
31-
case .invalidManifestFormat(let error, _):
32-
return "Invalid manifest\n\(error)"
31+
case .invalidManifestFormat(let error, _, let compilerCommandLine):
32+
let suffix: String
33+
if let compilerCommandLine = compilerCommandLine {
34+
suffix = " (compiled with: \(compilerCommandLine))"
35+
} else {
36+
suffix = ""
37+
}
38+
return "Invalid manifest\(suffix)\n\(error)"
3339
case .runtimeManifestErrors(let errors):
3440
return "Invalid manifest (evaluation failed)\n\(errors.joined(separator: "\n"))"
3541
}
@@ -279,7 +285,7 @@ public final class ManifestLoader: ManifestLoaderProtocol {
279285
// Throw now if we weren't able to parse the manifest.
280286
guard let manifestJSON = result.manifestJSON, !manifestJSON.isEmpty else {
281287
let errors = result.errorOutput ?? result.compilerOutput ?? "Missing or empty JSON output from manifest compilation for \(packageIdentity)"
282-
throw ManifestParseError.invalidManifestFormat(errors, diagnosticFile: result.diagnosticFile)
288+
throw ManifestParseError.invalidManifestFormat(errors, diagnosticFile: result.diagnosticFile, compilerCommandLine: result.compilerCommandLine)
283289
}
284290

285291
// We should not have any fatal error at this point.
@@ -589,6 +595,8 @@ public final class ManifestLoader: ManifestLoaderProtocol {
589595
let compiledManifestFile = tmpDir.appending(component: "\(packageIdentity)-manifest\(executableSuffix)")
590596
cmd += ["-o", compiledManifestFile.pathString]
591597

598+
evaluationResult.compilerCommandLine = cmd
599+
592600
// Compile the manifest.
593601
TSCBasic.Process.popen(arguments: cmd, environment: self.toolchain.swiftCompilerEnvironment, queue: callbackQueue) { result in
594602
dispatchPrecondition(condition: .onQueue(callbackQueue))
@@ -827,6 +835,9 @@ extension ManifestLoader {
827835
/// The manifest in JSON format.
828836
var manifestJSON: String?
829837

838+
/// The command line used to compile the manifest
839+
var compilerCommandLine: [String]?
840+
830841
/// Any non-compiler error that might have occurred during manifest loading.
831842
///
832843
/// For e.g., we could have failed to spawn the process or create temporary file.

‎Sources/PackageModel/Destination.swift

Lines changed: 20 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -167,12 +167,11 @@ public struct Destination: Encodable, Equatable {
167167
var extraCCFlags: [String] = []
168168
var extraSwiftCFlags: [String] = []
169169
#if os(macOS)
170-
if let sdkPaths = try Destination.sdkPlatformFrameworkPaths(environment: environment) {
171-
extraCCFlags += ["-F", sdkPaths.fwk.pathString]
172-
extraSwiftCFlags += ["-F", sdkPaths.fwk.pathString]
173-
extraSwiftCFlags += ["-I", sdkPaths.lib.pathString]
174-
extraSwiftCFlags += ["-L", sdkPaths.lib.pathString]
175-
}
170+
let sdkPaths = try Destination.sdkPlatformFrameworkPaths(environment: environment)
171+
extraCCFlags += ["-F", sdkPaths.fwk.pathString]
172+
extraSwiftCFlags += ["-F", sdkPaths.fwk.pathString]
173+
extraSwiftCFlags += ["-I", sdkPaths.lib.pathString]
174+
extraSwiftCFlags += ["-L", sdkPaths.lib.pathString]
176175
#endif
177176

178177
#if !os(Windows)
@@ -190,26 +189,29 @@ public struct Destination: Encodable, Equatable {
190189
/// Returns macosx sdk platform framework path.
191190
public static func sdkPlatformFrameworkPaths(
192191
environment: EnvironmentVariables = .process()
193-
) throws -> (fwk: AbsolutePath, lib: AbsolutePath)? {
192+
) throws -> (fwk: AbsolutePath, lib: AbsolutePath) {
194193
if let path = _sdkPlatformFrameworkPath {
195194
return path
196195
}
197-
let platformPath = try? TSCBasic.Process.checkNonZeroExit(
196+
let platformPath = try TSCBasic.Process.checkNonZeroExit(
198197
arguments: ["/usr/bin/xcrun", "--sdk", "macosx", "--show-sdk-platform-path"],
199198
environment: environment).spm_chomp()
200199

201-
if let platformPath = platformPath, !platformPath.isEmpty {
202-
// For XCTest framework.
203-
let fwk = try AbsolutePath(validating: platformPath).appending(
204-
components: "Developer", "Library", "Frameworks")
200+
guard !platformPath.isEmpty else {
201+
throw StringError("could not determine SDK platform path")
202+
}
203+
204+
// For XCTest framework.
205+
let fwk = try AbsolutePath(validating: platformPath).appending(
206+
components: "Developer", "Library", "Frameworks")
205207

206-
// For XCTest Swift library.
207-
let lib = try AbsolutePath(validating: platformPath).appending(
208-
components: "Developer", "usr", "lib")
208+
// For XCTest Swift library.
209+
let lib = try AbsolutePath(validating: platformPath).appending(
210+
components: "Developer", "usr", "lib")
209211

210-
_sdkPlatformFrameworkPath = (fwk, lib)
211-
}
212-
return _sdkPlatformFrameworkPath
212+
let sdkPlatformFrameworkPath = (fwk, lib)
213+
_sdkPlatformFrameworkPath = sdkPlatformFrameworkPath
214+
return sdkPlatformFrameworkPath
213215
}
214216
/// Cache storage for sdk platform path.
215217
private static var _sdkPlatformFrameworkPath: (fwk: AbsolutePath, lib: AbsolutePath)? = nil

‎Sources/SPMTestSupport/Toolchain.swift

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -33,18 +33,6 @@ private func resolveBinDir() throws -> AbsolutePath {
3333
#endif
3434
}
3535

36-
extension UserToolchain {
37-
38-
#if os(macOS)
39-
public var sdkPlatformFrameworksPath: AbsolutePath {
40-
get throws {
41-
return try Destination.sdkPlatformFrameworkPaths()!.fwk
42-
}
43-
}
44-
#endif
45-
46-
}
47-
4836
extension Destination {
4937
public static var `default`: Self {
5038
get throws {

‎Tests/CommandsTests/BuildToolTests.swift

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -280,8 +280,6 @@ final class BuildToolTests: CommandsTestCase {
280280
}
281281

282282
func testBuildCompleteMessage() throws {
283-
throw XCTSkip("This test fails to match the 'Compiling' regex; rdar://101815761")
284-
285283
try fixture(name: "DependencyResolution/Internal/Simple") { fixturePath in
286284
do {
287285
let result = try execute([], packagePath: fixturePath)

‎Tests/FunctionalTests/SwiftPMXCTestHelperTests.swift

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,7 @@ class SwiftPMXCTestHelperTests: XCTestCase {
4949

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

‎Tests/PackageLoadingTests/PD_4_0_LoadingTests.swift

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -311,7 +311,7 @@ class PackageDescription4_0LoadingTests: PackageDescriptionLoadingTests {
311311

312312
let observability = ObservabilitySystem.makeForTesting()
313313
XCTAssertThrowsError(try loadAndValidateManifest(content, observabilityScope: observability.topScope), "expected error") { error in
314-
if case ManifestParseError.invalidManifestFormat(let error, _) = error {
314+
if case ManifestParseError.invalidManifestFormat(let error, _, _) = error {
315315
XCTAssert(error.contains("error: 'package(url:version:)' is unavailable: use package(url:exact:) instead"), "\(error)")
316316
XCTAssert(error.contains("error: 'package(url:range:)' is unavailable: use package(url:_:) instead"), "\(error)")
317317
} else {

‎Tests/PackageLoadingTests/PD_4_2_LoadingTests.swift

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -103,7 +103,7 @@ class PackageDescription4_2LoadingTests: PackageDescriptionLoadingTests {
103103

104104
let observability = ObservabilitySystem.makeForTesting()
105105
XCTAssertThrowsError(try loadAndValidateManifest(content, observabilityScope: observability.topScope), "expected error") { error in
106-
if case ManifestParseError.invalidManifestFormat(let message, _) = error {
106+
if case ManifestParseError.invalidManifestFormat(let message, _, _) = error {
107107
XCTAssertMatch(
108108
message,
109109
.and(
@@ -166,7 +166,7 @@ class PackageDescription4_2LoadingTests: PackageDescriptionLoadingTests {
166166

167167
let observability = ObservabilitySystem.makeForTesting()
168168
XCTAssertThrowsError(try loadAndValidateManifest(content, observabilityScope: observability.topScope), "expected error") { error in
169-
if case ManifestParseError.invalidManifestFormat(let message, _) = error {
169+
if case ManifestParseError.invalidManifestFormat(let message, _, _) = error {
170170
XCTAssertMatch(message, .contains("is unavailable"))
171171
XCTAssertMatch(message, .contains("was introduced in PackageDescription 5"))
172172
} else {
@@ -188,7 +188,7 @@ class PackageDescription4_2LoadingTests: PackageDescriptionLoadingTests {
188188

189189
let observability = ObservabilitySystem.makeForTesting()
190190
XCTAssertThrowsError(try loadAndValidateManifest(content, observabilityScope: observability.topScope), "expected error") { error in
191-
if case ManifestParseError.invalidManifestFormat(let message, _) = error {
191+
if case ManifestParseError.invalidManifestFormat(let message, _, _) = error {
192192
XCTAssertMatch(message, .contains("is unavailable"))
193193
XCTAssertMatch(message, .contains("was introduced in PackageDescription 5"))
194194
} else {
@@ -208,7 +208,7 @@ class PackageDescription4_2LoadingTests: PackageDescriptionLoadingTests {
208208

209209
let observability = ObservabilitySystem.makeForTesting()
210210
XCTAssertThrowsError(try loadAndValidateManifest(content, observabilityScope: observability.topScope), "expected error") { error in
211-
if case ManifestParseError.invalidManifestFormat(let message, _) = error {
211+
if case ManifestParseError.invalidManifestFormat(let message, _, _) = error {
212212
XCTAssertMatch(message, .contains("is unavailable"))
213213
XCTAssertMatch(message, .contains("was introduced in PackageDescription 5"))
214214
} else {
@@ -239,7 +239,7 @@ class PackageDescription4_2LoadingTests: PackageDescriptionLoadingTests {
239239

240240
let observability = ObservabilitySystem.makeForTesting()
241241
XCTAssertThrowsError(try loadAndValidateManifest(content, observabilityScope: observability.topScope), "expected error") { error in
242-
if case ManifestParseError.invalidManifestFormat(let message, _) = error {
242+
if case ManifestParseError.invalidManifestFormat(let message, _, _) = error {
243243
XCTAssertMatch(message, .contains("is unavailable"))
244244
XCTAssertMatch(message, .contains("was introduced in PackageDescription 5"))
245245
} else {
@@ -499,7 +499,7 @@ class PackageDescription4_2LoadingTests: PackageDescriptionLoadingTests {
499499

500500
let observability = ObservabilitySystem.makeForTesting()
501501
XCTAssertThrowsError(try loadAndValidateManifest(content, observabilityScope: observability.topScope), "expected error") { error in
502-
if case ManifestParseError.invalidManifestFormat(let message, let diagnosticFile) = error {
502+
if case ManifestParseError.invalidManifestFormat(let message, let diagnosticFile, _) = error {
503503
XCTAssertNil(diagnosticFile)
504504
XCTAssertEqual(message, "'https://someurl.com' is not a valid path for path-based dependencies; use relative or absolute path instead.")
505505
} else {
@@ -519,9 +519,9 @@ class PackageDescription4_2LoadingTests: PackageDescriptionLoadingTests {
519519
case .invalidAbsolutePath:
520520
return nil
521521
case .relativePath:
522-
return .invalidManifestFormat("file:// URLs cannot be relative, did you mean to use '.package(path:)'?", diagnosticFile: nil)
522+
return .invalidManifestFormat("file:// URLs cannot be relative, did you mean to use '.package(path:)'?", diagnosticFile: nil, compilerCommandLine: nil)
523523
case .unsupportedHostname:
524-
return .invalidManifestFormat("file:// URLs with hostnames are not supported, are you missing a '/'?", diagnosticFile: nil)
524+
return .invalidManifestFormat("file:// URLs with hostnames are not supported, are you missing a '/'?", diagnosticFile: nil, compilerCommandLine: nil)
525525
}
526526
}
527527

0 commit comments

Comments
 (0)