Brad King wrote:
> Can't you instead simply have the usage requirements skip appending
> a target's own requirements to itself? That way the resulting
> generator expression would still not have a self reference.
Yes, but I only realized that after replying to the rest of the mail :).
As it has some useful information, I've left the rest of the reply intact.
> Steve,
>
> Can you explain the need for 953def1e17f3bbba0aa42037ae15ced011d8fd2a:
>
> Fix DAG checker finding cycling dependencies.
>
> Before this patch, the following is reported falsely as a
> self-reference:
>
> target_link_libraries(empty2 LINK_PUBLIC empty3)
> target_link_libraries(empty3 LINK_PUBLIC empty2)
>
> add_custom_target(...
> -DINCLUDES=$<TARGET_PROPERTY:empty2,INTERFACE_INCLUDE_DIRECTORIES>
> )
Before the patch, this was not allowed:
add_library(foo ...)
add_library(bat ...)
set_property(TARGET foo PROPERTY
INCLUDE_DIRECTORIES
$<TARGET_PROPERTY:bat,INTERFACE_INCLUDE_DIRECTORIES>
)
set_property(TARGET bat PROPERTY
INTERFACE_INCLUDE_DIRECTORIES
$<TARGET_PROPERTY:bat,INTERFACE_INCLUDE_DIRECTORIES>
)
Note that the INTERFACE_INCLUDE_DIRECTORIES of bat depends on its own
INTERFACE_INCLUDE_DIRECTORIES.
The generator expressions allow cycles involving another node, but disallow
direct self-loops. So this is allowed:
add_library(foo ...)
add_library(bat ...)
add_library(zar ...)
set_property(TARGET foo PROPERTY
INCLUDE_DIRECTORIES
$<TARGET_PROPERTY:bat,INTERFACE_INCLUDE_DIRECTORIES>
)
set_property(TARGET bat PROPERTY
INTERFACE_INCLUDE_DIRECTORIES
$<TARGET_PROPERTY:zar,INTERFACE_INCLUDE_DIRECTORIES>
)
set_property(TARGET zar PROPERTY
INTERFACE_INCLUDE_DIRECTORIES
$<TARGET_PROPERTY:bat,INTERFACE_INCLUDE_DIRECTORIES>
)
The foo INCLUDE_DIRECTORIES will include the interface includes from zar and
bat, and there is no error. The INCLUDE_DIRECTORIES of foo are determined by
cmTarget::GetIncludeDirectories, which creates a DAGChecker recording that
the graph starts with target=foo,property=INCLUDE_DIRECTORIES.
Similarly, the INCLUDE_DIRECTORIES of bat are determined by creating a
DagChecker with target=bat,property=INCLUDE_DIRECTORIES, following to zar,
and then to target=bat,property=INTERFACE_INCLUDE_DIRECTORIES and and
aborting when target=zar,property=INTERFACE_INCLUDE_DIRECTORIES is
encountered again. Because there is still another dagchecker parent (the one
created in cmTarget::GetIncludeDirectories), this is correctly determined to
be a cycle by cmGeneratorExpressionDAGChecker::checkGraph.
(target -- property -- parentDagChecker)
bar -- INTERFACE_INCLUDE_DIRECTORIES -- 0x7fff5b38efd0
zat -- INTERFACE_INCLUDE_DIRECTORIES -- 0x7fff5b38f750
bar -- INTERFACE_INCLUDE_DIRECTORIES -- 0x7fff5b38edd0
foo -- INCLUDE_DIRECTORIES -- 0x0 # null parent, abort
In the case of evaluating a generator expression which reads the
INCLUDE_DIRECTORIES of bar, such as in a add_custom_target invocation,
things work similarly, but the 'top' dag checker is created in
TargetProprtyNode.
add_custom_command(...
$<TARGET_PROPERTY:bar,INCLUDE_DIRECTORIES>
...)
cmTarget::GetIncludeDirectories is not called.
The case where the INTERFACE includes are used in a genex is not the same
though:
add_custom_command(...
$<TARGET_PROPERTY:bar,INTERFACE_INCLUDE_DIRECTORIES>
...)
Here the top DagChecker is created with
target=bar,property=INTERFACE_INCLUDE_DIRECTORIES, and again the zat
dependency is followed, which reaches
target=bar,property=INTERFACE_INCLUDE_DIRECTORIES again. From that point,
the DagCheckers are traversed up:
(target -- property -- parentDagChecker)
bar -- INTERFACE_INCLUDE_DIRECTORIES -- 0x7fff5b38efd0
zat -- INTERFACE_INCLUDE_DIRECTORIES -- 0x7fff5b38f750
bar -- INTERFACE_INCLUDE_DIRECTORIES -- 0x0
Previously, the code assumed incorrectly that there would always be a
INCLUDE_DIRECTORIES node as the parent at the top, because it assumed that
INCLUDE_DIRECTORIES would only be read through GetIncludeDirectories. So the
parent check was reasonable at the time. Logically though, it is not
correct.
Meanwhile, there was another bug that the link interface libraries were not
queried for includes and defines of bar in cases like
add_custom_command(...
$<TARGET_PROPERTY:bar,INTERFACE_INCLUDE_DIRECTORIES>
...)
because the code which reads the link interface is only entered if the
parent DagChecker is non-null. That is fixed in
a10539b91bcd832c51bc8549e000d530d67de4cb.
However, as the fix processes the link interface of targets, and as that
forms a graph from which we get includes, and as the LinkInterfaceLoop test
has a self-reference which is represented in the graph of DagCheckers, the
DAG check() correctly now finds the self-reference.
So, if the self-reference is allowed in the LINK_INTERFACE_LIBRARIES of a
target, it must also be allowed in the INTERFACE_INCLUDE_DIRECTORIES of a
target.
Hopefully the above is clear. I've updated the branch on my clone with some
logging output.
> ? The LinkInterfaceLoop test was added here:
>
> http://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=8e756d2b
>
> to cover a case that was accidentally allowed but that should not be used.
> I'd rather not drop this great safety check for backward-compatibility
> with bad code.
Perhaps a policy can be added for that?
Thanks,
Steve.
--
Powered by www.kitware.com
Visit other Kitware open-source projects at
http://www.kitware.com/opensource/opensource.html
Please keep messages on-topic and check the CMake FAQ at:
http://www.cmake.org/Wiki/CMake_FAQ
Follow this link to subscribe/unsubscribe:
http://public.kitware.com/cgi-bin/mailman/listinfo/cmake-developers
