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.

Sign up for GitHub

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

Fix cmake for installation to a location #233

Open
wants to merge 22 commits into
base: master
Choose a base branch
from
+129 −13
Open

Fix cmake for installation to a location #233

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
d31ab33
Fix cmake for installation to a location
JacobLotz Aug 2, 2023
583a2ce
Merge remote-tracking branch 'origin/master' into new-cmake-install
JacobLotz Aug 30, 2023
e09d264
Added license to libROMConfig.cmake.in
JacobLotz Aug 30, 2023
0cd3331
Streamlined input options
JacobLotz Aug 31, 2023
0d73dc6
Merge branch 'new-cmake-install' of github.com:JacobLotz/libROM-fork …
JacobLotz Aug 31, 2023
7eb30d2
Proposition for construction for finding MFEM.
JacobLotz Aug 31, 2023
311cd88
fix check for internal or enxternal MFEM
JacobLotz Aug 31, 2023
008ff13
Merge remote-tracking branch 'origin/master' into new-cmake-install
JacobLotz Oct 11, 2023
816fcee
fix bug in cmakelists in string compare
JacobLotz Oct 11, 2023
6266504
fix testing
JacobLotz Oct 12, 2023
71df74c
Fix testing 2
JacobLotz Oct 12, 2023
e67d36a
Fix testing 2
JacobLotz Oct 12, 2023
3b88a78
fix testing 3
JacobLotz Oct 12, 2023
1180287
Fix testing 4
JacobLotz Oct 12, 2023
6db4f60
Fix testing 5
JacobLotz Oct 12, 2023
35be353
Fix testing 6
JacobLotz Oct 12, 2023
3c5840d
Fix testing 7
JacobLotz Oct 12, 2023
fb9bddd
Fix testing 8
JacobLotz Oct 12, 2023
f9167d4
clean up
JacobLotz Oct 12, 2023
09cc80a
more clean up and some doc
JacobLotz Oct 12, 2023
8701ef2
Apply fix for hyper reduction from https://github.com/LLNL/libROM/pul…
JacobLotz Sep 15, 2024
1d87f21
Merge branch 'new-cmake-install' of github.com:JacobLotz/libROM-fork …
JacobLotz Sep 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 46 additions & 0 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,14 @@ project(libROM
HOMEPAGE_URL "https://github.com/LLNL/libROM"
LANGUAGES C CXX Fortran)

include(GNUInstallDirs)

set(libROM_CMAKE_PATH ${CMAKE_SOURCE_DIR}/cmake)
set(libROM_CMAKE_MODULE_PATH ${libROM_CMAKE_PATH}/modules)
list(APPEND CMAKE_MODULE_PATH ${libROM_CMAKE_MODULE_PATH})

option(USE_MFEM "Build libROM with MFEM" OFF)
option(USE_EXTERNAL_MFEM "Build libROM with external MFEM specified in -DMFEM_DIR" OFF)
option(MFEM_USE_GSLIB "Build libROM with MFEM using GSLIB" OFF)
option(BUILD_STATIC "Build libROM as a static library" OFF)
option(ENABLE_EXAMPLES "Build examples and regression tests" ON)
Expand Down Expand Up @@ -111,6 +114,11 @@ find_package(Doxygen 1.8.5)

find_package(GTest 1.6.0)

if (USE_EXTERNAL_MFEM)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not exactly sure we need this part, or even USE_EXTERNAL_MFEM.

If we want to use external mfem directory, we can specify with the cmake flag -DMFEM_DIR=/path/to/mfem.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I only understand this partly. Do you mean that we can streamline the input -DMFEM_DIR and -DUSE_EXTERNAL_MFEM? This is what I have streamlined in a new commit. If -DMFEM_DIR is not empty it will set the boolean USE_EXTERNAL_MFEM to true.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is removed.

find_library(MFEM mfem "${MFEM_DIR}/lib")
find_path(MFEM_INCLUDES mfem.hpp "${MFEM_DIR}/include")
endif()

if (USE_MFEM)
find_library(MFEM mfem "${CMAKE_SOURCE_DIR}/dependencies/mfem" "${MFEM_DIR}/lib")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might want to switch the order of paths to control the default action.

