[cmake-developers] Generator expressisons in target properties (was Re: conditionals in generator expressions)

[Stephen Kelly]

Brad King wrote:

> On 10/22/2012 01:24 PM, Stephen Kelly wrote:
>> That's in the use-generator-target-2 branch in my clone.
>> 
>> git://gitorious.org/~steveire/cmake/steveires-cmake.git
>> 
>> I'll rebase and submit the contents of it in several parallel branches
>> when 2.8.11 opens. I'll also fix the style issues in the commits etc.
> 
> Thanks.  If possible please re-organize the topic to make as many of the
> refactoring changes (e.g. moves to cmGeneratorTarget) to the front without
> any functional changes.  That way we can merge/test those first.

Most of the moves to cmGeneratorTarget depend on the earlier commits. Eg, 
'Port cmExportBuildFileGenerator to cmGeneratorTarget.' can't be committed 
until cmExportBuildFileGenerator is executed at generate-time. 'Move 
GetLinkInterface to cmGeneratorTarget.' can't be committed until 'Use a new 
code path to compute target location at configure-time.' is committed.

The section from 'Add a null check to the generator target accessor.' to 
'Move TraceDependencies to cmGeneratorTarget.' does not have dependencies 
though, and is one of the sections I plan to submit in parallel when 2.8.11 
opens.

> 
>> Also in my clone is the interface-target-properties branch. The commits
>> there 'finish' the work needed to make linking work with generator
>> expressions.
> 
> The cmComputeTargetDepends cannot be per-config because IDE generators use
> one set of inter-target dependencies for all configurations.  

