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

Replace hardcoded lib with CMAKE_INSTALL_LIBDIR #132

Merged
merged 4 commits into from
May 5, 2021
Merged

Replace hardcoded lib with CMAKE_INSTALL_LIBDIR #132

merged 4 commits into from
May 5, 2021

Conversation

andriish
Copy link

@andriish andriish commented May 4, 2021

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

  • Replace hardcoded lib with CMAKE_INSTALL_LIBDIR when appropriate

ENDRELEASENOTES

@gaede
Copy link
Contributor

gaede commented May 4, 2021

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
Copy link
Author

andriish commented May 4, 2021

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
Copy link
Contributor

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
Copy link
Contributor

export LD_LIBRARY_PATH=${PROJECT_BINARY_DIR}/lib:$LD_LIBRARY_PATH
should be the culprit for failing tests in CI, I think.

@tmadlener
Copy link
Contributor

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

@andriish
Copy link
Author

andriish commented May 4, 2021

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:

  • Some tests are enabled even if -DBUILD_TESTING:BOOL=OFF is given
  • python tests run just "python" w/o finding the python with find_package()

@tmadlener
Copy link
Contributor

  • 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.

@tmadlener tmadlener mentioned this pull request May 4, 2021
4 tasks
@andriish
Copy link
Author

andriish commented May 5, 2021

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.

  1. 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
Copy link
Contributor

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
Copy link
Contributor

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

@andriish
Copy link
Author

andriish commented May 5, 2021

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.

@gaede gaede merged commit 4ec7576 into iLCSoft:master May 5, 2021
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.

None yet

3 participants