[cmake-developers] IMPORTED targets for some Find modules

[Philipp Moeller]

Stephen Kelly <steveire at gmail.com> writes:

> Philipp Möller wrote:
>
>> To simplify exporting targets I added IMPORTED targets to some of the
>> Find modules.
>
> Thanks for working on this. Just a few minor comments:
>
> In the FindX11 documentation commit, one of the changes is to replace use of 
> two spaces between sentences with one. That's counter to the prevailing 
> style in CMake.
>
> cmExportFileGenerator marks frameworks with a FRAMEWORK target property, and 
> Qt 5 emulates that. It could be done with these files too (I notice in 
> FindGLUT at least). I don't know if it has any effect on IMPORTED targets, 
> but it may in the future even if it does not currently.

That sounds like a good idea. Although I think that would make things
very confusing: the IMPORTED_LOCATION would be the full path of the
library, the INTERFACE_INCLUDE_DIRECTORIES would be the full path to the
includes, but all of this is ignored as soon as -framework is used and
the full path to the framework isn't specified (as far as my
understanding of OSX goes).

Maybe it would make sense to add a FRAMEWORK library type and a
find_framework command to encapsulate all this. It would make writing
those imports a lot smoother as well.

> Is there any reason to make the boost components not depend on each other? 
> Or is that just left for future development?
>
> The Boost module documents that component imported targets have lower-case 
> names, but that is not the case (haha). The names depend on the arguments to 
> find_package currently:
>
>  find_package(Boost REQUIRED Thread)
>
>  if (TARGET Boost::Thread)
>    message("YES")
>  else()
>    message("NO")
>  endif()

AFAIK, the documentation says that components should be specified by
their canonical name. Unfortunately, it doesn't say what that is
exactly and I blindly assumed it to always be lower-case. I'll fix
this.

> It looks like a good idea to add Boost::boost to the 
> INTERFACE_LINK_LIBRARIES of each component imported target, or to similarly 
> populate the INTERFACE_INCLUDE_DIRECTORIES of each component imported 
> target.

I contemplated that, but here I tried to anticipate Boost
modularization. If we ever get specific include directories for each
component we can break a lot of builds that wrongly relied on the fact
that we brought in all of the Boost includes. If the includes are
separated, this will be much easier to adapt to for users.

> I would say something similar about the GLUT imported targets, but it seems 
> that only GLUT::GLUT is documented, so presumably it is the only one 
> intended for users to use. Is it verified that the other libraries are 
> really interface dependencies and not runtime requirements? If they are 
> really interface usage requirements, where are the headers of the other 
> libraries located?

I don't understand what you mean by 'runtime requirements'. GLUT
depends on either some X11 libraries or Cocoa for window creation, but
doesn't expose the system APIs directly. You will still need to link
against them.

I just went through the freeglut implementation and the external
headers are only windows.h, gl.h, and glu.h. Those dependencies also
missing in the original FindGLUT and this would be a worthwhile fix.

> Multiple IMPORTED_LOCATION_<CONFIG> are populated on the boost targets, but 
> the IMPORTED_CONFIGURATIONS target property is not populated.

Thanks. Didn't know this was necessary. Will be fixed.

> Is there a need to populate the IMPORTED_IMPLIB_<CONFIG> target properties 
> on Windows for any of the targets?

I don't have a Windows machine available right now, but I can try to
figure this out later. Windows users tend to rely on Boost
auto-linking though, which unfortunately doesn't interact very well with
IMPORTED targets.

> cmExportFileGenerator populates the IMPORTED_LINK_INTERFACE_LANGUAGES target 
> property, and Qt 5 emulates that. The same could be set to CXX at least for 
> the Boost targets.

Will be fixed.

> One of the reasons Qt imported targets are called Qt4::Foo and Qt5::Foo is 
> to avoid accidental combination of, say, Qt::Core and Qt::Gui from different 
> major versions. They also encode a INTERFACE_QT_MAJOR_VERSION and add  
> QT_MAJOR_VERSION to the COMPATIBLE_INTERFACE_STRING target property. 
> Something similar could be added for these imported targets.
>
> In the case of Boost, because they don't provide binary compatibility or 
> promise source compatibility, it might make sense to encode the full 
> version, not only the major version, in a similar way to ensure that only 
> boost libraries from the same boost release are used together. In the 
> future, post modularization, boost may attempt to release some modules on a 
> separate release schedule and with disparate version numbers. They may still 
> release 'boost foobar 1.3' with 'Boost 1.58' though, so '1.58' would still 
> be the appropriate 'distribution version' to encode. 

[...snipped...]

>
> In the case of boost, it would also make sense to add 
> INTERFACE_MULTITHREADED to the targets and COMPATIBLE_INTERFACE_BOOL. I'm 
> assuming that a multithreaded Boost::system can't be used with a non-
> multithreaded Boost::filesystem for example. Currently I have the debian 
> package libboost-thread1.54-dev installed on my system but not the package 
> libboost-system1.54-dev, so FindBoost will find the thread component but not 
> the system component (in that prefix at least). Depending on the 
> build/package of boost it might be possible to conflict on things like that.
>
> Others to consider for this compatibility requirement are the 
> Boost_COMPILER, Boost_NAMESPACE, Boost_THREADAPI etc.

Those changes would greatly enhance usability and I'll look into
implementing them. For now I went with a minimal system that does not
grant more safety than you would already get with the old-school
variable style.

First we need to get dependencies between Boost libraries figured out
and then we should move on towards the full feature set.

I'll open a new branch on GitHub as soon as the current changes hit
master.

Thanks for your feedback,
Philipp