config.h values included in official DUNE headers

added bug label

TL;DR: HAVE_UMFPACK is deprecated in favor of HAVE_SUITESPARSE_UMFPACK. This flag should be set through the CMake function add_dune_suitesparse_flags(), manually through a compile option definition, or a custom config file (like yours). There is a missing CHANGELOG entry for the deprecation of HAVE_UMFPACK though.

The main reason is that previous to this release, we handed-in modules that were not completely configured. This is explained in detail in !1262 (merged). In short, the configuration options of a module (i.e. config.h) was never know until the configuration file was generated in the downstream module. This happened upon calling dune_finalize(). !1262 (merged) fixed that and now we hand in a completely configured module when installed: that comes from the dune-common-config.hh that you see in the log. Note that you can switch to the old mode by just adding the old config.h previous to any dune header inclusion. This is explained in the Build System documentation. (Although I believe you guys don't use dune_finalize() and finalize configuration of modules manually, so this will probably won't work).

A problem with the new approach is that imported packages cannot be made available again through HAVE_<package> with a different value after a module is installed (i.e. first configure HAVE_UMFPACK=0 at configuration time, install this, and later change it to HAVE_UMFPACK=1: this would modify the configuration of an already configured module). Thus, these flags need to be defined downstream rather than in dune-common. Therefore, flags for optional packages were moved from the config file of the upstream package to compile definitions of the downstream project through the CMake functions add_dune_<package>_flags(). For suite sparse, this was done in !1294 (merged).

Now, looking at the changes !1294 (merged) more in detail, we removed HAVE_UMFPACK in favor of HAVE_SUITESPARSE_UMFPACK since we used to define both saying essentially the same thing. However, we forgot to add a CHANGELOG entry for this deprecation. @simon.praetorius This needs to be fixed before tagging the release

All these changes were designed to be backwards compatible the common scenario: people using the dune_finalize() function to generate their config.h. Other settings probably need a slight change on how this is handled. I understand it's a bit annoying, but we deemed it was a necessary step towards a more powerful build system.

I hope this clarifies a bit the rationale and what may be needed to be done on your side.

I think we should not include the dune-*-config.hh in any headers. This is currently done at various places:

$ find dune-*/dune/ -name \*.hh | xargs grep -r \\-config.hh
dune-common/dune/common/dynmatrixev.hh:#include <dune-common-config.hh>  // HAVE_LAPACK
dune-common/dune/common/fmatrixev.hh:#include <dune-common-config.hh>  // HAVE_LAPACK
dune-common/dune/common/std/functional.hh:#include <dune-common-config.hh> // DUNE_HAVE_CXX_STD_IDENTITY
dune-common/dune/common/std/type_traits.hh:#include <dune-common-config.hh> // DUNE_HAVE_CXX_EXPERIMENTAL_IS_DETECTED
dune-common/dune/common/stdstreams.hh:#include <dune-common-config.hh> // DUNE_MINIMAL_DEBUG_LEVEL
dune-common/dune/common/version.hh:#include <dune-common-config.hh> // DUNE_[MODULE]_VERSION_[TYPE]
dune-geometry/dune/geometry/quadraturerules/jacobiNquadrature.hh:#include <dune-common-config.hh> // HAVE_LAPACK
dune-grid/dune/grid/io/file/dgfparser/dgfug.hh:#include <dune-grid-config.hh> // HAVE_DUNE_UGGRID
dune-grid/dune/grid/uggrid.hh:#include <dune-grid-config.hh> // HAVE_DUNE_UGGRID
dune-grid/dune/grid/uggrid/ugmessagebuffer.hh:#include <dune-uggrid-config.hh> // DUNE_UGGRID_HAVE_DDDCONTEXT
dune-grid/dune/grid/uggrid/ugwrapper.hh:#include <dune-uggrid-config.hh> // DUNE_UGGRID_HAVE_DDDCONTEXT, ModelP
dune-grid/dune/grid/utility/gridtype.hh:#include <dune-grid-config.hh> // HAVE_GRIDTYPE
dune-istl/dune/istl/operators.hh:#include <dune-istl-config.hh> // DUNE_ISTL_SUPPORT_OLD_CATEGORY_INTERFACE
dune-istl/dune/istl/preconditioner.hh:#include <dune-istl-config.hh> // DUNE_ISTL_SUPPORT_OLD_CATEGORY_INTERFACE
dune-istl/dune/istl/solver.hh:#include <dune-istl-config.hh> // DUNE_ISTL_SUPPORT_OLD_CATEGORY_INTERFACE
dune-istl/dune/istl/superlufunctions.hh:#include <dune-istl-config.hh> // SUPERLU_INT_TYPE

