CXX-Interop: Fix missing header deps edge #9
Conversation
|
@afabri, I think this should fix the issue. I've tied the header generation into the target itself a little more tightly and am now seeing the header recognized as one of the dependencies in the generated dependency file from clang. I'm also seeing that modifying the Swift file marks the header as dirty, which then cascades to the C++ file and onward resulting in a full rebuild as expected. |
etcwilde
force-pushed
the
ewilde/fix-cxx-interop-deps-graph
branch
from
25f01fe to
bef44f8
Compare
| cmake_parse_arguments(ARG "" "" "SEARCH_PATHS;MODULE_NAME" ${ARGN}) | ||
|
|
||
| if(NOT ARG_MODULE_NAME) | ||
| set(ARG_MODULE_NAME $<TARGET_PROPERTY:${target},Swift_MODULE_NAME>) |
There was a problem hiding this comment.
I think there should be a validation of ${ARG_MODULE_NAME}.
I do not know if all strings are a valid swift module names, but I imagine it is not the case.
At the very least, the code should message(FATAL_ERROR ...) if ARG_MODULE_NAME is empty (when both the argument to the function and the target property are empty or not defined).
There was a problem hiding this comment.
Validation of the module name at the cmake level would be more brittle than allowing the compiler to determine what it considers to be a valid module name than trying to replicate the logic here.
As for the empty case, that's a good point. If unset, CMake will default the module name to the name of the target, so I'll do the same with the generated header to match.
| COMMENT | ||
| "Generating '${header}'" |
The custom target dependency was an order-only dependency edge. As a result, changing the swift file wouldn't result in the header getting regenerated at the right time because nothing actually depended on it being up-to-date. This patch should fix that and ties it into the `fibonacci` target a little more cleanly. The generated header is marked as being one of the target sources, so the target itself depends on the header getting built. This ensures that the C++ file gets rebuilt if the header changes. The header still depends on the Swift sources in the target. We're now extracting that list directly from the target itself instead of needing to pass it in explicitly. The module is also extracted from the target itself. We're also automatically adding the location of the header include directory so that dependees can find it as part of the target interface. Fixes apple#8
etcwilde
force-pushed
the
ewilde/fix-cxx-interop-deps-graph
branch
from
bef44f8 to
51d1ba4
Compare
The custom target dependency was an order-only dependency edge. As a result, changing the swift file wouldn't result in the header getting regenerated at the right time because nothing actually depended on it being up-to-date. This patch should fix that and ties it into the
fibonaccitarget a little more cleanly. The generated header is marked as being one of the target sources, so the target itself depends on the header getting built. This ensures that the C++ file gets rebuilt if the header changes. The header still depends on the Swift sources in the target. We're now extracting that list directly from the target itself instead of needing to pass it in explicitly. The module is also extracted from the target itself. We're also automatically adding the location of the header include directory so that dependees can find it as part of the target interface.Fixes #8