From e00045be31fcab9056656254b14d5d0947ec224e Mon Sep 17 00:00:00 2001 From: stevapple Date: Thu, 8 Sep 2022 22:13:52 +0800 Subject: [PATCH 1/6] Remove `rdar://73710910` workaround --- Sources/PackageLoading/ManifestLoader.swift | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Sources/PackageLoading/ManifestLoader.swift b/Sources/PackageLoading/ManifestLoader.swift index 4bcb47364a6..9fbe2b5aeef 100644 --- a/Sources/PackageLoading/ManifestLoader.swift +++ b/Sources/PackageLoading/ManifestLoader.swift @@ -290,8 +290,7 @@ public final class ManifestLoader: ManifestLoaderProtocol { // We might have some non-fatal output (warnings/notes) from the compiler even when // we were able to parse the manifest successfully. if let compilerOutput = result.compilerOutput { - // FIXME: Temporary workaround to filter out debug output from integrated Swift driver. [rdar://73710910] - if !(compilerOutput.hasPrefix(":0: remark: new Swift driver at") && compilerOutput.hasSuffix("will be used")) { + if !compilerOutput.isEmpty { let metadata = result.diagnosticFile.map { diagnosticFile -> ObservabilityMetadata in var metadata = ObservabilityMetadata() metadata.manifestLoadingDiagnosticFile = diagnosticFile From cc19d07bf17a3e9f9df125c681a4a7f0cbda48d9 Mon Sep 17 00:00:00 2001 From: stevapple Date: Thu, 8 Sep 2022 22:28:13 +0800 Subject: [PATCH 2/6] Cache compiled manifest path for diagnostic --- Sources/PackageLoading/ManifestLoader.swift | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/Sources/PackageLoading/ManifestLoader.swift b/Sources/PackageLoading/ManifestLoader.swift index 9fbe2b5aeef..324d8bbd2d0 100644 --- a/Sources/PackageLoading/ManifestLoader.swift +++ b/Sources/PackageLoading/ManifestLoader.swift @@ -585,6 +585,7 @@ public final class ManifestLoader: ManifestLoaderProtocol { #endif let compiledManifestFile = tmpDir.appending(component: "\(packageIdentity)-manifest\(executableSuffix)") cmd += ["-o", compiledManifestFile.pathString] + evaluationResult.compiledManifestFile = compiledManifestFile // Compile the manifest. TSCBasic.Process.popen(arguments: cmd, environment: self.toolchain.swiftCompilerEnvironment, queue: callbackQueue) { result in @@ -806,6 +807,9 @@ extension ManifestLoader { extension ManifestLoader { struct EvaluationResult: Codable { + /// The path to the compiled manifest. + var compiledManifestFile: AbsolutePath? + /// The path to the diagnostics file (.dia). /// /// This is only present if serialized diagnostics are enabled. From 5d628f453f293c15444a85861a395fbc299ceea5 Mon Sep 17 00:00:00 2001 From: stevapple Date: Thu, 8 Sep 2022 22:35:57 +0800 Subject: [PATCH 3/6] `LINK` library creation note is not warning --- Sources/PackageLoading/ManifestLoader.swift | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/Sources/PackageLoading/ManifestLoader.swift b/Sources/PackageLoading/ManifestLoader.swift index 324d8bbd2d0..f4e4ec6506c 100644 --- a/Sources/PackageLoading/ManifestLoader.swift +++ b/Sources/PackageLoading/ManifestLoader.swift @@ -290,7 +290,18 @@ public final class ManifestLoader: ManifestLoaderProtocol { // We might have some non-fatal output (warnings/notes) from the compiler even when // we were able to parse the manifest successfully. if let compilerOutput = result.compilerOutput { - if !compilerOutput.isEmpty { + // FIXME: Isn't the judgement too naive? + var outputIndicatesWarning = !compilerOutput.isEmpty + #if os(Windows) + // Filter out link.exe note when creating manifest executable. + if let compiledManifestName = result.compiledManifestFile?.basenameWithoutExt, + !compilerOutput.contains(where: \.isNewline) { + if compilerOutput.contains("\(compiledManifestName).lib") && compilerOutput.contains("\(compiledManifestName).exp") { + outputIndicatesWarning = false + } + } + #endif + if outputIndicatesWarning { let metadata = result.diagnosticFile.map { diagnosticFile -> ObservabilityMetadata in var metadata = ObservabilityMetadata() metadata.manifestLoadingDiagnosticFile = diagnosticFile From 49482eb87a9d001e9319bec5322c3bc153a3dedb Mon Sep 17 00:00:00 2001 From: stevapple Date: Thu, 8 Sep 2022 22:40:41 +0800 Subject: [PATCH 4/6] Emit `LINK` note as debug-level diagnostic. --- Sources/PackageLoading/ManifestLoader.swift | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/Sources/PackageLoading/ManifestLoader.swift b/Sources/PackageLoading/ManifestLoader.swift index f4e4ec6506c..474b1b4adb0 100644 --- a/Sources/PackageLoading/ManifestLoader.swift +++ b/Sources/PackageLoading/ManifestLoader.swift @@ -291,23 +291,23 @@ public final class ManifestLoader: ManifestLoaderProtocol { // we were able to parse the manifest successfully. if let compilerOutput = result.compilerOutput { // FIXME: Isn't the judgement too naive? - var outputIndicatesWarning = !compilerOutput.isEmpty + var outputSeverity: Basics.Diagnostic.Severity = .warning #if os(Windows) // Filter out link.exe note when creating manifest executable. if let compiledManifestName = result.compiledManifestFile?.basenameWithoutExt, !compilerOutput.contains(where: \.isNewline) { if compilerOutput.contains("\(compiledManifestName).lib") && compilerOutput.contains("\(compiledManifestName).exp") { - outputIndicatesWarning = false + outputSeverity = .debug } } #endif - if outputIndicatesWarning { + if !compilerOutput.isEmpty { let metadata = result.diagnosticFile.map { diagnosticFile -> ObservabilityMetadata in var metadata = ObservabilityMetadata() metadata.manifestLoadingDiagnosticFile = diagnosticFile return metadata } - observabilityScope.emit(warning: compilerOutput, metadata: metadata) + observabilityScope.emit(severity: outputSeverity, message: compilerOutput, metadata: metadata) // FIXME: (diagnostics) deprecate in favor of the metadata version ^^ when transitioning manifest loader to Observability APIs //observabilityScope.emit(.warning(ManifestLoadingDiagnostic(output: compilerOutput, diagnosticFile: result.diagnosticFile))) From ca94d2494cf47e29f5955c0b4448c400ace9f3b1 Mon Sep 17 00:00:00 2001 From: stevapple Date: Thu, 8 Sep 2022 23:03:04 +0800 Subject: [PATCH 5/6] Minor update --- Sources/PackageLoading/ManifestLoader.swift | 26 ++++++++++----------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/Sources/PackageLoading/ManifestLoader.swift b/Sources/PackageLoading/ManifestLoader.swift index 474b1b4adb0..0c0980db452 100644 --- a/Sources/PackageLoading/ManifestLoader.swift +++ b/Sources/PackageLoading/ManifestLoader.swift @@ -290,28 +290,26 @@ public final class ManifestLoader: ManifestLoaderProtocol { // We might have some non-fatal output (warnings/notes) from the compiler even when // we were able to parse the manifest successfully. if let compilerOutput = result.compilerOutput { - // FIXME: Isn't the judgement too naive? + // FIXME: Isn't the strategy too naive? var outputSeverity: Basics.Diagnostic.Severity = .warning - #if os(Windows) - // Filter out link.exe note when creating manifest executable. +#if os(Windows) + // Filter out `LINK` note for creating manifest executable. if let compiledManifestName = result.compiledManifestFile?.basenameWithoutExt, !compilerOutput.contains(where: \.isNewline) { if compilerOutput.contains("\(compiledManifestName).lib") && compilerOutput.contains("\(compiledManifestName).exp") { outputSeverity = .debug } } - #endif - if !compilerOutput.isEmpty { - let metadata = result.diagnosticFile.map { diagnosticFile -> ObservabilityMetadata in - var metadata = ObservabilityMetadata() - metadata.manifestLoadingDiagnosticFile = diagnosticFile - return metadata - } - observabilityScope.emit(severity: outputSeverity, message: compilerOutput, metadata: metadata) - - // FIXME: (diagnostics) deprecate in favor of the metadata version ^^ when transitioning manifest loader to Observability APIs - //observabilityScope.emit(.warning(ManifestLoadingDiagnostic(output: compilerOutput, diagnosticFile: result.diagnosticFile))) +#endif + let metadata = result.diagnosticFile.map { diagnosticFile -> ObservabilityMetadata in + var metadata = ObservabilityMetadata() + metadata.manifestLoadingDiagnosticFile = diagnosticFile + return metadata } + observabilityScope.emit(severity: outputSeverity, message: compilerOutput, metadata: metadata) + + // FIXME: (diagnostics) deprecate in favor of the metadata version ^^ when transitioning manifest loader to Observability APIs + //observabilityScope.emit(.warning(ManifestLoadingDiagnostic(output: compilerOutput, diagnosticFile: result.diagnosticFile))) } return try ManifestJSONParser.parse( From 58e661a20dfc1ae43b383de75acc2ba6d641cf7d Mon Sep 17 00:00:00 2001 From: YR Chen Date: Wed, 14 Sep 2022 02:53:32 +0800 Subject: [PATCH 6/6] Explain for `FIXME` of `compilerOutput` processing --- Sources/PackageLoading/ManifestLoader.swift | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Sources/PackageLoading/ManifestLoader.swift b/Sources/PackageLoading/ManifestLoader.swift index 534f4f2442c..d292f9bbba8 100644 --- a/Sources/PackageLoading/ManifestLoader.swift +++ b/Sources/PackageLoading/ManifestLoader.swift @@ -290,7 +290,9 @@ public final class ManifestLoader: ManifestLoaderProtocol { // We might have some non-fatal output (warnings/notes) from the compiler even when // we were able to parse the manifest successfully. if let compilerOutput = result.compilerOutput { - // FIXME: Isn't the strategy too naive? + // FIXME: We shouldn't assume the compiler output to be a single piece. There could be combined + // output from different stages of compiling the manifest, but it's hard to distinguish them. + // A better approach might be teaching the driver to emit a structured log with context and severity. var outputSeverity: Basics.Diagnostic.Severity = .warning #if os(Windows) // Filter out `LINK` note for creating manifest executable.