ValeevGroup / tiledarray

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

Fix `INSTALL` paths #426

Closed wavefunction91 closed 9 months ago

wavefunction91 commented 9 months ago

As I mentioned in the PR discsussion, #404 breaks install as CMAKE_INSTALL_XYZ are not set until one of the proper CMake XYZInstallDirs is included.

To see the offending behaviour, simply add the following likes CMakeLists.txt

message(STATUS "TILEDARRAY_INSTALL_BINDIR        ${TILEDARRAY_INSTALL_BINDIR} ")
message(STATUS "TILEDARRAY_INSTALL_INCLUDEDIR    ${TILEDARRAY_INSTALL_INCLUDEDIR}")
message(STATUS "TILEDARRAY_INSTALL_LIBDIR        ${TILEDARRAY_INSTALL_LIBDIR} ")
message(STATUS "TILEDARRAY_INSTALL_SHAREDIR      ${TILEDARRAY_INSTALL_SHAREDIR}")
message(STATUS "TILEDARRAY_INSTALL_DATADIR       ${TILEDARRAY_INSTALL_DATADIR}")
message(STATUS "TILEDARRAY_INSTALL_DOCDIR        ${TILEDARRAY_INSTALL_DOCDIR}")
message(STATUS "TILEDARRAY_INSTALL_CMAKEDIR      ${TILEDARRAY_INSTALL_CMAKEDIR}")

With a basic cmake invocation

cmake -S /path/to/ta -B /path/to/build -DCMAKE_INSTALL_PREFIX=/path/to/install

Behaviour before (incorrect)

-- TILEDARRAY_INSTALL_BINDIR         
-- TILEDARRAY_INSTALL_INCLUDEDIR    
-- TILEDARRAY_INSTALL_LIBDIR         
-- TILEDARRAY_INSTALL_SHAREDIR      /tiledarray/1.0.0
-- TILEDARRAY_INSTALL_DATADIR       /tiledarray/1.0.0/data
-- TILEDARRAY_INSTALL_DOCDIR        /tiledarray/1.0.0/doc
-- TILEDARRAY_INSTALL_CMAKEDIR      /cmake/tiledarray

Behaviour after (correct)

-- TILEDARRAY_INSTALL_BINDIR        bin 
-- TILEDARRAY_INSTALL_INCLUDEDIR    include
-- TILEDARRAY_INSTALL_LIBDIR        lib 
-- TILEDARRAY_INSTALL_SHAREDIR      share/tiledarray/1.0.0
-- TILEDARRAY_INSTALL_DATADIR       share/tiledarray/1.0.0/data
-- TILEDARRAY_INSTALL_DOCDIR        share/tiledarray/1.0.0/doc
-- TILEDARRAY_INSTALL_CMAKEDIR      lib/cmake/tiledarray

Assuming that GNUInstallDirs is the expected default (which was the case before #404, only hardcoded), this PR resolves the expected behaviour. N.B. Installs are currently broken without this change (unless a user manually specifies all the CMAKE_INSTALL_XYZ variables by hand.

FWIW - it might be worth adding a unit test for this: I check both install + discover and subproject builds in GauXC, ExchCXX, MACIS, etc (since there's a butterfly effect in changing CMakeLists.txt, these tests ensure that we don't regress expected behaviour) . If you think that's worth it for TA, I'll add an issue and add it when I get some free time.

evaleev commented 9 months ago

yes, of course https://github.com/ValeevGroup/tiledarray/pull/404#discussion_r1208030129 is all wrong ... I missed the uses of TILEDARRAY_INSTALL_* dirs in TA's own CMakeLists.txt

removed unused instances of TILEDARRAY_INSTALL_*, good to go

evaleev commented 9 months ago

FWIW - it might be worth adding a unit test for this: I check both install + discover and subproject builds in GauXC, ExchCXX, MACIS, etc (since there's a butterfly effect in changing CMakeLists.txt, these tests ensure that we don't regress expected behaviour) . If you think that's worth it for TA, I'll add an issue and add it when I get some free time.

of course this would be good. Subproject builds are already tested at the top of VG (i.e. (https://github.com/ValeevGroup/mpqc4), [MPQC]) but need minimal subproject build anyway to avoid interaction with existing cmake state