[ROOT8062] ADD INTERFACEINCLUDEDIRECTORIES AND NAMESPACING TO EXPORTEDIMPORTED CMAKE TARGETS

[ROOT-8062] Add INTERFACE_INCLUDE_DIRECTORIES and namespacing to exported/imported CMake targets Created: 16/Mar/16  Updated: 15/May/18  Resolved: 01/Mar/18

Status:

Closed

Project:

ROOT

Component/s:

Build System

Affects Version/s:

6.06/02

Fix Version/s:

6.13/01

Type:

New Feature

Priority:

Medium

Reporter:

Benjamin Morgan

Assignee:

Guilherme Amadio

Resolution:

Fixed

Votes:

2

Labels:

None

Remaining Estimate:

Not Specified

Time Spent:

Not Specified

Original Estimate:

Not Specified

Environment:

All

PlannedEnd:

01/Mar/18

PlannedStart:

16/Mar/16

Development:

Actual End:

01/Mar/18 3:14 PM

I'd like to request that ROOT's CMake package configuration files add usage requirements
for the created imported targets. This would require a bump in the minimum CMake version for building ROOT itself to 2.8.12, but shouldn't affect clients of an installed ROOT using 2.8.8 or similar.

The main usage requirement I'd like to request is use of the INTERFACE_INCLUDE_DIRECTORIES property on targets, that can be set using the target_include_directories command:

https://cmake.org/cmake/help/v2.8.12/cmake.html#command:target_include_directories

This can help both internally and externally as setting the include directories on a target means a client of the target does not need to explicitly call include_directories. For example, instead of doing

find_package(ROOT REQUIRED)

...

include_directories(${ROOT_INCLUDE_DIRS})

...

target_link_libraries(MyTarget ${ROOT_LIBRARIES})

a client can simply do

find_package(ROOT REQUIRED)

...

target_link_libraries(MyTarget ${ROOT_LIBRARIES})

and CMake will handle adding the requisite include paths to the compilation of {{MyTarget}}s sources.

I'd also like to request a "namespace" for the ROOT imported targets both to avoid name clashes and to clarify the origin of a consumed target in client scripts, e.g.

export(... NAMESPACE ROOT:: ...)

install(EXPORT <exportsetname> NAMESPACE ROOT:: DESTINATION ...)

A client can then do, for example

find_package(ROOT REQUIRED Physics)

...

# Client package's Physics lib

add_library(Physics ...)

...

target_link_libraries(MyTarget ROOT::Physics Physics)

and not have a name clash between the internal and ROOT targets. Clients using the ROOT_LIBRARIES variable are not affected as the namespacing is hidden to them.

The additional benefit here is that usage requirements are transitively applied and have public/private scope, so if the Client package exposes ROOT in its interface, it only needs to re-find ROOT in its own "ClientPackageConfig.cmake" file rather than manually configure/set up include directories (which might hard-code values). That helps both ease of use and creation of fully relocatable packages.

Additional usage requirements such as compile options, definitions and features would be helpful, but I think the above are the most useful.

Comment by Pere Mato Vila [ 24/Nov/16 ]

Hi Ben, sorry for being very late on this one. I have implemented the namespace for libraries with the following commit
https://github.com/root-mirror/root/commit/840f3d2e43e65c82ad03d40189f233aa639347f5

What is not clear to me is how to implement the first part of your request. In particular, when using the command target_include_directories() I do not know what to put in there, the current location of the header files in the binary directory or the installation location. We have all the header files grouped under the same heading. I do not think is make sense that each library adds again the include directory pointing to the same place. Any light on this would be very much appreciated.

Comment by Benjamin Morgan [ 29/Nov/16 ]

Hi Pere,

For target_include_directories, there are a couple of ways to do handle it. To retain the current build behaviour (include_directories) and just get INTERFACE_INCLUDE_DIRECTORIES in the installed targets, the INCLUDES DESTINATION argument to install(TARGETS ...) could be used with $CMAKE_INSTALL_INCLUDEDIR/root as input.

If generator expressions are allowed in ROOT's targets then target_include_directories can handle both build and install contexts with something like

target_include_directories(SomeTarget

  PUBLIC

    $<BUILD_INTERFACE:${PROJECT_BINARY_DIR}/include> # Assumes all headers copied under build directory

                                                     # Add additional $<BUILD_INTERFACE:path> lines for any further internal paths needed

    $<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}/root>)

However, I've had issues with dictionary generation when target-level include directories are used as ROOT_GENERATE_DICTIONARY and similar only use the directory level INCLUDE_DIRECTORIES property and don't consider target properties. It should be possible to use the above in addition to current include_directories calls without conflict as CMake should strip out duplicated entries (see below).

