Fix cmake for installation to a location #233

JacobLotz · 2023-08-02T12:02:25Z

EDITED 31-08-2023
Last Januari I have been working on two features: 1) to install libROM to a specified install location 2) to use MFEM from a specified location. This branch got out of sync with the master branch so I have re-emplemented this in the latest master of libROM. The changes mostly consist of changes in the cmake.

I am not an cmake expert and I learned this on the fly. The code works on my own local machine, but unfortunately more elaborate testing is difficult. I am creating this PR as it would be a pity if this would be lost and someone has to redo it again. However, this addition probably needs a thorough review of someone that actually knows what he is doing in cmake. If we cannot make that work it would be unfortunate. Then I will just use this in my forked version of libROM.

For feature 1) I have modified the cmake script with standard features such that it installs the binary and header files to a specified location. This location is specified in -DCMAKE_INSTALL_PREFIX. I have added standard options such that another cmake script is able to find it. The dependencies of libROM have to be added separately. I think there should be a way to account for the dependencies in libROM.config.in, but I could not figure out how to do that. I have added an example script on how to find libROM in cmake below.

For feature 2) I have added two options (which probably can be streamlined with other options or merged into one). These are -DUSE_EXTERNAL_MFEM=On and -DMFEM_DIR=$INSTALLDIR. This allows for usage of in external installation of MFEM, such that that you can compile libROM with the same version of MFEM you are already using in your code. In my workflow I will first compile and install MFEM, then compile and install libROM with this version of MFEM, then compile my code with MFEM and libROM.

An example shell script installation in such a workflow is as follows:

    mkdir build
    cd build
    cmake .. \
    -DCMAKE_INSTALL_PREFIX=$INSTALLDIR \
    -DCMAKE_CXX_COMPILER=/usr/bin/mpicxx \
    -DBUILD_STATIC=On \
    -DENABLE_EXAMPLES=Off \
    -DUSE_MFEM=Off \
    -DUSE_EXTERNAL_MFEM=On \
    -DMFEM_DIR=$INSTALLDIR
    make install -j $cores

An example script for cmake to find libROM would then be:

# Import libROM
find_package(librom REQUIRED NAMES libROM HINTS "${CMAKE_INSTALL_PREFIX}")
enable_language(${libROM_ENABLED_LANGUAGES})
link_libraries(${libROM_LIBRARIES})
# Librom dependencies
find_package(HDF5 1.8.0 REQUIRED)
find_package(BLAS 3.4.0 REQUIRED)
find_package(LAPACK 3.4.0 REQUIRED)
find_package(MPI 1.2 REQUIRED)
find_package(ZLIB 1.2.3 REQUIRED)
find_package(Doxygen 1.8.5)
find_package(GTest 1.6.0)

message(STATUS "Found libROM: in ${librom_DIR} (version ${librom_VERSION})")
message(STATUS "libROM_LIBRARIES = ${libROM_LIBRARIES}")
message(STATUS "libROM_ENABLED_LANGUAGES = ${libROM_ENABLED_LANGUAGES}")

chldkdtn · 2023-08-22T18:58:46Z

@dreamer2368 & @dylan-copeland can you review this PR?

dylan-copeland · 2023-08-23T00:46:22Z

cmake/libROMConfig.cmake.in

@@ -0,0 +1,19 @@
+## Some copyright problably goes here


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

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

dreamer2368 · 2023-08-23T01:05:10Z

The code-style failure comes from IncrementalSVDBrand, which is already fixed in the current master branch. We might want to rebase this w.r.t. the current master branch.

dylan-copeland · 2023-08-23T02:52:40Z

In the past, whenever I needed to build libROM with a specific branch or commit of MFEM, I would just cd libROM/dependencies/mfem_parallel and checkout that branch, rebuild MFEM there, and then rebuild libROM with ./scripts/compile.sh -m. However, that is not working for me now. Something must have changed in the build system recently. Assuming we fix that, wouldn't that accomplish the same thing as this PR. If that is the case, is there still a good reason to merge this PR, or is the procedure I described sufficient, @JacobLotz?

dreamer2368 · 2023-08-23T15:33:48Z

Assuming we fix that, wouldn't that accomplish the same thing as this PR. If that is the case, is there still a good reason to merge this PR, or is the procedure I described sufficient, @JacobLotz?

I'm wondering if I can jump in. I think it might be useful to allow users to have their custom mfem path. They may already using mfem somewhere else and wouldn't want to change the setting, or have a duplicate mfem?

Although, I also think some part is somewhat redundant, like USE_EXTERNAL_MFEM variable (which is commented in the code review).

JacobLotz · 2023-08-30T07:09:44Z

@dreamer2368: I have rebased with respect to the current master branch and the tests seem to pass now.

@dylan-copeland: I think you accomplish the same. However, I think that the method in this PR is more elegant and is less prone to errors as you are very sure that you use the same MFEM version.

@dreamer2368: Yes please jump in! I think you have good ideas and we should use them. I have given you access to my fork. I will have a look if I can move this branch to libROM repo (not the fork) itself, that might work a bit easier. EDIT: I think that is not possible.

BTW I think that we should not forget that the main feature of this branch is to be able to install libROM to a specific location.

dreamer2368 · 2023-08-23T15:10:15Z

CMakeLists.txt

@@ -111,6 +114,11 @@ find_package(Doxygen 1.8.5)

 find_package(GTest 1.6.0)

+if (USE_EXTERNAL_MFEM)


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.

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.