There should be another way to force dune-modules to include those headers automatically without this using the DUNE build system.

For our case I guess we will just define DUNE_COMMON_CONFIG_HH and the likes and are hopefully done.

I think we should not include the dune-*-config.hh in any headers.

Could you elaborate on the reasoning for this opinion? The configuration file is what says how a project was configured and should not be changed by any other downstream module. These files are now directly generated in the source, why would you not include them where they are needed?

There should be another way to force dune-modules to include those headers automatically without this using the DUNE build system.

This is exactly the point of !1262 (merged): be able do it in the source code. This frees the user from having to use the DUNE build system or any other form to hack around with those configuration options. We discussed alternatives to this in #234 (closed), but no good standard alternative came up, so we had stick to compile definitions and inclusion of header files.

This is done by reason. It is a paradicmic change, though. There is not necessary a need to include config.h file in your source .cc files anymore, at least not in the dune-core modules. The main reason is, that we change the way optional packages are enabled. What is in the config.h file in the core modules are only hard configurations that cannot be changed in downstream modules. Thus they can safely be included in these module headers. We thereby follow the principle include-what-you-use. If a header file needs to know a configuration it needs to include the config file where this configuration is specified.

Everything that a downstream module wants to enable target-wise must have a corresponding add_dune_xxx_flags or the dune_enable_all_packages call. Note that flags that are used somewhere in a compiled library must be fixed.

The configuration file is what says how a project was configured and should not be changed by any other downstream module. These files are now directly generated in the source, why would you not include them where they are needed?

I tend to disagree... we need to distinguish between compiled libs and header-only modules. For libs I agree, the state must be clearly "documented", but for headers we have (in principal) some freedom to choose what we like in the downstream module, as the upstream module did not yet compile anything that depends on this choice.

Yes, but this is the whole point. It is now explained x-times. Configurations, that are to be changed cannot be part of a compiled library. Sure. Thus, it must be set on the downstream target where it is used. This is done via the add_dune_xxx_flags cmake functions, by adding directly compiler flags. This way, we do not need to put anything about these into the config.h file. This is what we worked on over the last year. Remove any remainder of flags that are meant for optional later setup. The HAVE_UMFPACK is actually not a flag. It is just a (deprecated) alias to the actual flag that must be set as compile flag.

Anything else is fixed and is not allowed to be changed. This would have unpredictable consequences. Thus, it can either be put into the config.h file on configure time, and this file is then not allowed to be changed (neither directly, or by any compile flag that comes later). Hard dependency that always are part of a library and not optional do not even need a config.h entry. They must always be set. But these are only very few dependencies.

If you want to have a project that depends on Dune, but does not use the dune build system, you are responsible for adding optional flags yourself to the compiler. No need to write a cofig.h file for that. Just add the flags. They are clearly listed in the corresponding add_dune_xxx_flags function in the AddDuneXXXFlags.cmake module files.

@christi I think we might be viewing the same problem from different angles. Let me try to clarify.

Your concern about (1) and (2) being achievable simultaneously is valid. However, the proposals in !1262 (merged) + !1294 (merged) don’t prevent this but modifies how we achieve them. To address both:

