Skip to content

Conversation

@wavefunction91
Copy link

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

evaleev commented Oct 18, 2023

yes, of course #404 (comment) 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 evaleev merged commit d0c0ded into master Oct 18, 2023
@evaleev
Copy link
Member

evaleev commented Oct 18, 2023

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

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.

3 participants