For example,

     find_library(MFEM mfem "${MFEM_DIR}/lib" "${CMAKE_SOURCE_DIR}/dependencies/mfem")

This way, any user-specified MFEM_DIR can be searched first. And if not specified, cmake will further search the dependencies directory.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I get it, but I am not sure if that will work. The external MFEM installation will also bring Hypre, Metis etc. So we do not have to import them again here. I propose that we use a construction such as.

if (USE_EXTERNAL_MFEM)
   ...
elseif(USE_MFEM)
   ...
endif()

I will pushed a commit with such a proposition. It is closely related to the comment above I think.

I wonder if we should put in some checks to verify that MFEM indeed brings Hypre, Metis etc for the external mfem option. What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The external MFEM installation will also bring Hypre, Metis etc. So we do not have to import them again here. I propose that we use a construction such as.

if (USE_EXTERNAL_MFEM)
   ...
elseif(USE_MFEM)
   ...
endif()

I'm a little unsure about this. If this was the case, we wouldn't have had to include mfem dependencies from the beginning. In fact, in order to use the external mfem correctly, I think we do have to import all the dependencies explicitly as well.

In any cases, to me the suggestion I made above seems sufficient. At the point MFEM_DIR is specified and cmake does find the mfem from that path, it is clear that the installation is using external mfem. We could probably do the same path order change for other dependencies as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @dreamer2368 , a lot of stuff came in between, I am sorry for that. I just to a look at this again.

I think the current solution is the way to go for the following reasoning.

We have two ways of importing MFEM:

  1. MFEM compiled/installed in the libROM folder (default)
  2. An external installation with the location installed in ${MFEM_DIR}/lib.

The first and the second option might have different dependencies. They do not have to be different, but they might be different. Also, most probable they are installed in different locations, but this does not have to be. We find them in the following way for both options:

  1. We find the dependencies installed specifically for the MFEM in the libROM folder by means of a search for these libraries independently.
  2. We import the dependencies whilst importing the external MFEM. The cmake of MFEM is configured as such that uppon importing MFEM, it will also import its dependencies.

So in the second option, if we import the external MFEM and we import the dependencies, we might load the dependencies twice. The current set up avoids this.

I think the logic could be different in CMakeLists.txt but it would not give a different result.

@dreamer2368 Do you agree?

Remark 1: I am very unsure about this, but I would not be surprised if the separate finding of Hypre and (Par)Metis is not necessary for option 1. However, it would probably not hurt.

Remark 2: The way that importing MFEM will also bring its dependencies has not been implemented by me in this PR. For the current install, upon importing libROM, the dependencies have to be imported separately.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @dreamer2368 , I had a look again and I think you were right. I have implemented your suggestion and it works out! I left the comment above for documentation of the conversation, but you can ignore it.

find_library(HYPRE HYPRE "${CMAKE_SOURCE_DIR}/dependencies/hypre/src/hypre/lib" "${HYPRE_DIR}/lib")
Expand Down Expand Up @@ -301,3 +309,41 @@ if(DOXYGEN_FOUND)
add_dependencies(doxygen_tagfile documentation)

endif(DOXYGEN_FOUND)

#-------------------------------------------------------------------------------
# Installation
# Configure CMake find_package() config files
#-------------------------------------------------------------------------------

# Default option
if (NOT CMAKE_BUILD_TYPE)
set(CMAKE_BUILD_TYPE "Release")
endif()

include(CMakePackageConfigHelpers)

# Extract the enabled languages required to use ROM
get_property(ROM_ENABLED_LANGUAGES GLOBAL PROPERTY ENABLED_LANGUAGES)

configure_package_config_file(
"${PROJECT_SOURCE_DIR}/cmake/${PROJECT_NAME}Config.cmake.in"
"${PROJECT_BINARY_DIR}/${PROJECT_NAME}Config.cmake"
INSTALL_DESTINATION
${CMAKE_INSTALL_LIBDIR}/cmake/${PROJECT_NAME}
PATH_VARS
ROM_ENABLED_LANGUAGES
)

