Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CMake configuration overhaul #133

Open
4 tasks
tmadlener opened this issue May 4, 2021 · 7 comments · May be fixed by #140
Open
4 tasks

CMake configuration overhaul #133

tmadlener opened this issue May 4, 2021 · 7 comments · May be fixed by #140

Comments

@tmadlener
Copy link
Contributor

tmadlener commented May 4, 2021

The cmake configuration of several ilcsoft packages is pretty old and could potentially profit from exploiting newer cmake functionalities and in general newer cmake paradigms, e.g. target based configurations. Since LCIO is at the very base of everything, it could serve as a good starting point to check what would need changing and could then serve as an example for other packages as well.

A few points that should be decided/considered beforehand:

  • Which minimum cmake version do we require? LCG releases 96 and 97 come with 3.14, which looks like it is probably the hardest constraint. The current stable version is 3.20, Ubuntu 20.04 has 3.16
  • "Deprecation strategy" for the old configuration. In principle the target based and XXX_LIBRARIES, XXX_INCLUDE_DIRS approach can co-exist (see, e.g. SIO, so that it should be possible to update this package by package without breaking everything while updating.

A more modern cmake configuration has the potential to make some of the cmake macros that are part of ilcsoft obsolete, as cmake can now handle many things for which these macros were originally designed by itself. To which degree we can remove these macros is hard to estimate and will be something we find out along the way.

Other TODOs (keeping them here because then they show up as X of Y in the overview):

  • Use FindPython to detect python
  • Make sure the BUILD_TESTING option is considered for all tests
@tmadlener
Copy link
Contributor Author

Arguments for going to cmake > 3.15: https://cmake.org/cmake/help/latest/policy/CMP0094.html#policy:CMP0094

@tmadlener
Copy link
Contributor Author

tmadlener commented May 4, 2021

#132 (comment) for two more points:

  • Use FindPython to detect python
  • Make sure the BUILD_TESTING option is considered for all tests

@tmadlener
Copy link
Contributor Author

Some more considerations for the choice of the minimum required cmake version:
#132 (comment)

@tmadlener tmadlener linked a pull request Jun 4, 2021 that will close this issue
7 tasks
@Romendakil
Copy link

This remark fits to this issue, but feel free to open a separate one: I noticed that the two files LCIOConfig.cmake and LCIOConfigVersion.cmake end up after 'make install' in <CMAKE_INSTALL_PREFIX> directly, while for most (maybe all?) cmake built projects I know they are installed/written to <CMAKE_INSTALL_PREFIX>/lib/cmake
I would find it more consistent for these files to end up of there.

@tmadlener
Copy link
Contributor Author

Yes, definitely part of this issue. I need to find some time to finalize #140 and then this should also be fixed.

@jan-busa
Copy link

When I build latest version v2-20-2, LCIOConfig.cmake is placed under lib/cmake/LCIO/LCIOConfig.cmake and SIOTargets.cmake goes into lib/cmake/SIO/SIOTargets.cmake (notice the different directories). This leads to the problem when LCIOConfig.cmake around line 75 looks for SIOTargets.cmake in the line include("${CMAKE_CURRENT_LIST_DIR}/SIOTargets.cmake"). Of course the easiest way for fix would be to hard-code SIO path as include("${CMAKE_CURRENT_LIST_DIR}/.../SIO//SIOTargets.cmake") but I'm sure, there is a more elegant way, I'm just not so familiar with CMake.

@tmadlener
Copy link
Contributor Author

I have moved your issue (@jan-busa) into a separate one, because I this one is much broader and more work in general than solving yours (I hope)

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 a pull request may close this issue.

3 participants