Skip to content

Commit b02afb6

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 4111569 commit b02afb6

18 files changed

+79
-78
lines changed

‎Sources/Commands/Utilities/TestingSupport.swift

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -82,13 +82,13 @@ enum TestingSupport {
8282
),
8383
sanitizers: sanitizers
8484
)
85-
// Add the sdk platform path if we have it. If this is not present, we
86-
// might always end up failing.
87-
if let sdkPlatformFrameworksPath = try Destination.sdkPlatformFrameworkPaths() {
88-
// appending since we prefer the user setting (if set) to the one we inject
89-
env.appendPath("DYLD_FRAMEWORK_PATH", value: sdkPlatformFrameworksPath.fwk.pathString)
90-
env.appendPath("DYLD_LIBRARY_PATH", value: sdkPlatformFrameworksPath.lib.pathString)
91-
}
85+
86+
// Add the sdk platform path if we have it. If this is not present, we might always end up failing.
87+
let sdkPlatformFrameworksPath = try Destination.sdkPlatformFrameworkPaths()
88+
// appending since we prefer the user setting (if set) to the one we inject
89+
env.appendPath("DYLD_FRAMEWORK_PATH", value: sdkPlatformFrameworksPath.fwk.pathString)
90+
env.appendPath("DYLD_LIBRARY_PATH", value: sdkPlatformFrameworksPath.lib.pathString)
91+
9292
try TSCBasic.Process.checkNonZeroExit(arguments: args, environment: env)
9393
// Read the temporary file's content.
9494
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: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,8 @@ public enum ManifestParseError: Swift.Error, Equatable {
2323
/// The manifest contains invalid format.
2424
case invalidManifestFormat(String, diagnosticFile: AbsolutePath?)
2525
// TODO: Test this error.
26+
case invalidManifestFormat(String, diagnosticFile: AbsolutePath?, compilerCommandLine: [String]?)
27+
2628
/// The manifest was successfully loaded by swift interpreter but there were runtime issues.
2729
case runtimeManifestErrors([String])
2830

@@ -36,8 +38,14 @@ extension ManifestParseError: CustomStringConvertible {
3638
switch self {
3739
case .emptyManifest(let manifestPath):
3840
return "'\(manifestPath)' is empty"
39-
case .invalidManifestFormat(let error, _):
40-
return "invalid manifest\n\(error)"
41+
case .invalidManifestFormat(let error, _, let compilerCommandLine):
42+
let suffix: String
43+
if let compilerCommandLine = compilerCommandLine {
44+
suffix = " (compiled with: \(compilerCommandLine))"
45+
} else {
46+
suffix = ""
47+
}
48+
return "Invalid manifest\(suffix)\n\(error)"
4149
case .runtimeManifestErrors(let errors):
4250
return "invalid manifest (evaluation failed)\n\(errors.joined(separator: "\n"))"
4351
case .importsRestrictedModules(let modules):
@@ -292,7 +300,7 @@ public final class ManifestLoader: ManifestLoaderProtocol {
292300
// Throw now if we weren't able to parse the manifest.
293301
guard let manifestJSON = result.manifestJSON, !manifestJSON.isEmpty else {
294302
let errors = result.errorOutput ?? result.compilerOutput ?? "Missing or empty JSON output from manifest compilation for \(packageIdentity)"
295-
throw ManifestParseError.invalidManifestFormat(errors, diagnosticFile: result.diagnosticFile)
303+
throw ManifestParseError.invalidManifestFormat(errors, diagnosticFile: result.diagnosticFile, compilerCommandLine: result.compilerCommandLine)
296304
}
297305

298306
// We should not have any fatal error at this point.
@@ -660,6 +668,8 @@ public final class ManifestLoader: ManifestLoaderProtocol {
660668
let compiledManifestFile = tmpDir.appending(component: "\(packageIdentity)-manifest\(executableSuffix)")
661669
cmd += ["-o", compiledManifestFile.pathString]
662670

671+
evaluationResult.compilerCommandLine = cmd
672+
663673
// Compile the manifest.
664674
TSCBasic.Process.popen(arguments: cmd, environment: self.toolchain.swiftCompilerEnvironment, queue: callbackQueue) { result in
665675
dispatchPrecondition(condition: .onQueue(callbackQueue))
@@ -898,6 +908,9 @@ extension ManifestLoader {
898908
/// The manifest in JSON format.
899909
var manifestJSON: String?
900910

911+
/// The command line used to compile the manifest
912+
var compilerCommandLine: [String]?
913+
901914
/// Any non-compiler error that might have occurred during manifest loading.
902915
///
903916
/// 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
@@ -195,12 +195,11 @@ public struct Destination: Encodable, Equatable {
195195
var extraCCFlags: [String] = []
196196
var extraSwiftCFlags: [String] = []
197197
#if os(macOS)
198-
if let sdkPaths = try Destination.sdkPlatformFrameworkPaths(environment: environment) {
199-
extraCCFlags += ["-F", sdkPaths.fwk.pathString]
200-
extraSwiftCFlags += ["-F", sdkPaths.fwk.pathString]
201-
extraSwiftCFlags += ["-I", sdkPaths.lib.pathString]
202-
extraSwiftCFlags += ["-L", sdkPaths.lib.pathString]
203-
}
198+
let sdkPaths = try Destination.sdkPlatformFrameworkPaths(environment: environment)
199+
extraCCFlags += ["-F", sdkPaths.fwk.pathString]
200+
extraSwiftCFlags += ["-F", sdkPaths.fwk.pathString]
201+
extraSwiftCFlags += ["-I", sdkPaths.lib.pathString]
202+
extraSwiftCFlags += ["-L", sdkPaths.lib.pathString]
204203
#endif
205204

206205
#if !os(Windows)
@@ -217,26 +216,29 @@ public struct Destination: Encodable, Equatable {
217216
/// Returns macosx sdk platform framework path.
218217
public static func sdkPlatformFrameworkPaths(
219218
environment: EnvironmentVariables = .process()
220-
) throws -> (fwk: AbsolutePath, lib: AbsolutePath)? {
219+
) throws -> (fwk: AbsolutePath, lib: AbsolutePath) {
221220
if let path = _sdkPlatformFrameworkPath {
222221
return path
223222
}
224-
let platformPath = try? TSCBasic.Process.checkNonZeroExit(
223+
let platformPath = try TSCBasic.Process.checkNonZeroExit(
225224
arguments: ["/usr/bin/xcrun", "--sdk", "macosx", "--show-sdk-platform-path"],
226225
environment: environment).spm_chomp()
227226

228-
if let platformPath = platformPath, !platformPath.isEmpty {
229-
// For XCTest framework.
230-
let fwk = try AbsolutePath(validating: platformPath).appending(
231-
components: "Developer", "Library", "Frameworks")
227+
guard !platformPath.isEmpty else {
228+
throw StringError("could not determine SDK platform path")
229+
}
230+
231+
// For XCTest framework.
232+
let fwk = try AbsolutePath(validating: platformPath).appending(
233+
components: "Developer", "Library", "Frameworks")
232234

233-
// For XCTest Swift library.
234-
let lib = try AbsolutePath(validating: platformPath).appending(
235-
components: "Developer", "usr", "lib")
235+
// For XCTest Swift library.
236+
let lib = try AbsolutePath(validating: platformPath).appending(
237+
components: "Developer", "usr", "lib")
236238

237-
_sdkPlatformFrameworkPath = (fwk, lib)
238-
}
239-
return _sdkPlatformFrameworkPath
239+
let sdkPlatformFrameworkPath = (fwk, lib)
240+
_sdkPlatformFrameworkPath = sdkPlatformFrameworkPath
241+
return sdkPlatformFrameworkPath
240242
}
241243

242244
/// Cache storage for sdk platform path.

‎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

‎Tests/PackageLoadingTests/PD_5_0_LoadingTests.swift

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -112,7 +112,7 @@ class PackageDescription5_0LoadingTests: PackageDescriptionLoadingTests {
112112

113113
let observability = ObservabilitySystem.makeForTesting()
114114
XCTAssertThrowsError(try loadAndValidateManifest(content, observabilityScope: observability.topScope), "expected error") { error in
115-
if case ManifestParseError.invalidManifestFormat(let message, _) = error {
115+
if case ManifestParseError.invalidManifestFormat(let message, _, _) = error {
116116
XCTAssertMatch(message, .contains("'v3' is unavailable"))
117117
XCTAssertMatch(message, .contains("'v3' was obsoleted in PackageDescription 5"))
118118
} else {
@@ -274,7 +274,7 @@ class PackageDescription5_0LoadingTests: PackageDescriptionLoadingTests {
274274

275275
let observability = ObservabilitySystem.makeForTesting()
276276
XCTAssertThrowsError(try loadAndValidateManifest(content, observabilityScope: observability.topScope), "expected error") { error in
277-
if case ManifestParseError.invalidManifestFormat(let message, _) = error {
277+
if case ManifestParseError.invalidManifestFormat(let message, _, _) = error {
278278
XCTAssertMatch(message, .contains("error: 'v11' is unavailable"))
279279
XCTAssertMatch(message, .contains("note: 'v11' was introduced in PackageDescription 5.3"))
280280
XCTAssertMatch(message, .contains("note: 'v14' was introduced in PackageDescription 5.3"))
@@ -298,7 +298,7 @@ class PackageDescription5_0LoadingTests: PackageDescriptionLoadingTests {
298298

299299
let observability = ObservabilitySystem.makeForTesting()
300300
XCTAssertThrowsError(try loadAndValidateManifest(content, observabilityScope: observability.topScope), "expected error") { error in
301-
if case ManifestParseError.invalidManifestFormat(let message, _) = error {
301+
if case ManifestParseError.invalidManifestFormat(let message, _, _) = error {
302302
XCTAssertMatch(message, .contains("error: 'v10_16' has been renamed to 'v11'"))
303303
XCTAssertMatch(message, .contains("note: 'v10_16' has been explicitly marked unavailable here"))
304304
XCTAssertMatch(message, .contains("note: 'v14' was introduced in PackageDescription 5.3"))
@@ -395,7 +395,7 @@ class PackageDescription5_0LoadingTests: PackageDescriptionLoadingTests {
395395
fileSystem: fs,
396396
observabilityScope: observability.topScope
397397
)
398-
} catch ManifestParseError.invalidManifestFormat(let error, let diagnosticFile) {
398+
} catch ManifestParseError.invalidManifestFormat(let error, let diagnosticFile, _) {
399399
XCTAssertMatch(error, .contains("expected expression in container literal"))
400400
let contents = try localFileSystem.readFileContents(diagnosticFile!)
401401
XCTAssertNotNil(contents)
@@ -506,7 +506,7 @@ class PackageDescription5_0LoadingTests: PackageDescriptionLoadingTests {
506506
do {
507507
let observability = ObservabilitySystem.makeForTesting()
508508
XCTAssertThrowsError(try loadAndValidateManifest(content, observabilityScope: observability.topScope), "expected error") { error in
509-
if case ManifestParseError.invalidManifestFormat(let message, _) = error {
509+
if case ManifestParseError.invalidManifestFormat(let message, _, _) = error {
510510
XCTAssertMatch(message, .contains("is unavailable"))
511511
XCTAssertMatch(message, .contains("was introduced in PackageDescription 5.2"))
512512
} else {
@@ -553,7 +553,7 @@ class PackageDescription5_0LoadingTests: PackageDescriptionLoadingTests {
553553

554554
let observability = ObservabilitySystem.makeForTesting()
555555
XCTAssertThrowsError(try loadAndValidateManifest(content, observabilityScope: observability.topScope), "expected error") { error in
556-
if case ManifestParseError.invalidManifestFormat(let message, _) = error {
556+
if case ManifestParseError.invalidManifestFormat(let message, _, _) = error {
557557
XCTAssertMatch(message, .contains("is unavailable"))
558558
XCTAssertMatch(message, .contains("was introduced in PackageDescription 5.2"))
559559
} else {

0 commit comments

Comments
 (0)