Public header depends on config.h

[atgeirr]

CpGrid.hpp includes CpGridData.hpp, which uses the version macros. This makes CpGrid.hpp depend on config.h, which is prone to create trouble.

[jokva]

#if DUNE_VERSION_NEWER(DUNE_COMMON, 3, 0)
#include <dune/common/parallel/collectivecommunication.hh>
#else
#include <dune/common/collectivecommunication.hh>
#endif

The problem is that this check fails in the test snippet in cmake/Modules/Finddune-cornerpoint.cmake

[atgeirr]

This check will no longer fail, since this code is no longer included by the snippet, if you use #108.

I took a closer look however, and CpGrid.hpp uses version macros and HAVE_MPI in a number of places. I think this is very unfortunate, but fail to see how it can be fixed easily. So even with the fix, the Finddune-cornerpoint.cmake macro would fail, just in a different place.

A hack to fix this would be to #define some version numbers into the test program.

[jokva]

I just applied the patch and tested - cornerpoint compiles fine (as expected & as it did before), but the feature test still fails on the exact same spot. From the test program with actual compiler output:

In file included from /usr/local/include/dune/grid/cpgrid/Iterators.hpp:116:0,
                 from /usr/local/include/dune/grid/cpgrid/Entity.hpp:357,
                 from /usr/local/include/dune/grid/CpGrid.hpp:48,
                 from /home/jorgekva/test/main.cpp:2:
/usr/local/include/dune/grid/cpgrid/CpGridData.hpp:54:36: fatal error: dune/common/mpihelper.hh: No such file or directory

Iterators includes CpGridData.hpp and the version checks fails, resulting in the wrong file being included.

Trying to compile opm-benchmarks gives some possibly related error as well, where dune/grid/common/Volumes.hpp cannot find the right file. Gcc error message: fatal error: dune/common/misc.hh: No such file or directory

In the git version of dune it is now called math.hh

[atgeirr]

So there are even more places these macros are used, it seems to be impossible to fix all of these anytime soon. We'll have to rewrite the test then. Does it work if you #define the version numbers in the test?

[jokva]

Yep. opm-benchmark also built when I #define'd the same numbers before the inclusion of Volumes.

[atgeirr]

Reopening as the issue is not fully handled by #108 as I thought when I submitted the PR, there are still many config.h dependencies in CpGrid.hpp.

[andlaus]

I think the "headers are not allowed to use config.h variables" policy cannot be implemented (or is unpractical at best): E.g. opm-parser needs to change the type of the regular expression class in the ParserKeyword depending on whether std::regex is usable or not. Also, Dune has (very likely) a ton of such issues..

[atgeirr]

For our own code I think it should be doable, but hard. This can be one reason to use the pimpl pattern, which likely would work fine with ParserKeyword. I agree that we cannot enforce this for all existing code anytime soon, but I will be looking for such things, and try to avoid it in future code.

[andlaus]

the opaque pointer pattern may help, but so does the "everything is in the headers" approach of the templates. Since the opaque pointer approach still does not solve the problem completely (method signatures may be different depending on config macros), I think that this is not really a compelling argument for it.

Even if it was, I personally much prefer having issues like this to losing any agility with regard interface changes (ever wanted to add an argument to the constructor?). Maybe I'll change my mind once the project has settled and such changes happen only very rarely and we start guaranteing ABI and API compatibility...

[jokva]

Honestly, the big issue here (at least from a build perspective) is that include paths depend on Dune versioning. However, since this is a git build, a fairly reasonable assumption is building against the latest Dune git version (>=3.x). A simple solution is to, in trunk, use the 3.x paths, omitting the #if altogether. The directives are there to support building against several Dunes, which might not be a real-world issue.

[bska]

since this is a git build, a fairly reasonable assumption is building against the latest Dune git version (>=3.x).

It's not quite that clear cut. There is, for instance, a benchmark case that doesn't converge at all in Dune 2.3 but works well with a Dune 2.2.1 back-end (don't know about Dune's Git master). There is also a peculiar performance regression in the AMG/CG-based linear solver in Dune 2.3. I have runs for which the latter is slower by a factor of two compared to the edition in Dune 2.2.1. I have no idea why and it may well be pilot error. We need to track down the reason before we can obsolete Dune 2.2.1 however.

As for targeting the latest Dune Git version: That won't happen as long as I am the maintainer of dune-cornerpoint. We simply don't have the project resources (manpower) to chase a moving target of that size. Patches to "fix" things when targeting Dune master will be accepted only when they do not conflict with working production setups. That said, we also don't have a good (or even particularly workable) policy for when and how to synchronise the OPM sources to new editions of third-party prerequisites.

[andlaus]

@jorgekva
Out of curiosity: are you using the dune or the OPM build system, or a home-brewn one?

[jokva]

@andlaus
I'm building both Dune and OPM with cmake, so the upstream build system.

[andlaus]

well using CMake does not automatically mean that you'r using the Opm build system. if you have include (OpmLibMain) in your CMakeLists.txt, you're using the OPM build system, if no then not...

[jokva]

I'm using the OPM build system.

[andlaus]

hm. then this should not happen. could you post the config.h it generates as a gist?

[jokva]

https://gist.github.com/jorgekva/db72378f8fb96175ce0d

Notice that Dune vars are set correctly. But the problem isn't building the project itself, it's the cmake find modules that uses test programs that don't use the config.h when compiling the test program, leading to broken include paths.

This is building on Ubuntu 12.04

[andlaus]

hm, right. I didn't think about this. the simplest fix would be to change the cmake modules so that they don't need config.h (which is not available at that stage yet...) I'll have to meditate about it a bit, though...

[jokva]

I really haven't checked if this is possible yet, but if some dune-cornerpoint class/header doesn't involve pulling dune headers the simplest solution is to just change the test example to use that.

[bska]

Just for the record: The Autotools pass the results of previous tests along as -DFOO options when performing a new test. We do the same in our find_opm_package() CMake macro through the CMAKE_REQUIRED_DEFINITIONS variable. In other words, it should be possible to write probes that adapt to Dune versions. Doing so probably requires some thought, though, especially as most projects' CMakeLists.txt load the prerequisites before processing the config_hook.

It's a bit of a chicken and egg problem. Maybe the simplest approach is to always do the equivalent of

opm_need_version_of ("dune-${module}")

as part of Finddune-${module}.cmake?

[andlaus]

I think that would be hard. but instead of checking for dune/grid/CpGrid.hpp it could check for a header which is less complex, like dune/grid/cpgrid/Entity.hpp. that would not be a "once and forever" solution but until the entity header needs config.h variables, it will work...

[andlaus]

Just for the record: The Autotools pass the results of previous tests along as -DFOO options when performing a new test.

cmake does this as well as far as I remember. the problem is that config.h is not regenerated and included before each test...

In other words, it should be possible to write probes that adapt to Dune versions.

yep. if the config.h file gets abondoned entirely and all variables defined therein are converted into "-D" compiler flags. That would quite a bit of work, though...

[atgeirr]

It seems that we will not be able to resolve this across-the-board without abandoning the traditional "config.h" approach. So I am closing it as a "won't fix" case.