Regarding repeated -I arguments, I think that for targets within a package/namespace, duplicate entries in expansion of INTERFACE_INCLUDE_DIRECTORIES will be removed, so in the install context

target_link_libraries(MyTarget ROOT::Core ROOT::Physics)

should only result in -I/path/to/headers being added once in compilation commands for MyTarget sources. This should also be the same in the build context. I've tested this on CMake 3.3/3.6/3.7 with a test project of two libraries (when they don't link and when they do), and the header path only appears once even when linking both libraries - that's a very simple case though, so quite possible ROOT's larger dependency tree may cause other effects.

Comment by Pere Mato Vila [ 29/Nov/16 ]

Hi Ben. Thanks very much for the clear explanation. I'll give it a ry.

Comment by Dennis Klein [ 11/Sep/17 ]

We also would very much appreciate the added INTERFACE_INCLUDE_DIRECTORIES property to the exported targets. Currently, we use a workaround after the find_package(ROOT) call:

foreach(t ROOT::vectorDict ROOT::listDict ROOT::forward_listDict ROOT::dequeDict ROOT::mapDict ROOT::map2Dict ROOT::unordered_mapDict ROOT::multimapDict ROOT::multimap2Dict ROOT::unordered_multimapDict ROOT::setDict ROOT::unordered_setDict ROOT::multisetDict ROOT::unordered_multisetDict ROOT::complexDict ROOT::valarrayDict ROOT::Cling ROOT::MultiProc ROOT::Rint ROOT::Thread ROOT::Imt ROOT::New ROOT::Core ROOT::rmkdepend ROOT::MathCore ROOT::MathMore ROOT::Matrix ROOT::Minuit ROOT::Minuit2 ROOT::Fumili ROOT::Physics ROOT::MLP ROOT::Quadp ROOT::Foam ROOT::Smatrix ROOT::SPlot ROOT::GenVector ROOT::Genetic ROOT::Hist ROOT::HistPainter ROOT::Spectrum ROOT::SpectrumPainter ROOT::Unfold ROOT::Hbook ROOT::Tree ROOT::TreePlayer ROOT::TreeViewer ROOT::RIO ROOT::SQLIO ROOT::XMLIO ROOT::XMLParser ROOT::Net ROOT::RootAuth ROOT::Krb5Auth ROOT::SrvAuth ROOT::rootd ROOT::Netx ROOT::NetxNG ROOT::RHTTP ROOT::Gpad ROOT::Graf ROOT::Postscript ROOT::mathtext ROOT::GX11 ROOT::GX11TTF ROOT::ASImage ROOT::ASImageGui ROOT::Graf3d ROOT::X3d ROOT::Eve ROOT::RGL ROOT::GLEW ROOT::FTGL ROOT::Gviz3d ROOT::Gui ROOT::Ged ROOT::FitPanel ROOT::GuiBld ROOT::GuiHtml ROOT::Recorder ROOT::SessionViewer ROOT::Proof ROOT::ProofPlayer ROOT::ProofDraw ROOT::ProofBench ROOT::proofd ROOT::XrdProofd ROOT::proofexecv ROOT::Proofx ROOT::pq2 ROOT::Html ROOT::EG ROOT::VMC ROOT::EGPythia6 ROOT::EGPythia8 ROOT::Geom ROOT::GeomBuilder ROOT::GeomPainter ROOT::Gdml ROOT::root ROOT::minicern ROOT::MemStat ROOT::rootn.exe ROOT::roots.exe ROOT::ssh2rpd ROOT::xpdtest ROOT::root.exe ROOT::proofserv.exe ROOT::hadd ROOT::rootnb.exe ROOT::g2root ROOT::h2root ROOT::rootcling ROOT::PyROOT ROOT::JupyROOT ROOT::PgSQL ROOT::RSQLite ROOT::TMVA ROOT::TMVAGui ROOT::RooFitCore ROOT::RooFit ROOT::RooStats ROOT::HistFactory ROOT::hist2workspace)

  if(TARGET ${t})

    get_target_property(incdirs ${t} INTERFACE_INCLUDE_DIRECTORIES)

    if(NOT incdirs)

      set_target_properties(${t} PROPERTIES INTERFACE_INCLUDE_DIRECTORIES ${ROOT_INCLUDE_DIRS})

    endif()

  endif()

endforeach()

This workaround is ofc ugly, because of the hardcoded target list, which will go out of sync over time. We would also love an exported variable that contains just a list of all exported targets.

Comment by Guilherme Amadio [ 01/Mar/18 ]

I believe that this has been solved by the commit below:

commit 28f8d27572ed565428e8b133de99fe06b05ea5f7

Author: Guilherme Amadio <amadio@cern.ch>

Date: Mon Feb 26 17:28:35 2018 +0100

Add include directory to ROOT exported library targets

Can someone please confirm and close this issue?