[ModuleMaps] C wrapper library requires custom module map in Swift 5.2 [SR-12758] #2813
Conversation
|
@swift-ci please smoke test |
abertelrud
changed the title
| 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: .empty) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I would not, for example, want to add another bool to the Target() declaration to say whether or not a module is expected.
|
Does the documentation and/or the implementation need to account for C++ and/or ObjC++? IIRC, both do not support modules at all, but I think when we say "C language targets" in the docs, we actually mean C-family, right? |
Yes, the documentation is a bit murky here. I thought that Clang modules do not support C++ and Objective-C++, but I was recently corrected in the comments for another PR; it's just that Swift can't import C++ and Objective-C++. I considered whether checking for ".h" files is the right thing here, but decided that would be too risky and anyway an orthogonal change to make. But the documentation should at least be changed. Yes, Clang targets can include C, C++, Objective-C, Objective-C++, and Assembler source files. |
|
After some more discussion, I think the first right thing to do is to add the warning but not change the behaviour, and to nominate that for Swift 5.3. This will at least call out the solution of either making the headers conform to the documentation or to add a custom module map. Then a later change (not nominated to Swift 5.3) would change the behaviour. The concern is that we might otherwise end up breaking packages that don't fully conform to the documented semantics but that just happen to work today. |
|
After discussion, I've opened a separate PR at #2815 to add just the warning but keep the behaviour the same. This would then be nominated for 5.3 to make the problem clearer without risking breaking existing packages. Once that PR has landed I will rebase this one on it for changing the behavior. |
… map in Swift 5.2 The problem here is actually that the logic in SwiftPM always creates a module map with either an umbrella header or an umbrella directory for every C target, even though the documentation says that a module map will only be created for a C target under particular conditions. According to https://github.com/apple/swift-package-manager/blob/master/Documentation/Usage.md#creating-c-language-targets, here are the rules: > Swift Package Manager will automatically generate a modulemap for each > C language library target for these 3 cases: > > • If “include/Foo/Foo.h” exists and “Foo” is the only directory under the > “include” directory, then “include/Foo/Foo.h” becomes the umbrella header. > > • If “include/Foo.h” exists and “include” contains no other subdirectory then > “include/Foo.h” becomes the umbrella header. > > • Otherwise, if the “include” directory only contains header files and no > other subdirectory, it becomes the umbrella directory. > > In case of complicated include layouts, a custom module.modulemap can be > provided inside “include”. SwiftPM will error out if it can not generate > a modulemap with respect to the above rules. But the problem is that the third of those bullet points is not how the logic works. Any header layout that doesn't match one of the first two cases automatically became an umbrella directory in the generated module map. This bug was masked because module map files weren’t passed from one C target to another until https://bugs.swift.org/browse/SR-10707 was fixed, at which point this started breaking. For C targets whose module maps were passed to Swift targets, it was always broken (but only surfaced as errors from the compiler as a result of trying to build a module from the non-modularized headers, not surfaced as errors from SwiftPM about an incorrect header layout). We don’t want to undo the fix for SR-10707, nor is it right to tie this behavior to particular Swift tools versions, since that only postpones the problem. It’s perfectly legitimate to want to wrap an unmodified C or C++ library (with non-modularized header layout) in an authored C wrapper target that then presents a Swift-friendly modular interface to Swift targets, so non-modular C-to-C target imports should continue to be supported. The code also alludes to this, at BuildPlan.swift:409 in current main branch: > // FIXME: We should probably only warn if we're unable to generate the > // modulemap because the clang target is still a valid, it just can't be > // imported from Swift targets. This is above the line that does a `try` that ends up passing on any errors if there’s a problem with the layout. After having gone through the code, and discussing this in some detail, and trying a couple of different approaches, I think the right thing to do is: 1. Fix the logic to only create an umbrella directory module map in the cases that are spelled out in the third bullet of the documentation (i.e. match the documented behavior). 2. But emit a warning, not an error, suggesting that the target add a custom module map. If we emitted an error as the documentation said, we would break a lot of packages. 3. Update the documentation to account for one other case that’s already being checked in the logic for case 1, which is that there are no headers next to the umbrella directory. Instead of not generating a module map at all, we generate an empty module map (along with emitting the warning) in the cases in which an incorrect umbrella directory would previously have been generated into the module map. This has the least impact on the downstream logic.
abertelrud
force-pushed
the
eng/SR-12758-module-maps-with-umbrella-headers-always-generated
branch
from
12f1ae1
to
1d08df1
Compare
|
@swift-ci please smoke test |
|
Superseded by #2815 |
The problem here is actually that the logic in SwiftPM always creates a module map with either an umbrella header or an umbrella directory for every C target, even though the documentation says that a module map will only be created for a C target under particular conditions. According to https://github.com/apple/swift-package-manager/blob/master/Documentation/Usage.md#creating-c-language-targets, here are the rules:
But the problem is that the third of those bullet points is not how the logic works. Any header layout that doesn't match one of the first two cases automatically became an umbrella directory in the generated module map.
This bug was masked because module map files weren’t passed from one C target to another until https://bugs.swift.org/browse/SR-10707 was fixed, at which point this started breaking. For C targets whose module maps were passed to Swift targets, it was always broken (but only surfaced as errors from the compiler as a result of trying to build a module from the non-modularized headers, not surfaced as errors from SwiftPM about an incorrect header layout).
We don’t want to undo the fix for SR-10707, nor is it right to tie this behavior to particular Swift tools versions, since that only postpones the problem. It’s perfectly legitimate to want to wrap an unmodified C or C++ library (with non-modularized header layout) in an authored C wrapper target that then presents a Swift-friendly modular interface to Swift targets, so non-modular C-to-C target imports should continue to be supported. The code also alludes to this, at BuildPlan.swift:409 in current main branch:
This is above the line that does a
trythat ends up passing on any errors if there’s a problem with the layout.After having gone through the code, and discussing this in some detail, and trying a couple of different approaches, I think the right thing to do is:
Fix the logic to only create an umbrella directory module map in the cases that are spelled out in the third bullet of the documentation (i.e. match the documented behavior).
But emit a warning, not an error, suggesting that the target add a custom module map. If we emitted an error as the documentation said, we would break a lot of packages.
Update the documentation to account for one other case that’s already being checked in the logic for case 1, which is that there are no headers next to the umbrella directory.
Instead of not generating a module map at all, we generate an empty module map (along with emitting the warning) in the cases in which an incorrect umbrella directory would previously have been generated into the module map. This has the least impact on the downstream logic.