Ah, I was wondering about that when I wrote that patch, but forgot to put in 
the email. I thought they would be able to have per-config dependencies.

 set_property(TARGET foo LINK_LIBRARIES $<$<Config:Debug>:debughelpers>

will never work with IDE generators? That's not a limitation of the 
generators but of the IDEs?

>> The way I've implemented it there is to populate a LINK_LIBRARIES
>> property as a result of calling target_link_libraries. The
>> INTERFACE_LINK_LIBRARIES property can also be populated on a target, and
>> it 'chains', which means that the INTERFACE_LINK_LIBRARIES from the
>> dependency tree are all processed, and the DAG checker ensures there are
>> no cycles in their content.
>> 
>> That chaining is whitelisted in the cmGeneratorExpressionEvaluator
>> because it is necessary to set up a DAG checker before evaluating. So,
>> the properties that are whitelisted are the properties that always get a
>> DAG checker set on them before they are evaluated.
>> 
>> When linking to a target, the INTERFACE_LINK_LIBRARIES are automatically
>> used too. The content of the INTERFACE_LINK_LIBRARIES is assumed to be
>> mandatory, so this makes sense.
> 
> Okay.  I'd prefer the term "transitive" to "chains".  The latter is
> already used in our source for a different meaning (target prop chains to
> dir prop).

That's fine with me.

> 
>> For backwards compatibility, the debug and optimized keywords are also
>> processed into generator expressions using $<CONFIG:...>.
> 
> Nice.  However:
> 
> +  else if (llt == OPTIMIZED)
> +    {
> +    l = std::string("$<CONFIG:Release:") + l + std::string(">");
> +    }
> 
> The "optimized" keyword is not just for Release.  It is really "not
> debug". Look at how the "llt" is used elsewhere to be sure.

Ok, that should be workable too with $<NOT>. I'll have a look at the other 
uses of llt.

> 
>> A side-effect of using generator expressions in the LINK_LIBRARIES
>> property in this way is that all libraries will appear to be in the link
>> implementation, and may appear again when processing the link interface.
>> I don't know the consequences of that, if any.
> 
> Can you explain that in more detail, please?


 add_library(foo ...)
 add_library(bar ...)
 add_library(blub ...)
 add_executable(exe ...)

 target_link_libraries(bar LINK_PUBLIC foo)
 # bar LINK_LIBRARIES is now:
 #   "foo;$<TARGET_PROPERTY:foo,INTERFACE_LINK_LIBRARIES>"
 # bar INTERFACE_LINK_LIBRARIES is now:
 #   "foo;$<TARGET_PROPERTY:foo,INTERFACE_LINK_LIBRARIES>"

 target_link_libraries(blub LINK_PUBLIC bar)
 # blub LINK_LIBRARIES is now
 #   "bar;$<TARGET_PROPERTY:bar,INTERFACE_LINK_LIBRARIES>"
 # blub INTERFACE_LINK_LIBRARIES is now
 #   "bar;$<TARGET_PROPERTY:bar,INTERFACE_LINK_LIBRARIES>"

 target_link_libraries(exe LINK_PUBLIC blub)
 # exe LINK_LIBRARIES is now
 #   "blub;$<TARGET_PROPERTY:blub,INTERFACE_LINK_LIBRARIES>"


In the interface-target-properties branch, 
cmTarget::GetOriginalLinkLibraries(const char *) for the 'exe' target will 
return 'blub;bar;foo'.

It's my understanding from reading the code that before generator 
expressions become available in the LINK_LIBRARIES property, it would return 
only 'blub'. The other libraries would be retrieved through the 
LINK_INTERFACE_LIBRARIES in cmTarget::ComputeLinkInterface and populate the 
LinkInterface.Libraries and a dependency graph would be calculated for 
'exe'.

In my branch, the LinkInterface.Libraries would probably still get the same 
content as before, but now everything in there is a duplicate of what's 
already in the 'original' link libraries. That have consequences, but I'm 
not certain.

>> I've also come around to the idea that
>> 
>>  target_link_libraries(foo bar)
>> 
>> should be enough to make foo compile and link with bar
> 
> Your justification is reasonable.  The code that adds INTERFACE_
> properties to 'bar' in the application could be made conditional if
> necessary anyway for compatibility.
> 

Yes, I've been considering a policy for this. I would prefer not to let it 
be more granularly conditional than that.

>> At this point, because multiple properties can use generator expressions,
>> and because several properties will depend on the content of the
>> LINK_LIBRARIES property, cycles in generator expressions become a problem
>> for the first time (up to now they haven't been). This is to be expected
>> whenever a declarative language is used. In the top commit in my clone I
>> describe the problem more fully, and a way which might work to break
>> cycles like that, but which require user-intervention. I don't think I
>> have the full solution yet, so more ideas are welcome.
> 
> It will be difficult to define the behavior for cycles.  I think rejecting
> them and requiring the user to handle it is reasonable for now.  We can
> always define non-error behavior for them later in certain cases.

Yes, that's my approach so far. I fear the limit of it will be hit soon, 
especially as more properties depend on the LINK_LIBRARIES, as I wrote in 
the commit message there. If we have this:

 set_property(TARGET LibFoo INTERFACE_LINK_LIBRARIES
     $<$<TARGET_PROPERTY:WIN32_EXECUTABLE>:libwinhelper> 
 ) # If the using property is WIN32, link to libwinhelper

 add_executable(exe LibFoo)
 # In my branch WIN32_EXECUTABLE would be defaulted to:
 #
 # $<EACH:$<TARGET_PROPERTY:LINK_LIBRARIES>,"
 #           "$<OR:$<EACH_ACCUMULATED:0>,"
 #                "$<BOOL:$<TARGET_PROPERTY:$<EACH_ARG>,"
 #                "INTERFACE_WIN32_EXECUTABLE>>>"
 #
 # That is, check if any of the link libraries say WIN32_EXECUTABLE should 
 # be ON. However, because we need to know our own WIN32_EXECUTABLE value in 
 # order to know our link libraries (do we link to libwinhelper or not?), we 
 # have a cycle-error appearing in innocent-looking user-code because of the 
 # cmake implementation of defaulting the value of WIN32_EXECUTABLE like 
 # that. The error would probably be confusing too.



One solution I'm thinking of now probably to disallow the use of 
TARGET_PROPERTY in any evaluation of LINK_LIBRARIES. This makes some sense 
because the link libraries are supposed to be the primary connections on 
which everything else depends. However, it would disallow my goal of having 
the INTERFACE_LINK_LIBRARIES of Qt5::Core contain 
"$<TARGET_PROPERTY:WIN32_EXECUTABLE:Qt5::WinMain>". The WinMain library is 
just an implementation detail of Qt, so that's a good goal too.

So, we'll have to choose some trade-off one way or the other.

> The fact that the Plugin test had to be fixed for export() to be done at
> generate time is a bit worrying to me.  It is possible that some projects
> export info from one directory and load it in another.  This could happen
> for example when a dependency is added as a subdirectory but still used
> with find_package().  I didn't think about this case when you asked about
> export() before.  

When I refactored the test I was considering the case at hand and couldn't 
see any use for what it was doing apart from cmake unit tests (it brings 
existing targets into scope under a different name).

Can you give more information about the case you mention? I don't see any 
mention in the documentation of using find_package() with a directory.

> We need to consider how to handle compatibility here,
> but only with projects not using the new features enabled by delaying
> export() until generate time.  Can you explain this in more detail?

This may only be doable by duplicating a lot more code and making 
cmExportBuildFileGenerator not inherit from cmExportFileGenerator. The 
latter uses GetLinkInterface, GetSoName and HasSoName. 

HasSoName depends on the link languages, and therefore the libraries and 
therefore has to move to cmGeneratorTarget. If export() is to remain a 
configure-time construct, then HasSoName would have to remain in some form 
in cmTarget. That could mean a lot of duplication.

Thanks,

Steve.