[5.3] Add a warning for cases in which SwiftPM generates a module map for unsupported header layouts #2819
Conversation
abertelrud
requested review from
ankitspd,
friedbunny,
neonichu and
rballard
as code owners
|
@swift-ci please test |
|
This looks like it's picked up an unrelated change in |
No, not expected at all. Thanks for pointing that out! I'll see what happened and fix it. |
… for unsupported header layouts 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 for 5.3 is to keep the current behavior but emit a warning asking package authors to add a custom module map. We also 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. Suggesting to use a custom module map for the cases in which the package target doesn't conform to the documented layout seems like the right thing even for packages that currently happen to work. rdar://65751265 (cherry picked from commit fc0a871)
abertelrud
force-pushed
the
eng/warn-about-clang-targets-with-nonmodular-header-layout-5.3
branch
from
7cfae97
to
e8541ed
Compare
|
@swift-ci please test |
Fixed it. Looks like a cherry pick error. Thanks for noticing it! |
| } | ||
|
|
||
| // 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). | ||
| warningStream <<< "warning: the include directory of target '\(target.name)' has " |
There was a problem hiding this comment.
This is writing to stdout when used by sourcekit-lsp, which breaks us. I should be able to fix sourcekit-lsp to not let you write to stdout, but this is bad behaviour for a library in general. It should at least be on stderr.
There was a problem hiding this comment.
I totally agree. I found that this file was doing this already, so I was following suit, but I am preparing the PR to switch to DiagnosticsEngine as a separate PR. I didn't realize that this change would cause breakage though (I guess any of the existing uses of warningStream would as well — I thought they were pointed at stderr and not stdout).
|
Ankit pointed out that the original Swift evolution proposal was amended so that an umbrella directory is created in all the fallback cases: #219 (comment). I will amend this PR to instead change the documentation to match. |
|
Actually, that was not an evolution proposal link at all, but a PR comment. Still, since the semantics have been unchanged here for four years, I'm closing this PR and will be opening a new one to correct the documentation. |
This is the SwiftPM 5.3 nomination for a change that has landed in master.
There is a much more detailed description in #2815, but the basic summary is that SwiftPM has up until now accepted some public-header layouts that were not valid according to the documentation. This sometimes causes problems, and it's not clear how to fix them.
For master there will be an upcoming change to tighten the rules for when SwiftPM generates a module map, but for SwiftPM 5.3 it would be good to at least add the warning for the out-of-spec targets, suggesting that the package authors add a custom module map to declare their intent.