Fixed configuration options (e.g., compiled libraries): My proposal in !1262 (merged) suggests to generate and install a dune-*-config.hh from the public part of config.h.cmake.
Optional behavior (e.g., header-only libraries): Simon’s proposal in !1294 (merged) moves preprocessor definitions into compiler flags, ensuring we can maintain optional behavior.

This approach keeps both (1) and (2) intact. Additionally, while we could always achieve (1) and (2) before version 2.10, we also needed:

A fully configured project post-installation: This means no delayed configuration generation for the downstream user (no reliance on the DUNE build system after installation). Maybe used for setups without CMake.
Backwards compatibility: This was a crucial factor since we didn’t want users to overhaul their modules unnecessarily.

In #234 (closed) and #252 (closed), we essentially realized that combining (1) and (2) in the same configuration file would push the configuration process onto the end user (again). This approach is in conflict with our goal of having a fully configured project after installation (3).

The backward compatibility (4) issue was a major problem. Many of the alternative proposals in #234 (closed) would have broken compatibility, requiring significant changes to all modules. After many discussions—both online and during DUNE meetings—we found a compromise with !1262 (merged) and !1294 (merged). This solution allows most modules, especially those following common patterns like dune_project/dune_finalize (and possibly enable_all_flags), to remain unaffected.

The main point I want to emphasize is that many DUNE modules, including those using Python bindings, did not need to modify their configuration files or build systems to benefit from this change. They are still able to meet the goals of (1), (2), and (3), while ensuring backward compatibility.

If anything, the configuration of modules has actually become much easier and powerful. However, it does require a shift in paradigm, especially for modules like OPM that used to rely on custom configuration files based on the old approach.

one issue that I see related to your reported problem: when someone writes a header file that also defines HAVE_UMFPACK, then it might come to multiple definitions. The problem is lated to the issue that our macro flags are mot prefixed with DUNE_. Another issue might be if you write your own config.h file that is not generated by the buildsystem and does not follow the style of the autogenerated file then it might also have definition that otherwise are orovided already in dune config.h files, but relys on a different include guard.

a solution to your problem could be to simply include the individual dune-module-config.hh files in your user-defined config.h file. for all modules you are using. See the auto-generated config.h files

maybe just remove any macro definitions from your user-defined config.h file that was intriduced in any core module and add only those new variables that are introduced in opm. the core config.h files are mostly included automatically alongside the header files that need these defines. A few exceptions are there, though. For example, if you check versions of dune core modules with the DUNE_VERSION macros you still need to include the corresponding module config before.

We cannot do either of this as we need to support multiple DUNE versions (there are clusters with Ubuntu 20.04/22.04 LTS) and we cannot use any DUNE build system magic. The latter is actually now implied by the former, as the behavior seems rather unstable. Backwards compatibility here means that one is safe as long as one only targets one DUNE version and does nothing special.

Actually, we explicitly tried to be backwards compatible with mixed versions. The generated config.h file even supports this situation.

What has the ubuntu version to do with the dune versions? I think that all the versions should be supported on these ubuntu versions easily.

the ubuntu version used on a cluster or company machine often implies the DUNE version for non-expert users.

Note, redefining any preprocessor variable used in the dune config.h files is a bug, but in the downstream code.

How could a self written config.h look like? Let's say you have a module that depends on all core modules >= 2.10, then write

#ifndef MY_MODULE_CONFIG_H
#define MY_MODULE_CONFIG_H

#include <dune-common-config.hh>
#include <dune-geometry-config.hh>
#include <dune-grid-config.hh>
#include <dune-istl-config.hh>
#include <dune-localfunctions-config.hh>

// my other defines needed for the MY_MODULE module

#define XYZ 123

#endif // MY_MODULE_CONFIG_H

