[cmake-developers] Generator Expression self-references now allowed?

[Stephen Kelly]

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.