write_basic_package_version_file(
"${PROJECT_NAME}ConfigVersion.cmake"
VERSION ${PROJECT_VERSION}
COMPATIBILITY SameMajorVersion
)

install(FILES
${PROJECT_BINARY_DIR}/${PROJECT_NAME}Config.cmake
${PROJECT_BINARY_DIR}/${PROJECT_NAME}ConfigVersion.cmake
DESTINATION
${CMAKE_INSTALL_LIBDIR}/cmake/${PROJECT_NAME}
)

19 changes: 19 additions & 0 deletions cmake/libROMConfig.cmake.in
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
## Some copyright problably goes here
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose you can just copy the header from the main libROM/CMakeLists.txt file. @chldkdtn can you please confirm?

Copy link
Collaborator

@chldkdtn chldkdtn Aug 23, 2023 •
edited by dreamer2368
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct. @JacobLotz, you can copy it from the header of the main libROM/CMakeLists.txt, i.e.,

###############################################################################
#
#  Copyright (c) 2013-2023, Lawrence Livermore National Security, LLC
#  and other libROM project developers. See the top-level COPYRIGHT
#  file for details.
#
#  SPDX-License-Identifier: (Apache-2.0 OR MIT)
#
###############################################################################

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check!

## Some licence should problably go here

@PACKAGE_INIT@

include("${CMAKE_CURRENT_LIST_DIR}/@PROJECT_NAME@_Targets.cmake")

check_required_components("@PROJECT_NAME@")
check_required_components("ROM")

# Variables required for linking etc.
set(libROM_LIBRARIES ROM)
set(libROM_ENABLED_LANGUAGES "@ROM_ENABLED_LANGUAGES@")

check_required_components("@PROJECT_NAME@")
check_required_components("ROM")


# Somehow the dependencies should go here -> Did not yet get this working
45 changes: 41 additions & 4 deletions lib/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ list(APPEND source_files
linalg/Options.h
librom.h)

if (USE_MFEM)
if (USE_MFEM OR USE_EXTERNAL_MFEM)
list(APPEND source_files
mfem/PointwiseSnapshot.hpp
mfem/PointwiseSnapshot.cpp
Expand Down Expand Up @@ -129,12 +129,49 @@ target_link_libraries(ROM
${LAPACK_LIBRARIES} ${BLAS_LIBRARIES} ${MFEM} ${HYPRE} ${PARMETIS} ${METIS}
PRIVATE ${ZLIB_LIBRARIES} ZLIB::ZLIB)

target_include_directories(ROM PUBLIC
${CMAKE_CURRENT_SOURCE_DIR}
target_include_directories(ROM
PUBLIC
${MFEM_INCLUDES}
${HYPRE_INCLUDES}
${PARMETIS_INCLUDES}
${HDF5_C_INCLUDE_DIRS}
${MPI_C_INCLUDE_DIRS}
${MFEM_C_INCLUDE_DIRS}
PRIVATE ${CMAKE_CURRENT_SOURCE_DIR})
$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}>
$<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}> # Same as below with installation of headers
PRIVATE
${CMAKE_CURRENT_SOURCE_DIR}
)

#-------------------------------------------------------------------------------
# Installation
dreamer2368 marked this conversation as resolved.
Show resolved Hide resolved
#-------------------------------------------------------------------------------
# Set version information
set_target_properties(ROM
PROPERTIES VERSION ${PROJECT_VERSION} SOVERSION ${PROJECT_VERSION_MAJOR})

# Default option
if (NOT CMAKE_BUILD_TYPE)
set(CMAKE_BUILD_TYPE "Release")
endif()

install(TARGETS ROM
EXPORT libROM_Targets
LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR}
)

# Target information about that artefact
install(EXPORT libROM_Targets
DESTINATION ${CMAKE_INSTALL_LIBDIR}/cmake/libROM
)

# Install headers
install(DIRECTORY ${CMAKE_CURRENT_LIST_DIR}/
DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}
FILES_MATCHING
PATTERN *.h
PATTERN *.inl
PATTERN *.hpp
PATTERN *.ih
)