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

Discrepency between lib and lib64 install paths #428

Closed
wavefunction91 opened this issue Oct 20, 2023 · 6 comments · Fixed by #429
Closed

Discrepency between lib and lib64 install paths #428

wavefunction91 opened this issue Oct 20, 2023 · 6 comments · Fixed by #429

Comments

@wavefunction91
Copy link

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

evaleev commented Oct 20, 2023

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

evaleev commented Oct 22, 2023

... 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 ValeevGroup/kit-cmake@c299edf address this issue?

@wavefunction91
Copy link
Author

@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
Copy link
Member

evaleev commented Oct 26, 2023

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

@evaleev evaleev linked a pull request Oct 26, 2023 that will close this issue
@wavefunction91
Copy link
Author

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

evaleev commented Oct 27, 2023

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

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.

2 participants