If you do not depend on deprecated preprocessor variables like HAVE_UMFPACK in your own code, and do not need to check for dune module versions via the macros DUNE_VERSION_GT(...), for example, you most probably can even omit the include of the individual dune-module-config.hh in your own config.h file and just set the extra flags that you need for opm. Thus, writing a config.h file for a library that uses Dune without the build system became even simpler than before.

Note, redefining any preprocessor variable used in the dune config.h files is a bug, but in the downstream code.

This seems a bit rude. We have never redefined anything in config.h. YOu are now suddenly defining Macros in normal headers. We have defined what needed to be defined to use DUNE, just without relying on the build system.

IMHO this is exactly what DUNE could do instead of including such a header in header that merely defines macros.

On our side we cannot include headers that are not in older versions. We need to support multiple versions and DUNE is making that harder and harder. This is fine if there is a real benefit. I just don't see it here, at least for us.

I am sorry. Didn't mean to be rude.

I try to come up with a solution for your problem, but the description was a bit incomplete. Can I summarize your setup?

OPM uses Dune but not the Dune buildsystem
You are using one code that must work with multiple different dune core versions?
You are writing your own config.h file, since it is not generated by the buildsystem automatically.
Any further constraints I should be aware of?

In the new setup, we have dune-<module>-config.hh files that can be included in header files. That is their purpose. In the old setup, these files didn't exist and one has to define all the macros.

The easiest solution is again the way the current config.h file is generated. It is nothing else than a concatenation of all the dune-<module>-config.hh files, with all the correctly set include guards to prevent from multiple definitions. Also, there is the __has_include(<...>) macro that is used to conditionally include the config.hh files:

#ifndef MY_MODULE_CONFIG_H
#define MY_MODULE_CONFIG_H

#if __has_include(<dune-common-config.hh>)
  #include <dune-common-config.hh>
#elseif !define(DUNE_COMMON_CONFIG_HH)
  #define DUNE_COMMON_CONFIG_HH
  #define DUNE_HAVE_CXX_EXPERIMENTAL_IS_DETECTED 1
  #define DUNE_HAVE_CXX_UNEVALUATED_CONTEXT_LAMBDA 1
  #define DUNE_HAVE_CXX_STD_IDENTITY 1
  #define HAVE_BLAS 1
  #define HAVE_LAPACK 1
  #define HAVE_TBB 1

  /// \deprecated Use HAVE_SUITESPARSE_UMFPACK instead
  #define HAVE_UMFPACK HAVE_SUITESPARSE_UMFPACK
  #define LAPACK_NEEDS_UNDERLINE
#endif

#if __has_include(<dune-geometry-config.hh>)
  #include <dune-geometry-config.hh>
#elseif !defined(DUNE_GEOMETRY_CONFIG_HH)
  #define DUNE_GEOMETRY_CONFIG_HH
  // ...
#endif

#if __has_include(<dune-grid-config.hh>)
  #include <dune-grid-config.hh>
#elseif !defined(DUNE_GRID_CONFIG_HH)
  #define DUNE_GRID_CONFIG_HH
  // ...
#endif

#if __has_include(<dune-istl-config.hh>)
  #include <dune-istl-config.hh>
#elseif !defined(DUNE_ISTL_CONFIG_HH)
  #define DUNE_ISTL_CONFIG_HH
  // ...
#endif

#if __has_include(<dune-localfunctions-config.hh>)
  #include <dune-localfunctions-config.hh>
#elseif !defined(DUNE_LOCALFUNCTIONS_CONFIG_HH)
  #define DUNE_LOCALFUNCTIONS_CONFIG_HH
  // ...
#endif

// my other defines needed for the MY_MODULE module

#define XYZ 123

#endif // MY_MODULE_CONFIG_H

Note that there are also differences in the config.h file between dune versions, e.g., in older dune version we had the MPI workaround flags put into the config.h file. This is not necessary anymore because they are moved into compile flags directly. So, there are just very few flags remaining. If you want to support also dune 2.9 or even dune 2.8 or older, you need to merge all the flags. Actually, how to you know which dune-common version to set in your custom config.h file? Or do you write such a file for each dune version that you support?

