iLCSoft / LCIO

Linear Collider I/O
BSD 3-Clause "New" or "Revised" License
17 stars 34 forks source link

Replace hardcoded lib with CMAKE_INSTALL_LIBDIR #132

Closed andriish closed 3 years ago

andriish commented 3 years ago

In this PR the hardcoded installation paths of CMake modules and libraries are replaced with CMAKE_INSTALL_LIBDIR, which is expanded either to lib or to lib64. E.g. this results in a correct location path /usr/lib64/cmake for modules like MacroCheckPackageLibs.cmake, which otherwise are always installed to /usr/lib/cmake (which does not even exist).

BEGINRELEASENOTES

ENDRELEASENOTES

gaede commented 3 years ago

Hi @andriish. I am not sure if we really want to install LCIO libraries in a lib64 dir on any system. At least in the past, we never did this. In any case I don't think you ever want to install LCIO or any other iLCSoft package in /usr/lib or /usr/lib64 but always have it in a well defined global installation path. but I am also not a cmake expert really, so I let @tmadlener comment, as he has some ideas about modernizing the overall cmake code used for iLCSoft whether he thinks this is the right thing to do here (and then also consistently in the other iLCSoft packages) ?

andriish commented 3 years ago

Hi @gaede ,

The case with /usr is just an example. The canonical destinations when installing something with cmake are

binaries --> CMAKE_INSTALL_BINDIR libraries --> CMAKE_INSTALL_LIBDIR documentation --> CMAKE_INSTALL_DOCDIR data --> CMAKE_INSTALL_DATADIR and so on. The destinations depend on the system, but are known to CMake and to all other programs. It is very similar to the way this is done in autotools. In any case, with this approach it is guaranteed that cmake modules/macros from LCIO will be installed in the location visible to cmake.

Best regards,

Andrii

tmadlener commented 3 years ago

Hi @andriish thanks for this. As @gaede has mentioned there are a places in ilcsoft that implicitly assume that shared libraries are installed in <prefix>/lib, which also makes the CI fail currently. So there are a few things that would still need to be addressed in this pull request, which should only be minor tweaks to have the right environment setup for some of the unit tests.

However, in general I do think that this is the right thing to do, and this definitely goes into the right direction also for an upcoming overhaul of the cmake configuration for LCIO (and other ilcsoft packages). Just for completeness and to avoid potential misunderstandings using GNUInstallDirs and subsequently the CMAKE_INSTALL_XXXDIR will put things into ${CMAKE_INSTALL_PREFIX}/${CMAKE_INSTAL_XXXDIR}, so we will still have the correct prefix installation but instead of a lib folder we get a lib64 on some systems.

If we make these changes we also have to keep track of them in iLCInstall, since that checks the presence of certain binaries to check if an installation has been successful. In fact LCIO would not be the first package to use such a configuration, SIO already does: e.g. here and also has the corresponding config in iLCInstall.

In summary, I think these changes should go in, but the environment for the tests should be fixed first.

tmadlener commented 3 years ago

https://github.com/iLCSoft/LCIO/blob/8f9e86b93b7d5d83221fabb872ed7e82f1638476/CMakeLists.txt#L150 should be the culprit for failing tests in CI, I think.

tmadlener commented 3 years ago

Apart from #134, the mac workflows are currently broken for a different reason. See #135

andriish commented 3 years ago

Hi @tmadlener ,

the mac workflows are currently broken

Thanks! Good to know. Otherwise I would try to fix them as well :).

Best regards,

Andrii

P.S. Also, just noted two smaller issues with the tests:

tmadlener commented 3 years ago
  • python tests run just "python" w/o finding the python with find_package()

I think that is partially related to the fact that the minimum cmake is too old to have "native" support for this. I am actually not sure we have a FindPython.cmake module somewhere in our toolchain that would detect it.

I will try to fix the CI issues and have a look into why the one test is failing on some systems/environments.

andriish commented 3 years ago

Hi @tmadlener ,

1) It seems that for this PR only the python test is problematic.

2) > I am actually not sure we have a FindPython.cmake module somewhere in our toolchain that would detect it.

FindPython is available only from cmake 3.12+. Unfortunately some important systems (CentOS8) still use cmake 3.11. To solve this problem in HepMC3 we have copied the FindPython modules locally, in cmake directory. You can see that e.g. here https://gitlab.cern.ch/hepmc/HepMC3/-/archive/3.2.0/HepMC3-3.2.0.tar.gz There are very few of them. This solution works fine for cmake ~3.8+ or so.

However, if only the python executable is needed, one can use find_program( ). The easiest solution, as for me.

3) The lowest bound of cmake is dictated by ROOT, I think. So it cannot be lower than 3.8 or so. So if one uses just find_program for python executable, 3.8 or 3.9 should be fine.

Best regards,

Andrii

tmadlener commented 3 years ago

Hi @andriish,

The python test should be fixed in #136 together with an updated CI configuration for the macOS workflows. As already outlined in #133 the whole cmake configuration is in need of an update and in the course of that we will also bump the required minimum version of cmake to something more recent. I will link your comment to the discussion there as well.

tmadlener commented 3 years ago

@andriish could you rebase this onto the new master please. Otherwise github will not pick up the new CI workflow configurations.

andriish commented 3 years ago

Uh, actually I completely forgot that before cmake 3.12 there was a package FindPythonInterp which would satisfy the needs completely. No need for cmake version bump. At all.