Skip to content

convert the PackageLoading module to the new diagnostics apis #3754

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
Sep 17, 2021
Merged

convert the PackageLoading module to the new diagnostics apis #3754

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
43 changes: 24 additions & 19 deletions Sources/PackageLoading/ModuleMapGenerator.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,10 @@
See http://swift.org/CONTRIBUTORS.txt for Swift project authors
*/

import TSCBasic
import PackageModel
import Basics
import Foundation
import PackageModel
import TSCBasic

/// Name of the module map file recognized by the Clang and Swift compilers.
public let moduleMapFilename = "module.modulemap"
Expand Down Expand Up @@ -86,8 +87,14 @@ public struct ModuleMapGenerator {
}

/// Inspects the file system at the public-headers directory with which the module map generator was instantiated, and returns the type of module map that applies to that directory. This function contains all of the heuristics that implement module map policy for package targets; other functions just use the results of this determination.
public func determineModuleMapType(diagnostics: DiagnosticsEngine) -> ModuleMapType {
public func determineModuleMapType(observabilityScope: ObservabilityScope) -> ModuleMapType {
// The following rules are documented at https://github.com/apple/swift-package-manager/blob/master/Documentation/Usage.md#creating-c-language-targets. To avoid breaking existing packages, do not change the semantics here without making any change conditional on the tools version of the package that defines the target.

let diagnosticsEmitter = observabilityScope.makeDiagnosticsEmitter {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this part about creating a diagnostics emitter temporary until the old diagnostics engine has been removed, or will every client function create its own emitter in the future as well?

Copy link
Contributor Author

@tomerd tomerd Sep 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in most cases, code would emit directly from the ObservabilityScope, however, there are cases where you may want to customize metadata further without nesting another scope, which you can do in one of two ways: either create a DiagnosticsEmitter with additional metadata , or specific the additional metadata on the emit() call itself. in this case we choose the former since the code emit in many places so its more efficient to hold a emitter with metadata set.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. It will be interesting to see how this can be simplified if we have task locals or similar. It seems compelling to want to wrap a closure and add metadata to any diagnostics it emits without quite so much ceremony.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed. when we migrate the code to 5.5/5.6 we would be able to take advantage of task locals

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Right, it wasn't so much a commentary on this PR as thinking ahead. This looks good to me.

var metadata = ObservabilityMetadata()
metadata.targetName = self.targetName
return metadata
}

// First check for a custom module map.
let customModuleMapFile = publicHeadersDir.appending(component: moduleMapFilename)
Expand All @@ -97,7 +104,7 @@ public struct ModuleMapGenerator {

// Warn if the public-headers directory is missing. For backward compatibility reasons, this is not an error, we just won't generate a module map in that case.
guard fileSystem.exists(publicHeadersDir) else {
diagnostics.emit(.missingPublicHeadersDirectory(targetName: targetName, publicHeadersDir: publicHeadersDir))
diagnosticsEmitter.emit(.missingPublicHeadersDirectory(targetName: targetName, publicHeadersDir: publicHeadersDir))
return .none
}

Expand All @@ -108,7 +115,7 @@ public struct ModuleMapGenerator {
}
catch {
// This might fail because of a file system error, etc.
diagnostics.emit(.inaccessiblePublicHeadersDirectory(targetName: targetName, publicHeadersDir: publicHeadersDir, fileSystemError: error))
diagnosticsEmitter.emit(.inaccessiblePublicHeadersDirectory(targetName: targetName, publicHeadersDir: publicHeadersDir, fileSystemError: error))
return .none
}

Expand All @@ -122,7 +129,7 @@ public struct ModuleMapGenerator {
if fileSystem.isFile(umbrellaHeader) {
// In this case, 'PublicHeadersDir' is expected to contain no subdirectories.
if directories.count != 0 {
diagnostics.emit(.umbrellaHeaderHasSiblingDirectories(targetName: targetName, umbrellaHeader: umbrellaHeader, siblingDirs: directories))
diagnosticsEmitter.emit(.umbrellaHeaderHasSiblingDirectories(targetName: targetName, umbrellaHeader: umbrellaHeader, siblingDirs: directories))
return .none
}
return .umbrellaHeader(umbrellaHeader)
Expand All @@ -131,20 +138,20 @@ public struct ModuleMapGenerator {
/// Check for the common mistake of naming the umbrella header 'TargetName.h' instead of 'ModuleName.h'.
let misnamedUmbrellaHeader = publicHeadersDir.appending(component: targetName + ".h")
if fileSystem.isFile(misnamedUmbrellaHeader) {
diagnostics.emit(.misnamedUmbrellaHeader(misnamedUmbrellaHeader: misnamedUmbrellaHeader, umbrellaHeader: umbrellaHeader))
diagnosticsEmitter.emit(.misnamedUmbrellaHeader(misnamedUmbrellaHeader: misnamedUmbrellaHeader, umbrellaHeader: umbrellaHeader))
}

// If 'PublicHeadersDir/ModuleName/ModuleName.h' exists, then use it as the umbrella header.
let nestedUmbrellaHeader = publicHeadersDir.appending(components: moduleName, moduleName + ".h")
if fileSystem.isFile(nestedUmbrellaHeader) {
// In this case, 'PublicHeadersDir' is expected to contain no subdirectories other than 'ModuleName'.
if directories.count != 1 {
diagnostics.emit(.umbrellaHeaderParentDirHasSiblingDirectories(targetName: targetName, umbrellaHeader: nestedUmbrellaHeader, siblingDirs: directories.filter{ $0.basename != moduleName }))
diagnosticsEmitter.emit(.umbrellaHeaderParentDirHasSiblingDirectories(targetName: targetName, umbrellaHeader: nestedUmbrellaHeader, siblingDirs: directories.filter{ $0.basename != moduleName }))
return .none
}
// In this case, 'PublicHeadersDir' is also expected to contain no header files.
if headers.count != 0 {
diagnostics.emit(.umbrellaHeaderParentDirHasSiblingHeaders(targetName: targetName, umbrellaHeader: nestedUmbrellaHeader, siblingHeaders: headers))
diagnosticsEmitter.emit(.umbrellaHeaderParentDirHasSiblingHeaders(targetName: targetName, umbrellaHeader: nestedUmbrellaHeader, siblingHeaders: headers))
return .none
}
return .umbrellaHeader(nestedUmbrellaHeader)
Expand All @@ -153,7 +160,7 @@ public struct ModuleMapGenerator {
/// Check for the common mistake of naming the nested umbrella header 'TargetName.h' instead of 'ModuleName.h'.
let misnamedNestedUmbrellaHeader = publicHeadersDir.appending(components: moduleName, targetName + ".h")
if fileSystem.isFile(misnamedNestedUmbrellaHeader) {
diagnostics.emit(.misnamedUmbrellaHeader(misnamedUmbrellaHeader: misnamedNestedUmbrellaHeader, umbrellaHeader: nestedUmbrellaHeader))
diagnosticsEmitter.emit(.misnamedUmbrellaHeader(misnamedUmbrellaHeader: misnamedNestedUmbrellaHeader, umbrellaHeader: nestedUmbrellaHeader))
}

// Otherwise, if 'PublicHeadersDir' contains only header files and no subdirectories, use it as the umbrella directory.
Expand Down Expand Up @@ -197,7 +204,6 @@ public enum GeneratedModuleMapType {
case umbrellaDirectory(AbsolutePath)
}


public extension ModuleMapType {
/// Returns the type of module map to generate for this kind of module map, or nil to not generate one at all.
var generatedModuleMapType: GeneratedModuleMapType? {
Expand All @@ -209,36 +215,35 @@ public extension ModuleMapType {
}
}


private extension Diagnostic.Message {
private extension Basics.Diagnostic {

/// Warning emitted if the public-headers directory is missing.
static func missingPublicHeadersDirectory(targetName: String, publicHeadersDir: AbsolutePath) -> Diagnostic.Message {
static func missingPublicHeadersDirectory(targetName: String, publicHeadersDir: AbsolutePath) -> Self {
.warning("no include directory found for target '\(targetName)'; libraries cannot be imported without public headers")
}

/// Error emitted if the public-headers directory is inaccessible.
static func inaccessiblePublicHeadersDirectory(targetName: String, publicHeadersDir: AbsolutePath, fileSystemError: Error) -> Diagnostic.Message {
static func inaccessiblePublicHeadersDirectory(targetName: String, publicHeadersDir: AbsolutePath, fileSystemError: Error) -> Self {
.error("cannot access public-headers directory for target '\(targetName)': \(String(describing: fileSystemError))")
}

/// Warning emitted if a misnamed umbrella header was found.
static func misnamedUmbrellaHeader(misnamedUmbrellaHeader: AbsolutePath, umbrellaHeader: AbsolutePath) -> Diagnostic.Message {
static func misnamedUmbrellaHeader(misnamedUmbrellaHeader: AbsolutePath, umbrellaHeader: AbsolutePath) -> Self {
.warning("\(misnamedUmbrellaHeader) should be renamed to \(umbrellaHeader) to be used as an umbrella header")
}

/// Error emitted if there are directories next to a top-level umbrella header.
static func umbrellaHeaderHasSiblingDirectories(targetName: String, umbrellaHeader: AbsolutePath, siblingDirs: Set<AbsolutePath>) -> Diagnostic.Message {
static func umbrellaHeaderHasSiblingDirectories(targetName: String, umbrellaHeader: AbsolutePath, siblingDirs: Set<AbsolutePath>) -> Self {
.error("target '\(targetName)' has invalid header layout: umbrella header found at '\(umbrellaHeader)', but directories exist next to it: \(siblingDirs.map({ String(describing: $0) }).sorted().joined(separator: ", ")); consider removing them")
}

/// Error emitted if there are other directories next to the parent directory of a nested umbrella header.
static func umbrellaHeaderParentDirHasSiblingDirectories(targetName: String, umbrellaHeader: AbsolutePath, siblingDirs: Set<AbsolutePath>) -> Diagnostic.Message {
static func umbrellaHeaderParentDirHasSiblingDirectories(targetName: String, umbrellaHeader: AbsolutePath, siblingDirs: Set<AbsolutePath>) -> Self {
.error("target '\(targetName)' has invalid header layout: umbrella header found at '\(umbrellaHeader)', but more than one directory exists next to its parent directory: \(siblingDirs.map({ String(describing: $0) }).sorted().joined(separator: ", ")); consider reducing them to one")
}

/// Error emitted if there are other headers next to the parent directory of a nested umbrella header.
static func umbrellaHeaderParentDirHasSiblingHeaders(targetName: String, umbrellaHeader: AbsolutePath, siblingHeaders: Set<AbsolutePath>) -> Diagnostic.Message {
static func umbrellaHeaderParentDirHasSiblingHeaders(targetName: String, umbrellaHeader: AbsolutePath, siblingHeaders: Set<AbsolutePath>) -> Self {
.error("target '\(targetName)' has invalid header layout: umbrella header found at '\(umbrellaHeader)', but additional header files exist: \((siblingHeaders.map({ String(describing: $0) }).sorted().joined(separator: ", "))); consider reducing them to one")
}
}
2 changes: 1 addition & 1 deletion Sources/PackageLoading/PackageBuilder.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -885,7 +885,7 @@ public final class PackageBuilder {

if fileSystem.exists(publicHeadersPath) {
let moduleMapGenerator = ModuleMapGenerator(targetName: potentialModule.name, moduleName: potentialModule.name.spm_mangledToC99ExtendedIdentifier(), publicHeadersDir: publicHeadersPath, fileSystem: fileSystem)
moduleMapType = moduleMapGenerator.determineModuleMapType(diagnostics: ObservabilitySystem.topScope.makeDiagnosticsEngine())
moduleMapType = moduleMapGenerator.determineModuleMapType(observabilityScope: self.observabilityScope)
} else if targetType == .library, manifest.toolsVersion >= .v5_5 {
// If this clang target is a library, it must contain "include" directory.
throw ModuleError.invalidPublicHeadersDirectory(potentialModule.name)
Expand Down
41 changes: 31 additions & 10 deletions Tests/PackageLoadingTests/ModuleMapGenerationTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,12 +93,17 @@ class ModuleMapGeneration: XCTestCase {
}

func testWarnings() throws {
var fs = InMemoryFileSystem(emptyFiles:
"/Foo.c")
var fs = InMemoryFileSystem(emptyFiles: "/Foo.c")
ModuleMapTester("Foo", in: fs) { result in
result.checkNotCreated()
result.checkDiagnostics { result in
result.check(diagnostic: "no include directory found for target \'Foo\'; libraries cannot be imported without public headers", severity: .warning)
var expectedMetadata = ObservabilityMetadata()
expectedMetadata.targetName = "Foo"
result.check(
diagnostic: "no include directory found for target \'Foo\'; libraries cannot be imported without public headers",
severity: .warning,
metadata: expectedMetadata
)
}
}

Expand All @@ -114,21 +119,31 @@ class ModuleMapGeneration: XCTestCase {

""")
result.checkDiagnostics { result in
result.check(diagnostic: "/include/F-o-o.h should be renamed to /include/F_o_o.h to be used as an umbrella header", severity: .warning)
var expectedMetadata = ObservabilityMetadata()
expectedMetadata.targetName = "F-o-o"
result.check(
diagnostic: "/include/F-o-o.h should be renamed to /include/F_o_o.h to be used as an umbrella header",
severity: .warning,
metadata: expectedMetadata
)
}
}
}

func testUnsupportedLayouts() throws {
var fs: InMemoryFileSystem

fs = InMemoryFileSystem(emptyFiles:
var fs = InMemoryFileSystem(emptyFiles:
"/include/Foo/Foo.h",
"/include/Bar/Foo.h")
ModuleMapTester("Foo", in: fs) { result in
result.checkNotCreated()
result.checkDiagnostics { result in
result.check(diagnostic: "target 'Foo' has invalid header layout: umbrella header found at '/include/Foo/Foo.h', but more than one directory exists next to its parent directory: /include/Bar; consider reducing them to one", severity: .error)
var expectedMetadata = ObservabilityMetadata()
expectedMetadata.targetName = "Foo"
result.check(
diagnostic: "target 'Foo' has invalid header layout: umbrella header found at '/include/Foo/Foo.h', but more than one directory exists next to its parent directory: /include/Bar; consider reducing them to one",
severity: .error,
metadata: expectedMetadata
)
}
}

Expand All @@ -138,7 +153,13 @@ class ModuleMapGeneration: XCTestCase {
ModuleMapTester("Foo", in: fs) { result in
result.checkNotCreated()
result.checkDiagnostics { result in
result.check(diagnostic: "target 'Foo' has invalid header layout: umbrella header found at '/include/Foo.h', but directories exist next to it: /include/Bar; consider removing them", severity: .error)
var expectedMetadata = ObservabilityMetadata()
expectedMetadata.targetName = "Foo"
result.check(
diagnostic: "target 'Foo' has invalid header layout: umbrella header found at '/include/Foo.h', but directories exist next to it: /include/Bar; consider removing them",
severity: .error,
metadata: expectedMetadata
)
}
}
}
Expand All @@ -149,7 +170,7 @@ func ModuleMapTester(_ targetName: String, includeDir: String = "include", in fi
let observability = ObservabilitySystem.bootstrapForTesting()
// Create a module map generator, and determine the type of module map to use for the header directory. This may emit diagnostics.
let moduleMapGenerator = ModuleMapGenerator(targetName: targetName, moduleName: targetName.spm_mangledToC99ExtendedIdentifier(), publicHeadersDir: AbsolutePath.root.appending(component: includeDir), fileSystem: fileSystem)
let moduleMapType = moduleMapGenerator.determineModuleMapType(diagnostics: ObservabilitySystem.topScope.makeDiagnosticsEngine())
let moduleMapType = moduleMapGenerator.determineModuleMapType(observabilityScope: ObservabilitySystem.topScope)

// Generate a module map and capture any emitted diagnostics.
let generatedModuleMapPath = AbsolutePath.root.appending(components: "module.modulemap")
Expand Down