This is removed.

dreamer2368 · 2023-08-23T15:12:23Z

CMakeLists.txt

+    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")


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.

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?

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.

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:

MFEM compiled/installed in the libROM folder (default)

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:

We find the dependencies installed specifically for the MFEM in the libROM folder by means of a search for these libraries independently.

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.

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.

lib/CMakeLists.txt

dreamer2368 · 2023-08-31T05:21:24Z

I forgot to finish the review to leave the comments.. To me, I think PR seems okay once the comments can be resolved.

…into new-cmake-install

dylan-copeland · 2023-10-21T02:54:05Z

@JacobLotz is there a way for me to checkout your PR from libROM? I cloned your fork and manually copied the changed files to my clone of master, which would not be convenient for a larger PR. Is there a better way?

dylan-copeland · 2023-10-21T03:02:46Z

@JacobLotz my attempt at a standard build with ./scripts/compile.sh -m resulted in the following error. I suppose you are building in a different way, but this script should still work.

CMake Error in lib/CMakeLists.txt:
Target "ROM" INTERFACE_INCLUDE_DIRECTORIES property contains path:
"libROM/dependencies/mfem"
which is prefixed in the source directory.

chldkdtn · 2024-06-24T05:41:45Z

What is the status of this PR?

…into new-cmake-install

Fix cmake for installation to a location

d31ab33

JacobLotz added the enhancement label Aug 2, 2023

JacobLotz self-assigned this Aug 2, 2023

chldkdtn added the RFR Ready for review label Aug 22, 2023

chldkdtn requested review from dylan-copeland and dreamer2368 August 22, 2023 18:58

dylan-copeland reviewed Aug 23, 2023

View reviewed changes

Merge remote-tracking branch 'origin/master' into new-cmake-install

583a2ce

Added license to libROMConfig.cmake.in

e09d264

dreamer2368 reviewed Aug 31, 2023

View reviewed changes

JacobLotz added 14 commits August 31, 2023 14:12

Streamlined input options

0cd3331

Merge branch 'new-cmake-install' of github.com:JacobLotz/libROM-fork …

0d73dc6

…into new-cmake-install

Proposition for construction for finding MFEM.

7eb30d2

fix check for internal or enxternal MFEM

311cd88

Merge remote-tracking branch 'origin/master' into new-cmake-install

008ff13

fix bug in cmakelists in string compare

816fcee

fix testing

6266504

Fix testing 2

71df74c

Fix testing 2

e67d36a

fix testing 3

3b88a78

Fix testing 4

1180287

Fix testing 5

6db4f60

Fix testing 6

35be353

Fix testing 7

3c5840d

JacobLotz added 3 commits October 12, 2023 11:54

Fix testing 8

fb9bddd

clean up

f9167d4

more clean up and some doc

09cc80a

chldkdtn requested review from ckendrick, andersonw1, dylan-copeland, dreamer2368, chldkdtn and siuwuncheung October 17, 2023 06:42

JacobLotz added 2 commits September 15, 2024 13:17

Apply fix for hyper reduction from LLNL#258 here as well

8701ef2

Merge branch 'new-cmake-install' of github.com:JacobLotz/libROM-fork …

1d87f21

…into new-cmake-install

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix cmake for installation to a location #233

Fix cmake for installation to a location #233

JacobLotz commented Aug 2, 2023 •

edited

Loading

chldkdtn commented Aug 22, 2023

dylan-copeland Aug 23, 2023

chldkdtn Aug 23, 2023 •

edited by dreamer2368

Loading

JacobLotz Aug 31, 2023

dreamer2368 commented Aug 23, 2023

dylan-copeland commented Aug 23, 2023

dreamer2368 commented Aug 23, 2023 •

edited

Loading

JacobLotz commented Aug 30, 2023 •

edited

Loading

dreamer2368 Aug 23, 2023

JacobLotz Aug 31, 2023

JacobLotz Oct 12, 2023

dreamer2368 Aug 23, 2023

JacobLotz Aug 31, 2023

dreamer2368 Sep 5, 2023

JacobLotz Oct 12, 2023

JacobLotz Oct 12, 2023

dreamer2368 commented Aug 31, 2023

dylan-copeland commented Oct 21, 2023

dylan-copeland commented Oct 21, 2023

chldkdtn commented Jun 24, 2024

		@@ -111,6 +114,11 @@ find_package(Doxygen 1.8.5)

		find_package(GTest 1.6.0)

		if (USE_EXTERNAL_MFEM)

Fix cmake for installation to a location #233

Are you sure you want to change the base?

Fix cmake for installation to a location #233

Conversation

JacobLotz commented Aug 2, 2023 • edited Loading

chldkdtn commented Aug 22, 2023

Choose a reason for hiding this comment

chldkdtn Aug 23, 2023 • edited by dreamer2368 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dreamer2368 commented Aug 23, 2023

dylan-copeland commented Aug 23, 2023

dreamer2368 commented Aug 23, 2023 • edited Loading

JacobLotz commented Aug 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dreamer2368 commented Aug 31, 2023

dylan-copeland commented Oct 21, 2023

dylan-copeland commented Oct 21, 2023

chldkdtn commented Jun 24, 2024

JacobLotz commented Aug 2, 2023 •

edited

Loading

chldkdtn Aug 23, 2023 •

edited by dreamer2368

Loading

dreamer2368 commented Aug 23, 2023 •

edited

Loading

JacobLotz commented Aug 30, 2023 •

edited

Loading