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

include CMake macro definition #327

Closed
wants to merge 2 commits into from

Conversation

berndflemisch
Copy link
Member

No description provided.

@atgeirr atgeirr requested a review from akva2 June 13, 2018 09:44
@blattms
Copy link
Member

blattms commented Jun 13, 2018

I guess we never realized this as as all opm modules already include this before handling the *-prereqs.cmake files. This should be save to merge.

@berndflemisch
Copy link
Member Author

I had to add it for building the Dune module Dumux that suggests opm-grid as dependency.

@berndflemisch
Copy link
Member Author

I just recognized that this isn't the appropriate fix, since OpmPackage.cmake is possibly not found. It worked for me only since I had opm-common and opm-grid locally installed. Sorry!

@berndflemisch berndflemisch changed the title include CMake macro definition WIP include CMake macro definition Jun 15, 2018
@atgeirr
Copy link
Member

atgeirr commented Jun 15, 2018

I guess @akva2 will have to look at this when he is back.

@berndflemisch
Copy link
Member Author

It works if one adds opm-common/cmake/Modules to the CMAKE_MODULE_PATH. Maybe this can be achieved automatically?

@akva2
Copy link
Member

akva2 commented Jun 18, 2018

opm-grid has a hard dependency on opm-common due to the shared build system components. the opm-common config file adds the path to the CMAKE_MODULE_PATH.

the fix is correct in itself, but incomplete. you need opm-common, so the dune.module file should be updated to reflect this fact (ie it is not a suggest).

@berndflemisch berndflemisch changed the title WIP include CMake macro definition include CMake macro definition Jun 18, 2018
@berndflemisch
Copy link
Member Author

This works, thank you. I added a corresponding commit.

@blattms
Copy link
Member

blattms commented Jun 18, 2018

Sorry guys, but on my system this still does not work:

CMake Error at /home/mblatt/test-cpgrid-dumux/opm-grid/opm-grid-prereqs.cmake:37
 (include):
  include could not find load file:

    OpmPackage

At this point opm-common has not been searched for. Seems like the same chicken and egg problem as with dune-common. I guess dumux should search for opm-common in its CMakeLists.cmake.

With this patch to dumux it works:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 2d6d97067..477612c86 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -12,8 +12,18 @@ if(NOT (dune-common_DIR
       ${PROJECT_BINARY_DIR})
 endif()
 
+if(NOT (opm-common_DIR
+        OR opm-common_ROOT
+        OR "${CMAKE_PREFIX_PATH}" MATCHES ".*opm-common.*"))
+    string(REPLACE  ${CMAKE_PROJECT_NAME}
+      opm-common opm-common_DIR
+      ${PROJECT_BINARY_DIR})
+endif()
 #find dune-common and set the module path
 find_package(dune-common)
+#find opm-common to support OPM
+find_package(opm-common)
+
 list(APPEND CMAKE_MODULE_PATH ${dune-common_MODULE_PATH}
   "${PROJECT_SOURCE_DIR}/cmake/modules")
 #include the dune macros

@berndflemisch
Copy link
Member Author

Do you mean that one has to update Dumux' CMakeLists.txt only or that one also has to consider the changes of this PR?

@blattms
Copy link
Member

blattms commented Jun 19, 2018

Yes, you need to modify the CMakeLists.txt of every module using OPM wih DUNE's build system (in your case dumux). Otherwise you will run into problems in some circumstances as the config files of opm-common will not be found.
In your case I would guess that an installed version of opm-common was found. That might not be what you want.

@berndflemisch
Copy link
Member Author

The patch for Dumux works. I committed it under your authorship, thank you!

@berndflemisch
Copy link
Member Author

The patch to Dumux renders the changes proposed in this PR unnecessary. While they still seem sensible to me, please feel free to close without merging.

@blattms
Copy link
Member

blattms commented Jun 21, 2018

Thanks for the heads up. We should put that into the wiki.

@blattms blattms closed this Jun 21, 2018
@bska bska deleted the fix/include-cmake-macro-definition branch June 21, 2018 10:11
@atgeirr
Copy link
Member

atgeirr commented Jun 25, 2018

Thanks for the heads up. We should put that into the wiki.

@blattms can you do that?

@blattms
Copy link
Member

blattms commented Jul 6, 2018

Wiki entry is upcoming once this really works as advertised. My tests revealed that the macro definition was
still not there.

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.

4 participants