Skip to content

Include the compiler commandline in ManifestParseError.invalidManifestFormat #5958

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Dec 14, 2022
Merged

Include the compiler commandline in ManifestParseError.invalidManifestFormat #5958

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions Sources/PackageLoading/ManifestJSONParser.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand Down
19 changes: 16 additions & 3 deletions Sources/PackageLoading/ManifestLoader.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,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])

Expand All @@ -36,8 +38,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"))"
case .importsRestrictedModules(let modules):
Expand Down Expand Up @@ -292,7 +300,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.
Expand Down Expand Up @@ -660,6 +668,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))
Expand Down Expand Up @@ -898,6 +908,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.
Expand Down
2 changes: 1 addition & 1 deletion Tests/PackageLoadingTests/PD_4_0_LoadingTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
16 changes: 8 additions & 8 deletions Tests/PackageLoadingTests/PD_4_2_LoadingTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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 {
Expand All @@ -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 {
Expand All @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand All @@ -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)
}
}

Expand Down
12 changes: 6 additions & 6 deletions Tests/PackageLoadingTests/PD_5_0_LoadingTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ class PackageDescription5_0LoadingTests: 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("'v3' is unavailable"))
XCTAssertMatch(message, .contains("'v3' was obsoleted in PackageDescription 5"))
} else {
Expand Down Expand Up @@ -274,7 +274,7 @@ class PackageDescription5_0LoadingTests: 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("error: 'v11' is unavailable"))
XCTAssertMatch(message, .contains("note: 'v11' was introduced in PackageDescription 5.3"))
XCTAssertMatch(message, .contains("note: 'v14' was introduced in PackageDescription 5.3"))
Expand All @@ -298,7 +298,7 @@ class PackageDescription5_0LoadingTests: 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("error: 'v10_16' has been renamed to 'v11'"))
XCTAssertMatch(message, .contains("note: 'v10_16' has been explicitly marked unavailable here"))
XCTAssertMatch(message, .contains("note: 'v14' was introduced in PackageDescription 5.3"))
Expand Down Expand Up @@ -395,7 +395,7 @@ class PackageDescription5_0LoadingTests: PackageDescriptionLoadingTests {
fileSystem: fs,
observabilityScope: observability.topScope
)
} catch ManifestParseError.invalidManifestFormat(let error, let diagnosticFile) {
} catch ManifestParseError.invalidManifestFormat(let error, let diagnosticFile, _) {
XCTAssertMatch(error, .contains("expected expression in container literal"))
let contents = try localFileSystem.readFileContents(diagnosticFile!)
XCTAssertNotNil(contents)
Expand Down Expand Up @@ -506,7 +506,7 @@ class PackageDescription5_0LoadingTests: PackageDescriptionLoadingTests {
do {
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.2"))
} else {
Expand Down Expand Up @@ -553,7 +553,7 @@ class PackageDescription5_0LoadingTests: 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.2"))
} else {
Expand Down
14 changes: 7 additions & 7 deletions Tests/PackageLoadingTests/PD_5_2_LoadingTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ class PackageDescription5_2LoadingTests: 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: \'product(name:package:)\' is unavailable: the 'package' argument is mandatory as of tools version 5.2"))
} else {
XCTFail("unexpected error: \(error)")
Expand Down Expand Up @@ -277,7 +277,7 @@ class PackageDescription5_2LoadingTests: 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 {
XCTAssertMatch(error, .contains("is unavailable"))
XCTAssertMatch(error, .contains("was introduced in PackageDescription 5.3"))
} else {
Expand All @@ -303,7 +303,7 @@ class PackageDescription5_2LoadingTests: 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 {
XCTAssertMatch(error, .contains("is unavailable"))
XCTAssertMatch(error, .contains("was introduced in PackageDescription 5.3"))
} else {
Expand All @@ -329,7 +329,7 @@ class PackageDescription5_2LoadingTests: 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 {
XCTAssertMatch(error, .contains("is unavailable"))
XCTAssertMatch(error, .contains("was introduced in PackageDescription 5.3"))
} else {
Expand Down Expand Up @@ -359,7 +359,7 @@ class PackageDescription5_2LoadingTests: 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 {
XCTAssertMatch(error, .contains("is unavailable"))
XCTAssertMatch(error, .contains("was introduced in PackageDescription 5.3"))
} else {
Expand All @@ -384,7 +384,7 @@ class PackageDescription5_2LoadingTests: 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 {
XCTAssertMatch(error, .contains("is unavailable"))
XCTAssertMatch(error, .contains("was introduced in PackageDescription 5.3"))
} else {
Expand Down Expand Up @@ -415,7 +415,7 @@ class PackageDescription5_2LoadingTests: 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 {
XCTAssertTrue(error.contains("Operation not permitted"), "unexpected error message: \(error)")
} else {
XCTFail("unexpected error: \(error)")
Expand Down
2 changes: 1 addition & 1 deletion Tests/PackageLoadingTests/PD_5_3_LoadingTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -541,7 +541,7 @@ class PackageDescription5_3LoadingTests: 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 {
XCTAssertTrue(error.contains("Operation not permitted"), "unexpected error message: \(error)")
} else {
XCTFail("unexpected error: \(error)")
Expand Down
Loading