We have defined what needed to be defined to use DUNE, just without relying on the [DUNE] build system.

@markus.blatt I understand that the build system change is very annoying for you, and will need a bit of time to figure out. We will help with this transition! But please note that this is exactly the problem that we are trying to solve in this release: We used to hand in modules that were not really configured (!), technically useless without repeating whatever magic the DUNE build system did.

What you achieved to avoid the DUNE build system is impressive, but it now needs a shift in paradigm #379 (comment 140391) to allow other users also benefit from this kind of usage without the DUNE build system.

This is fine if there is a real benefit. I just don't see it here, at least for us.

The overall goal is that eventually you do not need to have any custom CMake magic in the downstream package to be able to use dune. Something along these lines:

cmake_minimum_required(VERSION 3.16)
project("opm-foo" CXX)

# find dune-bar project: exports the target Dune::Bar
find_package(dune-bar)

# create a target library with native cmake
add_library(opm-foo INTERFACE)

# link to dune targets without need of dune-isms (e.g., custom generation of config files)
target_link_libraries(opm-foo INTERFACE Dune::Bar)

# enable an optional package (e.g. METIS)
target_compile_definition(opm-foo PUBLIC HAVE_METIS=1)

Wouldn't it be nice if this worked, and thus drop all the custom work-arounds to consume dune? Right now, this path is very painful because you have to support mixed versions with custom scripts. But once this is achieved and you drop support for older versions, you can free yourself of trying to maintain scripts in OPM to mirror whatever the DUNE build system is doing.

Indeed, if there is a benefit for someone, is for you guys ;) Most of the other users do not even question the need of dune_project/dune_finalize and other dune-isms...

What is the reason not to define the DUNE version stuff in the CMake targets for the libs?

There was a long discussion, e.g., #234 (closed) The general tone was to not clutter the command line with unnecessary many flags. But it would make things much easier since it can directly be exported.

You don't see the command line unless you do debugging...

So now we are somewhere in-between config.h and targets. I see. So this is not an intermediate step but supposed to stay. Then my impression that we would go fully to targets was obviously wrong and the reason for my confusion.

Anyway, I have solved the problem. Seems like there are only warnings if defines are syntactically different. We had #define HAVE_UMFPACK 1 and you guys define it to the suitesparse one. Athough both should resolve to the same value, I get an error. Well, HAVE_UMFPACK is not used anywhere in DUNE 2.7-2.10 core so the solution was to simply skip it. I should probably check staging and extensions.

So now we are somewhere in-between config.h and targets. I see. So this is not an intermediate step but supposed to stay. Then my impression that we would go fully to targets was obviously wrong and the reason for my confusion.

You are right. We are not yet where we want to be. We can only go small steps. I hope we can get rid of the config.h file eventually and have a different mechanism to communicate options and activated packages to the dune libraries. As we advertised, for Dune 2.10 we do not need the config.h file in most places anymore. There are just a few leftover situations we need to deal with. This already allows to write projects consuming Dune without the need to provide their own config.h. But a few other things are also still open, e.g., #332 (closed), !1355 (merged), !1261, !910, #345...

mentioned in issue #381 (closed)

Thanks for testing and providing feedback! I feel a bit alienated, as this is the second topic concerning major changes that we discussed and merged for quite some time and we restart the discussion right at the doorstep of the upcoming 2.10 release.

A general question, what is the rationale for OPM to create their own build system? Can we learn from their need for Dune? I always perceived OPM as a downstream Dune module similar to Dune-FEM, PDELab, or DuMuX.

added discussion label and removed bug label

I am closing this. Thanks for the help and the discussion.

closed

mentioned in merge request !1434 (merged)

config.h values included in official DUNE headers

Designs

Child items ...

Activity