Skip to content

[ModuleMaps] C wrapper library requires custom module map in Swift 5.2 [SR-12758] #2813

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

Closed
wants to merge 1 commit into from
Closed

[ModuleMaps] C wrapper library requires custom module map in Swift 5.2 [SR-12758] #2813

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
11 changes: 7 additions & 4 deletions Sources/PackageLoading/ModuleMapGenerator.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,12 +143,12 @@ public struct ModuleMapGenerator {
return
}

// Otherwise, the target's public headers are considered to be incompatible with modules. Other C targets can still import them, but Swift won't be able to see them. This is documented as an error, but because SwiftPM has previously allowed it (creating module maps that then cause errors when used), we instead emit a warning and for now, continue to emit what SwiftPM has historically emitted (an umbrella directory include).
// Otherwise, the target's public headers are considered to be incompatible with modules. Other C targets can still import them, but Swift won't be able to see them. This is documented as an error, but because SwiftPM has previously allowed it (creating module maps that then cause errors when used), we instead emit a warning.
warningStream <<< "warning: the include directory of target '\(target.name)' has "
warningStream <<< "a layout that is incompatible with modules; consider adding a "
warningStream <<< "custom module map to the target"
warningStream <<< "a layout that is incompatible with modules; no module map will "
warningStream <<< "be generated; consider adding a custom module map to the target"
warningStream.flush()
try createModuleMap(inDir: wd, type: .directory(includeDir))
try createModuleMap(inDir: wd, type: .empty)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we consider making this warning conditional on the used languages and/or the root package? Presumably, if I am building an all-C package graph, I might not care about modules. That has the danger of shipping a C-only package that is not compatible with Swift, so it might make sense to still emit it for the root package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since the documentation states that it is an error for packages to not have their headers in a module compatible layout, it seems prudent to at least emit the warning so that package authors can add a custom module map.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that depends on what we want the future behaviour to be. It seems like we never implemented the error and now non-modular packages exist. Is this considered just something we support for backwards-compatibility and this would become an error in the future? If not, I think there needs be some way to not get the warning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Absolutely, agreed, there needs to be a way to not get the warning, but I think that way would be to add a custom module map to the target. The warning here is phrased similarly to the other warnings this function can emit, and in each case there should be wording to indicate how to avoid the warning. The problem now is that packages just fail to build and there's no warning about why.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would not, for example, want to add another bool to the Target() declaration to say whether or not a module is expected.

}

/// Warn user if in case target name and c99name are different and there is a
Expand All @@ -164,6 +164,7 @@ public struct ModuleMapGenerator {
}

private enum UmbrellaType {
case empty
case header(AbsolutePath)
case directory(AbsolutePath)
}
Expand All @@ -172,6 +173,8 @@ public struct ModuleMapGenerator {
let stream = BufferedOutputByteStream()
stream <<< "module \(target.c99name) {\n"
switch type {
case .empty:
break
case .header(let header):
stream <<< " umbrella header \"\(header.pathString)\"\n"
case .directory(let path):
Expand Down
3 changes: 1 addition & 2 deletions Tests/PackageLoadingTests/ModuleMapGenerationTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,12 +111,11 @@ class ModuleMapGeneration: XCTestCase {
"/Foo.c")
let expected2 = BufferedOutputByteStream()
expected2 <<< "module Foo {\n"
expected2 <<< " umbrella \"/include\"\n"
expected2 <<< " export *\n"
expected2 <<< "}\n"
ModuleMapTester("Foo", in: fs) { result in
result.check(value: expected2.bytes)
result.checkDiagnostics("warning: the include directory of target \'Foo\' has a layout that is incompatible with modules; consider adding a custom module map to the target")
result.checkDiagnostics("warning: the include directory of target \'Foo\' has a layout that is incompatible with modules; no module map will be generated; consider adding a custom module map to the target")
}
}

Expand Down