[cmake-developers] target_link_libraries, IMPORTED targets and SYSTEM includes

[Stephen Kelly]

Matthew Woehlke wrote:

> On 2013-07-25 13:25, Stephen Kelly wrote:
>> Brad King wrote:
>>
>>> On 07/25/2013 12:22 PM, Stephen Kelly wrote:
>>>> library A should have a unit test which attempts to compile all
>>>> of its headers with all warnings enabled. In Qt every module has a
>>>> 'headersclean' unit test which does exactly that.
>>>
>>> While this is a good idea we're not going to assume every project has
>>> such discipline.  Also some consumers may include headers with a
>>> different preprocessing context that exposes the warning.
>>
>> Right. Even if all headers are used by the upstream itself, this still
>> applies. However, I still think IMPORTED=SYSTEM by *default* is a good
>> idea, and let the edge case switch it back. I'm not seeing support for it
>> though, so I guess I'll drop it.
> 
> Somewhat echoing Brad's reply here, but it's not that I'm opposed to the
> idea, merely "concerned" that it is possible to get non-system includes
> when that is desired.
> 
> Perhaps that is a misreading on my part, but I was getting the
> impression this change would make it so that imported targets can only
> ever be SYSTEM.

I didn't really make that clear as the discussion developed, but yes, I 
think it makes sense to allow treating the include directories as non-system 
somehow, even if not by default.

> 
>>> Then why not make INTERFACE_SYSTEM_INCLUDE_DIRECTORIES the default in
>>> the Qt imported targets and have a switch to turn them off?  The code
>>>
>>>   set(QT_INCLUDE_DIRS_NO_SYSTEM 1)
>>>   find_package(Qt5Core)
>>>
>>> is not so bad in the edge case that needs it.
>>
>> I don't like it though :). I'd prefer not to have any variables that
>> change the behavior of a find_package call, so that only one find_package
>> is ever needed, not multiple in different directories for scope.
> 
> FWIW, I agree; variables controlling find_package (except search paths)
> are almost always an inferior solution :-).
> 
> That said... what about having a SYSTEM (and/or NO_SYSTEM) flag for
> find_package? This could, for config-based packages with imported
> targets, control the default behavior for imported include directories.

I don't think that solves the problem. You might have one target which you 
want to treat the directories as SYSTEM, and one which you don't. I think 
the API for this should have target scope: A target property like 

 set_property(TARGET foo PROPERTY IMPORTED_INTERFACE_DIRS_SYSTEM 1)

and 

 set(CMAKE_IMPORTED_INTERFACE_DIRS_SYSTEM 1)

to set the default, as usual.

or 

 set_property(TARGET foo PROPERTY IMPORTED_INTERFACE_DIRS "system")
 set_property(TARGET foo PROPERTY IMPORTED_INTERFACE_DIRS "non_system")

 set(CMAKE_IMPORTED_INTERFACE_DIRS "system")

>>> Either way, the tll() SYSTEM option could still be useful.
>>
>> I'll add that tomorrow.
> 
> Again FWIW, a nice advantage of this is the ability to specify SYSTEM
> includes per target. (I'm not sure why you'd want to build one target
> with imported includes as SYSTEM and another not, but you could...)

Yes. Even with the target property as above you could still do

 target_link_libraries(foo SYSTEM Bar::core)
 target_link_libraries(foo Bar::gui)

We could also add the opposite if you want. 

 target_link_libraries(foo NON_SYSTEM Bar::core)

However, as the above already is enough for all use-cases and what we're 
discussing is an edge-case already, we don't need too much API for it. We 
can always add it later if needed.

Thanks,

Steve.