Skip to content

Conversation

@dennisklein
Copy link
Contributor

@dennisklein dennisklein commented Aug 31, 2017

Dear ROOT team,

while integrating some CMake functionality provided by ROOT through the ROOT_USE_FILE include into our project, some small issues came up. If we are mistaken and there are better solutions than the one I provided, your advice is very welcome.

Our FindROOT.cmake implementation looks like this:

find_package(ROOT QUIET CONFIG
  HINTS
  ${ROOT_ROOT}  # aliBuild
  ${ROOTSYS}    # upstream
  $ENV{ROOTSYS} # upstream
  ${SIMPATH}    # FairSoft
)
include(FindPackageHandleStandardArgs)
find_package_handle_standard_args(ROOT CONFIG_MODE)
include(${ROOT_USE_FILE})

which is called in a CMakeLists.txt like this

find_package(ROOT 6.10.04 REQUIRED)

Please see the commit messages for more details about the issues we had.

Best regards,
Dennis

When calling ROOT_GENERATE_DICTIONARY in an external project after
a cmake include(${ROOT_USE_FILE}) and having installed ROOT in a
non-system location (e.g. in a home directory), the rootcling command is
missing its library path to find dynamic libraries. With the change in
this commit the correct environment for the rootcling command is set.
@dennisklein dennisklein requested a review from peremato as a code owner August 31, 2017 17:35
@phsft-bot
Copy link

Can one of the admins verify this patch?

@Teemperor
Copy link
Contributor

@phsft-bot build

@phsft-bot
Copy link

Starting build on centos7/gcc49, mac1012/native, slc6/gcc49, slc6/gcc62, ubuntu14/native with flags -Dvc=OFF -Dimt=ON -Dccache=ON
How to customize builds

#message(AUTHOR_WARNING "Couldn't find target: " ${library} "\n Forgot to call ROOT_GENERATE_DICTIONARY?")
endif()
add_dependencies(${library} move_headers)
if(NOT ARG_NOMOVEHEADERSDEP)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could also check for CMAKE_PROJECT_NAME equals ROOT which means that users don't have to figure out they have to call it with this parameter. Or at least we should rename the parameter something more generic like "ROOT_EXTERNAL" because we might add more special cases for external projects here. @peremato what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is right. Better to check for CMAKE_PROJECT_NAME to be or not to be ROOT.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated as requested.

Copy link
Contributor

@peremato peremato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not understand why you have to have your FindROOT.cmake. We do provide ROOTConfig.cmake and this should be sufficient. If it is not, then this is a bug. Be as native CMake as possible and of not try to complicate things by building additional interfaces.
The functions like ROOT_LINKER_LIBRARY where intended for internal consumption. In the example provided in the howto we only use ROOT_GENERATE_DICTIONARY.

#message(AUTHOR_WARNING "Couldn't find target: " ${library} "\n Forgot to call ROOT_GENERATE_DICTIONARY?")
endif()
add_dependencies(${library} move_headers)
if(NOT ARG_NOMOVEHEADERSDEP)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is right. Better to check for CMAKE_PROJECT_NAME to be or not to be ROOT.

@miniassetbot
Copy link

I do not understand why you have to have your FindROOT.cmake. We do provide ROOTConfig.cmake and this should be sufficient. If it is not, then this is a bug. Be as native CMake as possible and of not try to complicate things by building additional interfaces.

What we do in the FindROOT.cmake is as native and cmake idiomatic as it gets. find_package has two modes. The one used here is the config mode which is intended to be used to find and include your package config file (ROOTConfig.cmake).

The functions like ROOT_LINKER_LIBRARY where intended for internal consumption.

Does that mean you will not support usage in external projects? Or do you consider it now to support? If not, I suggest putting it in a different module, so its clearer which macros are meant to be for public use.

@miniassetbot
Copy link

Ups, logged in with wrong user, the last post was by @dennisklein

@dennisklein dennisklein force-pushed the external-cmake-integration branch 2 times, most recently from 14df1f4 to 6f918fb Compare September 1, 2017 13:03
If one calls the ROOT_LINKER_LIBRARY macro from an external project
after a cmake include(${ROOT_USE_FILE}), the build fails, because of the
missing move_headers target, which is defined in a ROOT project specific
CMakeLists.txt file. This commit will add the dependency to the
move_headers target only, if the macro is called from withing the ROOT
project.
@dennisklein dennisklein force-pushed the external-cmake-integration branch from 6f918fb to 5f43efa Compare September 1, 2017 13:05
@amadio
Copy link
Member

amadio commented Sep 4, 2017

@phsft-bot build

@phsft-bot
Copy link

Starting build on centos7/gcc49, mac1012/native, slc6/gcc49, slc6/gcc62, ubuntu14/native with flags -Dvc=OFF -Dimt=ON -Dccache=ON
How to customize builds

@Teemperor Teemperor merged commit 182a914 into root-project:master Sep 4, 2017
@Teemperor
Copy link
Contributor

Thanks for the patch!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants