ValeevGroup / tiledarray

A massively-parallel, block-sparse tensor framework written in C++
GNU General Public License v3.0
254 stars 52 forks source link

Discrepency between `lib` and `lib64` install paths #428

Closed wavefunction91 closed 11 months ago

wavefunction91 commented 11 months ago

As was introduced in #404 and amended in #426, current CMake best practice is to use GNUInstallDirs over hardcoded paths in CMAKE_INSTALL_XYZ. The issue is that not everyone does this uniformly, so care has to be taken when deciding where install paths actually get delegated to.

On some architectures (have yet to figure out how this is decided by cmake), GNUInstallDirs sets CMAKE_INSTALL_LIBDIR to lib64 rather than lib. This is a problem as many of the hardcoded install paths for dependencies assume lib. With my local (non-GPU) install, that list seems to contain:

  1. MADNESS
  2. Umpire
  3. BTAS

However, this issue is actualyl a bit deeper as not only to these dependencies not respect GNUInstallDirs (which could be argued to be unnecessicary) - they don't respect CMAKE_INSTALL_LIBDIR, which is a much graver issue.

From a top-level install, this can be resolved by hardwiring the install paths upon fetch. For example, I've been able to get (most) of MADNESS to install to a custom LIBDIR by adding the lines

diff --git a/cmake/modules/FindOrFetchMADWorld.cmake b/cmake/modules/FindOrFetchMADWorld.cmake
index 7be76bac..0d1de5dd 100644
--- a/cmake/modules/FindOrFetchMADWorld.cmake
+++ b/cmake/modules/FindOrFetchMADWorld.cmake
@@ -20,6 +20,13 @@ if (NOT TARGET MADworld)
   set(ENABLE_MEM_PROFILE ON CACHE INTERNAL "Whether to enable instrumented memory profiling in MADNESS")
   set(ENABLE_TASK_DEBUG_TRACE ${TILEDARRAY_ENABLE_TASK_DEBUG_TRACE} CACHE INTERNAL "Whether to enable task profiling in MADNESS")

+  set(MADNESS_INSTALL_BINDIR "${TILEDARRAY_INSTALL_BINDIR}"
+        CACHE PATH "MADNESS binary install directory" FORCE)
+  set(MADNESS_INSTALL_INCLUDEDIR "${TILEDARRAY_INSTALL_INCLUDEDIR}"
+        CACHE PATH "MADNESS INCLUDE install directory" FORCE)
+  set(MADNESS_INSTALL_LIBDIR "${TILEDARRAY_INSTALL_LIBDIR}"
+        CACHE PATH "MADNESS LIB install directory" FORCE)
+
   # Set error handling method (for TA_ASSERT_POLICY allowed values see top-level CMakeLists.txt)
   if(TA_ASSERT_POLICY STREQUAL TA_ASSERT_IGNORE)
     set(_MAD_ASSERT_TYPE disable)

The pkgconfig still gets installed to lib, but I suspect that's fixable. I'd assume that. Umpire should be straight forward, but BTAS is a bit of a pain as it installs a blaspp_header target to lib that clashes with blaspp's lib64 install path and breaks discovery.

The alternative is to require that CMAKE_INSTALL_LIBDIR is lib to maintain consistency - some people may not like that, but its a short term band-aid that seems to work

evaleev commented 11 months ago

thanks ... MADNESS and BTAS are easy since "we" control them, I'll make tracking issues + PRs. With Umpire we should make an issue (and potentially PR) also ...

P.S.

evaleev commented 11 months ago

... but BTAS is a bit of a pain as it installs a blaspp_header target to lib that clashes with blaspp's lib64 install path and breaks discovery.

does https://github.com/ValeevGroup/kit-cmake/commit/c299edfc41e58dce2edde904ab96fd187744e4e1 address this issue?

wavefunction91 commented 11 months ago

@evaleev, yes, that should fix it. Let me know if there's anything you need on my end to test the full-stack solution / if there's anywhere you'd like an extra pair of hands.

evaleev commented 11 months ago

https://github.com/ValeevGroup/BTAS/pull/166 and https://github.com/m-a-d-n-e-s-s/madness/pull/507 should fix the install issues with BTAS and MADNESS, respectively ... let me know what we can do about Umpire

wavefunction91 commented 11 months ago

Looking at it, the hardcoded use of lib in Umpire is pretty pervasive (also through their dependencies) - they have a lot of inertia, so it might be a bit of a challenge to get them to change things. I'll open some issues to test the water.

However, I think these PRs will do the trick - the main issue was BTAS installing / checking different locations of the blaspp_headers target when LIBDIR was something unexpected (lib64). I'll check that this fixes the install problem I was experiencing. Does #429 pin to these upstream changes?

evaleev commented 11 months ago

However, I think these PRs will do the trick - the main issue was BTAS installing / checking different locations of the blaspp_headers target when LIBDIR was something unexpected (lib64). I'll check that this fixes the install problem I was experiencing. Does #429 pin to these upstream changes?

yes, you can try #429 to see if this resolves the install issues (at least inherited via BTAS/MADNESS).