handling of `config.h` (proposal for dev-meeting 22.1.2021)

I am very much in favour of this ! (some discussion can be found here) Overall, the plan looks very good to me.

Thanks @christi for the detailed summary. I would just like to ask for a breakdown of what exactly a developer of a dune module needs to do to get this to work:

If I understand correctly then the include statement for config.h needs to be removed from all cc files?

In addition what changes are needed e.g. to the CMakeLists.txt files?

This change will influence dozens of dune modules (all of them I guess) so needs to be communicated very clearly to everyone (and since I'm responsible for quite a few modules I would like to know what this change will means for me)...

Yes, this change will impact many modules. How much you have to change depends strongly on your packages.

Assuming you use enable-all-packages, the changes are relatively small:

don't include config.h
update you own cmake/module/... checks to work similarly
update your root CMakeLists.txt to call the generateGridSelector function

Something like this should do the trick. Point (2) is something you most likely have to do anyway, once @simon.praetorius and @christoph.gersbacher proceed with their cmake cleanup.

The actual CMakeLists.txt should still work, as you didn't have to add target dependencies manually.

If you did the later, I guess you used the convenience function add_foo_flags. This function should still work, but perhaps it is now implemented in a slightly different way.

config.h include: If you have protected the includes of config.h with #if HAVE_CONFIG_H or something similar, then there is no hard requirement on removing this include. We should just not set this HAVE_CONFIG_H macro. Eventually, it would be better to remove these include, though, in order not not irritate users of your code what this non-existing config.h should be. Thus. without breaking existing code and preparing for possibly future removal of a config.h file, one should protect the include.
HAVE_FOO cpp variables: For the beginning, we should somehow preserve the existing variable names HAVE_FOO, or if changed add a fallback define with a deprecation. So, it will probably not be a hard requirement to change any of these variables in your code. It is just a different way of defining these variables. Either as compile flags, or inside the config.h file. All compile flags are automatically appended to all targets if they are linked to the upstream module targets (or using the variable ${DUNE_LIBS}. The dune-tests will automatically be linked and thus get these flags.
HAVE_FOO cmake variables: The cmake variables should not necessarily be touched. As @christi mentioned: if you include add_dune_foo_flags and use this macro, it should directly work and preserve the cmake variables HAVE_FOO and FOO_FOUND.
Transition of config.h files: A config.h file is created if finalize_project is called with at least 1 argument. So, for the core modules, we could simply update the config.h content, move everything either to the corresponding module header (like the LAPACK_MANGLE) or the list of compile flags and then simply do not set this argument to the finalize macro. In a downstream project, this could still be created. But then all dependent modules must also create their config.h file.

to 2. I would very much like to preserve the HAVE_FOO names, otherwise a lot of code has to be changed and these things are particularly difficult to find and debug. So keeping the names and have a little disturbance is usually a good idea in the bigger changes like this.

Yes. This is what I also wanted to say, actually :-)

I agree to remove the macros such as GridType from config.h.

I would suggest to keep the variables, like before, in a module specific header file that is created during configuration, e.g. for dune-grid

dune-grid/dune/grid/variables.hh

which contains the HAVE_FOO variables. This header needs header guards (#pragma once) and should be included by each header and source file in dune-grid.

Whit this we will not clutter the compiler command line with a lot of -DHAVE_FOO and still be able to achieve all of the above improvements.

In general, having for each module a header file that is created during configuration and stored and installed is (kind of) just a different way of passing compile flags for the module target. So, in general I'm fine with that idea. But it must be guaranteed that this variable is not regenerated in each cmake run of downstream modules and, as you said, must be included in each (header and source) file of the corresponding module.

The question is, whether this is more readable or maintainable than simply adding the HAVE_DOO flags to the compiler flags. Note, that if we have a config.h file (or variable.h file) we might still require to have for each/some HAVE_FOO flag a corresponding ENABLE_FOO flag in the list of compile flags. This is, because people want to disable or enable packages manually after configuration.

In general, having for each module a header file that is created during configuration and stored and installed is (kind of) just a different way of passing compile flags for the module target. So, in general I'm fine with that idea. But it must be guaranteed that this variable is not regenerated in each cmake run of downstream modules and, as you said, must be included in each (header and source) file of the corresponding module.

I don't like the idea that all headers have to include some special generated file.

There is an other possibility, the flags can be stored in a generated header, but IMHO the include should then be enforced via a command line option. This definitelly is possible for gcc and clang.

The question is, whether this is more readable or maintainable than simply adding the HAVE_DOO flags to the compiler flags. Note, that if we have a config.h file (or variable.h file) we might still require to have for each/some HAVE_FOO flag a corresponding ENABLE_FOO flag in the list of compile flags. This is, because people want to disable or enable packages manually after configuration.

The suggesttion is that we get rid of the ENABLE_FOO flags (in a transition) by replacing them with appropriate targets that can be added as a dependency.

I also think that the forced inclusion of that specific header in all files it a bit strange, but the other options seem to be dropping compiler support and problems with the installation if I understood that correctly. Adding all HAVE_FOOs as -D compiler flags I find not acceptable, but maybe that is what other packages do.

Adding all HAVE_FOOs as -D compiler flags I find not acceptable, but maybe that is what other packages do.

This is the "best-practice" cmake way.

I'm also not 100% convinced, but beeing able to have all defines availabel without generating an additional header is a necessary step to be able to have user modules without the dune buildsystem (which is in my opinion a desirabel goal).

The generated header in the core module does not hinder downstream modules to add -D... There was another comment by @simon.praetorius about the fact that in this case you will never see where these defines come from unless you manually inspect the compiler flags. Not sure whether this is a good idea. And as I said, not documentation will be possible.

boost seems to have a config.hpp which contains a lot of other config includes which is a variety of configurations for different compilers, platforms etc. I don't think that the -D version is very practical.

deal.ii also generates config.h file: https://www.dealii.org/9.1.1/developers/cmake-internals.html#config.h.in

Adding all HAVE_FOOs as -D compiler flags I find not acceptable, but maybe that is what other packages do.

This is the "best-practice" cmake way.

I'm also not 100% convinced, but beeing able to have all defines availabel without generating an additional header is a necessary step to be able to have user modules without the dune buildsystem (which is in my opinion a desirabel goal).

@christi @robert.kloefkorn

How many times do you use VERBOSE=1 each week?
Can't grep do the job and inspect what you are trying to find?
Is there other problems with compiler flags that I don't see?

VERBOSE=1 is the default. So every time I compile something.

Let me understand: you don't want to clutter your terminal with output (even when you are asking for verbose output) therefore you suggest making complicated header inclusion rules into the build system in order to avoid it. Is that right?

I'm using the -DALLOW_CXXFLAGS_OVERWRITE flag for the cmake system which produces the compiler output by default. Right now it's on the order of 8-10 lines per object file or target. That is fine. Using dune-fem and I guess this must be similar with other modules, you will get on the order of 90 HAVEs and HAVENOTs. I simply find this not practical. There is other reasons that are explained in the comments above (below).

I didn't suggest to make complicated header inclusions, I was laying out options. The option I favor is relatively simple and will not cause any major disruption to a well functioning system and yet solve the pkg-config problem. That is in my opinion a major plus point. And, as I reported above (or below), other large C++ template libraries like boost are doing the same and there cmake is used. So maybe you can reach out to them and ask why they did not choose the cmake way of including -D.... It would be very interesting to hear the answer.

The idea of using -include is nice as long as it is possible, transitive, and maintainable. If it is, I will stand for it. But from the comments that I gathered so far, it may not be as maintainable as it looks like, therefore I tag it as "complicated".

But let's be clear, my problem here is not your proposal. It's that you are immediately ruling out (and let me quote: "I find not acceptable") the simple, transitive, maintainable, and recommended solution because it produces too much output to your workflow. That is what I don't understand.

So maybe you can reach out to them and ask why they did not choose the cmake way of including -D.... It would be very interesting to hear the answer.

Noted, I will try, I am also interested ;)

Currently there are 4 options on the table:

leave as is and discard pkg-config
-D ...
include dune-module-variables.h in each header file
-iincludefile

I'm ok with 1) or 3). But I did not rule out anything. I'm merely discussing the options. In my opinion 2) the -DHAVE_FOO is not an acceptable (as in not practical) replacement. It also differs from the current approach and is thus more disruptive and will lead to a lot of work on downstream modules. And, if it is really the best practice for cmake then we should be able to find C++ template libraries that demonstrate how this can be done in a good way. So far these examples have not been put forward. But I'm sure those must exist. 4) is probably not practical (as in not acceptable) because that flag is not supported by some compilers, such as intel. Therefore, I find 3) the least disruptive and thus most practical approach where I can see we can find a compromise. I also don't like to include the header in each header file but I have not found a better way yet.

I just made an experiment with the forced include, see !924 (closed) A few changes are necessary to the modules we want to update to include this module-specific config file, but all legacy modules or not-yet-updated modules can stay as before. No changes necessary. I tested some core modules.

Instead of removing all the config.h includes in the dune-common files, one could add the corresponding dune-common-config.hh file as first include line. This is only necessary for the files in the modules we update. So, again, no change in any downstream module necessary. But including a header in all header and source files needs to be scripted and changes a lot of files.

The problem with the fixed install prefix may not be a real problem. The prefix is set anyway. We cannot easily move an installed dune module somewhere else.

But the problem that not all compilers support this forced include remains.

BTW: I would not go so far to say that including all configuration flags like HAVE_FOO into the compiler flags is the recommended and best-practice cmake way. It is just one way that is easy to maintain, because it is communicated via targets transitively and we do not get problems with multiple definitions and stuff. But if we generate with our cmake system an include config.h file that is unique to the module, is installed, and needs not to be regenerated, I think that this is also common practice and not a bad solution.

I think the -iincludefile could be a very good transition option which delivers both, a solution to the pkg-config problem and minimal change. For compilers that do not support the -iinclude option we could still offer the current way using an adjusted local config.h

In my opinion 2) the -DHAVE_FOO is not an acceptable (as in not practical) replacement. It also differs from the current approach and is thus more disruptive and will lead to a lot of work on downstream modules.

In which way does it require a lot of work in the downstream modules? You now get the flags on the command line, in the transition phase the config.h will still be there, but with less defines.

Modifying every single header seems to be to far more disruptive.

Maybe it's a good idea to also create a test branch and then we can compare the solution.

See !925 (closed) for an initial experiment. It does not completely remove the config.h file, but this is also not possible, since the other points in this proposal must be completed before. Also, as @gruenich pointed out somewhere in this discussion, first some steps in the cmake cleanup must be performed, before we could implement this successfully (e.g. moving c++ standard and compiler features to target properties)

Is the ENABLE_FOO magic used? Does it still work? What happens if ENABLE_FOO=1 during installation of dune-grid and then later ENABLE_FOO=0 when using the installed dune-grid module?

Here is what I get when I grep one of my tool chains, 60 define HAVE_FOOs (see below). Do we really want a compiler command line with 60 or more -D? Does this work with cmake? That is why I think it might be a good idea to keep these in a file. The naming could be a problem but there we could have a protection with #ifndef ... and check that if there is two values that these have the same define (0 or 1).

Example of HAVE_FOOs:

#define HAVE_CONSTEXPR 1
#define HAVE_CXA_DEMANGLE 1
#define HAVE_DUNE_ALUGRID 1
#define HAVE_DUNE_COMMON 1
#define HAVE_DUNE_FEM 1
#define HAVE_DUNE_FEM_DG 1
#define HAVE_DUNE_FOAMGRID 0
#define HAVE_DUNE_FUNCTIONS 0
#define HAVE_DUNE_GEOMETRY 1
#define HAVE_DUNE_GRID 1
#define HAVE_DUNE_GRID_TESTGRIDS 1
#define HAVE_DUNE_ISTL 1
#define HAVE_DUNE_LOCALFUNCTIONS 1
#define HAVE_DUNE_OPTIM 0
#define HAVE_DUNE_POLYGONGRID 1
#define HAVE_DUNE_PYTHON 0
#define HAVE_DUNE_SPGRID 1
#define HAVE_DUNE_UGGRID 0
#define HAVE_DUNE_VTK 1
#define HAVE_EIGEN 1
#define HAVE_GMP ENABLE_GMP
  #define HAVE_GRIDTYPE 1
#define HAVE_IS_INDEXABLE_SUPPORT 1
#define HAVE_KEYWORD_FINAL 1
#define HAVE_LAPACK 1
#define HAVE_MEM_USAGE_T_EXPANSIONS 1
#define HAVE_METIS 1
#define HAVE_MKSTEMP 1
#define HAVE_MPI ENABLE_MPI
#define HAVE_MPROTECT 1
#define HAVE_NETCDF 1
#define HAVE_NOEXCEPT_SPECIFIER 1
#define HAVE_NULLPTR 1
#define HAVE_OPM_GRID 1
#define HAVE_PETSC ENABLE_PETSC
#define HAVE_PTHREAD 1
#define HAVE_QUADMATH ENABLE_QUADMATH
#define HAVE_RANGE_BASED_FOR 1
#define HAVE_SLU_CDEFS_H 0
#define HAVE_SLU_DDEFS_H 0
#define HAVE_SLU_SDEFS_H 0
#define HAVE_SLU_ZDEFS_H 0
#define HAVE_STD_DECLVAL 1
#define HAVE_SUITESPARSE_AMD ENABLE_SUITESPARSE
#define HAVE_SUITESPARSE_CAMD ENABLE_SUITESPARSE
#define HAVE_SUITESPARSE_CCOLAMD ENABLE_SUITESPARSE
#define HAVE_SUITESPARSE_CHOLMOD ENABLE_SUITESPARSE
#define HAVE_SUITESPARSE_COLAMD ENABLE_SUITESPARSE
#define HAVE_SUITESPARSE ENABLE_SUITESPARSE
#define HAVE_SUITESPARSE_LDL ENABLE_SUITESPARSE
#define HAVE_SUITESPARSE_SPQR ENABLE_SUITESPARSE
#define HAVE_SUITESPARSE_UMFPACK ENABLE_SUITESPARSE
#define HAVE_SUPERLU ENABLE_SUPERLU
#define HAVE_SYS_MMAN_H 1
#define HAVE_TBB 1

You are right. These are a lot of defines. This is no problem for cmake, though. You will not see these flags unless you inspect your compiler flags manually. But also @christi said in another discussion, that too many flags are not preferable. But, I was told, that the module-specific config.h file (or variable.h file) idea was discussed before and no-one has implemented the workflow. It might also require to change every file in every module, to include the correct configuration file. So, we need to work on the ideas. Maybe we find some compromise.

Yes, the ENABLE_FOO magic is working, this is the way we have tests with SuplerLU and without. Without the enable trick, we couldn't deactivate SuperLU because if found, it would be pulled in.

All the HAVE_ variables are serving a purpose: You can create an #ifdef HAVE_ in your code and use a fallback option.

I don't want to state, that there is no way without these mechanism, but there is a reason that we have them.

I do manually inspect the compiler flags by default (like make VERBOSE=1). So for me this would be not practical if all of a sudden there is a lot of -DHAVE_FOO and I guess it would also not be practical for others.

Added the header to each file could be done with a script right after the #ifndef, #define lines. This should be no problem. For almost all headers this is line 3. The few special cases can be fixed by hand.

I suggest to add an empty defaultheader (e.g. please come up with good names) and then this defaultheader includes the auto generated variables.h, if existing. Something along these lines.

If the dune_project() + finalize_project() macros are used then definitely, one could generate a default header with a generic name, e.g. dune/module/dune-module.h or dune/module/config.h or whatever.

I have to think about that. Maybe it is not too complicated to implement.

I forgot to mention that there is an option (at least for gcc and I think also clang) to include header via compiler command line before all other headers are included. One could use

-idune-module-path/dune-module-config.h

to add the correct location of the dune-module-config.h to include the header with the HAVE_FOOs. This way we don't even have to change any code in the corresponding dune modules. I have been using this in dune-alugrid since a year or even longer and I guess nobody even notices, meaning it seems to work. Then the proposed workflow would be:

Split the HAVE_FOO into dune-module-config.h for each module which is installed alongside all other headers.
Add a '-idune-module-path/dune-module-config.h' for each module like the other flags to INCLUDE_FLAGS, e.g. -I, etc.

This should in my opinion be the simplest and thus most preferable fix the pkg-config/config.h crisis.

In general, this would indeed simplify things and I will play around with this idea to see whether there are limitations and how to implement this in a proper way.

Some initial thoughts:

including multiple config files might lead to the problem of multiple definitions of constants. This could be circumvented by corresponding define guards, though.
the forced include flag -include or /FI is a compiler-specific extension. It needs too be checked whether this is available on all used compilers. And a stable cmake macro to add this flag needs to be developed.
hiding an actual include file might be irritating to users that could ask: where does this or that definition comes from? It is not found in any included files. Also IDE might get confused by this. On the other hand, include a config.h file that does only exist inside the build directory is not better.

An implementation idea: Think of the config.h file as of a precompiled header (PCH). This can be set as a target property and cmake handles automatically the forced include of that header. Unfortunately, the PCH feature is available from cmake 3.16 only.

We just bumped the required cmake version anyway - it might be worth discussing to go to 3.16 (after a 2.8 release) if that opens up other options. I don't know which versions are available on which machines but I had the feeling that those who needed to upgrade ended up with something like cmake 3.18 anyway?

Regarding point 2: I think gcc and clang implement the -include flag, MSVC implement /FI, but intel icc only implements /FI on windows systems (this is what the documentation says). Don't know whether this flag is implemented in other compilers.

I would not worry so much about Windows as long as the code compiles using the Linux Subsystem.

@andreas.dedner: How about bumping to cmake 3.19.3?

I already make a remark on that

We should not restrict to just gcc and clang only. Intel is a meaningful compiler option.

@simon.praetorius: I don't see the problem of multiple definitions of constants, because if we had that now we would already see it since all constants are in one file. Besides that it could be guarded via an extra #ifndef.

The problem is that we get the HAVE_FOO in multiple packages. E.g. HAVE_DUNE_COMMON will be (and must be) defined in all downstream modules. It should no be allowed to include the dune-common-config.h in dune-grid, etc. This needs the be guarded using #ifndef. It is not a big problem. I just list some issues.

It should no be allowed to include the dune-common-config.h in dune-grid, etc

Out of curiosity: Do you mean that a dune-grid header foo.hh would disable inclusion of dune-common-config.h for dune-common headers included within foo.hh? Wouldn't this break the dune-common header if dune-grid-config.h is missing some dune-common-specific defines?

As far as I have understood the idea, one would include a header from dune-common, e.g., dune/common/fvector.hh. This header must itself include the dune-common-config.hh header to get the defines set by dune-common. The file in dune-grid, e.g., dune/grid/common/grid.hh will include first the dune-grid-config.hh header and then any other header, including (transitively) the fvector.hh and thus also the dune-common-config.hh

So, the dune-common-config.hh file only provides the defines set in the configuration of dune-common, e.g. which external packages are found. The dune-grid-config.hh provides defines for all external packages found in the cmake configuration of dune-grid only and also the defines which dune modules are found, e.g. HAVE_DUNE_COMMON.

Now I get your question: No, I would not explicitly disable the inclusion of the dune-common-config.hh header from from file in dune-grid. I just mean, that this header should not be included explicitly by any other module than dune-common directly. If we implement the -include flag, then one would not get any explicit include, either.

The problem is that we get the HAVE_FOO in multiple packages. E.g. HAVE_DUNE_COMMON will be (and must be) defined in all downstream modules. It should no be allowed to include the dune-common-config.h in dune-grid, etc. This needs the be guarded using #ifndef. It is not a big problem. I just list some issues.

It is even worse and that is my reason for proposing to rename these flags eventually...

We now work on a way to use dune without the dune build-system, so the user can quickly run int the problem that his/her buildsystem sets the same defines. Up to now this could not happen, as the defines were always generated in the module and with the same checks as in the "parent" module, but now we will now inherit them from above and the buildsystem has no chance t detect that there might be a "collision".

We could even have the situation that certain features are disabled in the dune modules (e.g. because you are installing from package), but the user wants use this external package in his/her application (perhaps for something completely different).

Another issue:

Having a forced include (-include) could be complicated when having build and install interface. One has to set an absolute path to the file that is included. So we loose the relocateability. While we can have different include-path for build and install interface, both absolute paths must be known at configure time.

After the above summary of different options the most sane version seems to be having a dune-module-config.h (or dune-module-variables.h) that is includes in every header file of the module. Then we don't have to worry about compiler compatibility, we don't distinguish between installed and not installed and it is always clear where the defines come from. Moreover, the be dune-module-variables.h also allows to add some documentation to the variables (as we have it now). To safeguard the against clashes with multiple defines of HAVE_DUNE_COMMON we simply add something like this to our cmake macro that generates the config file:

#ifndef HAVE_DUNE_COMMON 
  #define HAVE_DUNE_COMMON VALUE
#else // check that already defined value is the same
  #if HAVE_DUNE_COMMON != VALUE
    #error 
  #endif
#endif

It might also be a good idea to shop around a little bit how other software packages do it. I guess the traditional way, like PETSc and many other packages do it, is to have one header file that has to be included. This does not work well in our case because this would increase compile times dramatically, since most of the code is in headers. Is there similar C++ libraries where we could get ideas what other options might be?

We have the "complicated" situation, that Dune is not just one software, but a suite of an unknown number of modules. However, this might somehow be compared to PETSc (also a suite of mainly external packages) or SuiteSparse, maybe Trilinos.

In the Meson build-system documentation, I've found the following paragraph: https://mesonbuild.com/Wrap-best-practices-and-tips.html

I suggest to add an empty defaultheader (e.g. please come up with good names) and then this defaultheader includes the auto generated variables.h, if existing. Something along these lines.

This is not possible. The issue is that your module has to include the generated headers for all dependent and suggested modules. An additional complication is that you have to know which suggested modules are found.

The idea was that 'the header` would be included in all headers in a dune module. So whenever you include a header from a dune module somewhere else you will also get the correct config header included. Or am I missing something?

I have understood the idea as @robert.kloefkorn explained it. We get the transitive include of all config files by directly including the module headers.

I have understood the idea as @robert.kloefkorn explained it. We get the transitive include of all config files by directly including the module headers.

But if we want to use a module that has a suggestion, how can this module include the correct header?

At least there must be a define HAVE_DUNE_FOO on the command line.

I hope I understand this correctly: Let's say module dune-b suggests dune-a, (and dune-a is found), then in the file dune-b-config.h we get the line #define HAVE_DUNE_A 1. So, in any header of dune-b, we could check whether dune-a was found and then include its header files:

#include <dune/internal/dune-b-config.hh>

#if HAVE_DUNE_A
#include <dune/a/header.hh>

// ...
#endif

That's also how I thought it should (could) work.

-idune-module-path/dune-module-config.h

this is what I ment with "force the inclusion".

Having a forced include (-include) could be complicated when having build and install interface. One has to set an absolute path to the file that is included. So we loose the relocateability. While we can have different include-path for build and install interface, both absolute paths must be known at configure time.

but we have to specify the prefix anyway before installation.

@christi I have experimented a bit with the forced include strategy. In general this would simplify things for the user. But I've not yet found the right way to do it. I will upload a "playground" MR to experiment with this strategy a bit.

!924 (closed)

OK, at least I've found a small bug in my code. If we know the installation prefix, we can hard-code the forced include also for the install interface.

directly found a bug in a test in dune-common, because of a missing include of config.h :-)

Why do you have to give the install prefix, can't you just deliver that header on the dune/<module>/ folder? e.g. here and here works for me, even on installation.

or does the -include need an absolute path? If so, using it is not a good idea at all.

It might be that I do something wrong. I do not have much experience with the forced include. In some initial tests, the compiler complained about relative paths in the include-header name. Installation of the config file is no problem

mentioned in merge request !924 (closed)

I'm trying to follow this - not easy but here is what I gathered so far:

the option of changing each header file (and have to remember to do this for each new header file) is not very nice. It is not difficult to do obviously with some script but it lacks elegance....
the option of the forced include seems the nicest approach but has the issue with the intel compiler
finally the -D option would make verbose output really difficult to parse as Robert said
I did want to point out that @simon.praetorius did suggest the approach of precompiled headers. As I said above bumping cmake to 3.16 is something we could at least consider if it solves this issue even more elegantly and independent of compilers. We have already move to 3.13 recently so it's perhaps a good time to do this.

PS: I'm also trying to decide if PCH would greatly improve compile times for the Python bindings since for example compiling the Python module for the VTKWriter (as example) needs the include for the grid type as well. But those were also parsed to import the grid itself so perhaps that step could produce a PCH. I've not gotten around to testing this feature and with cmake support that would become easier to do.

Unfortunately, the PCH feature is not supported by the intel compiler, either. (As far as I could get from some googling) Probably because the forced include is used in combination with PCH.

Also, I don't understand how this would play together with dune-foo-config.cmake or with pkg-config. As far as I understand pch, it is a feature at the level of a particular application and not of a library. As there can only be one single pch, we can't distribute/install them with the modules and thus we can't use it to carry the defines... or did I miss something?

@christi much more likely that I am missing something. I just didn't want Simon's remark from the beginning of the issue get lost and hadn't been mentioned again - probally for a good reason

My initial idea was probably not so good. I think now that using PCH for the forced include is the wrong way. PCH should just be used for headers that are included anyway. Then it would be no problem with pkg-config, because you would just include the headers in a regular way. But for forced included headers that would not be included otherway, it would crash. So, a bad idea for our config.h problem.

Why is it difficult to create the config.h in dune-geometry for the dune-common parts?

Either we have all CMake variables (and targets) from dune-common in the CMake configure from dune-geometry, than we could use dune-common's config.h.in and re-generate it.
Or we do not have all CMake variables. But I don't think this will work at all. We would be also missing FOO_Found and the conditionally test execution breaks; and we cannot use the targets like dune-common to link to dune-geometry.

I think we should first figure out how to get the CMake variables to downstream modules. And to me this seems to be only possible with a super-build approach or treating all modules as what CMake calls an external project. Once we found a way, the config.h issue becomes far more easy.

Sorry to disagree, but if we want to reduce the dune specific parts on the buildsystem and support something like dune-foo-config.cmake or dune-foo.pc, we need to change the handling of config.h. Thus I started the discussion, because if we don't have a general agreement, that config.h in the current form should vanish (command line arguments or some module specific config header are then just different technical options to achieve the same goal), we have quite hard restrictions on the cmake cleanup and some things are simply not possible at all.

PS: sorry for the auto-correction mess... I manually cleaned up now...

Ok, I see your point of view. Thanks for starting this fruitful discussion!

mentioned in merge request !925 (closed)

There is a small comparison of the two approaches (at bit old but gives an impression) on stackoverflow: https://stackoverflow.com/questions/20833226/library-headers-and-define/20838147#20838147 It is a minimal example, though, and might not be directly comparable to our more complicated situation.

Another nice idea from Trilinos: Automatically put some of the compiler and linker arguments automatically in "response files": https://docs.trilinos.org/files/TrilinosBuildReference.html#enabling-the-usage-of-resource-files-to-reduce-length-of-build-lines This works for include flags and links flags very well. But I have not yet found the corresponding option for cflags and cxxflags.

I just checked...

While the feature is mainly advocated on windows systems (as older Windows versions support only relatively short command lines), both, gcc and clang, support feature files via the @file option.

E.g. ...

dune-common-config.res

-DDUNE_COMMON_VERSION="2.8-git"
-DDUNE_COMMON_VERSION_MAJOR=2
-DDUNE_COMMON_VERSION_MINOR=8
-DDUNE_COMMON_VERSION_REVISION=0
-DDUNE_MINIMAL_DEBUG_LEVEL=4
-DHAS_ATTRIBUTE_DEPRECATED=1
-DHAS_ATTRIBUTE_DEPRECATED_MSG=1
-DHAS_ATTRIBUTE_UNUSED=1
-DDUNE_HAVE_HEADER_EXPERIMENTAL_TYPE_TRAITS=1
-DDUNE_HAVE_CXX_BOOL_CONSTANT=1
-DDUNE_HAVE_CXX_APPLY=1
-DDUNE_HAVE_CXX_EXPERIMENTAL_MAKE_ARRAY=1
-DDUNE_HAVE_CXX_EXPERIMENTAL_IS_DETECTED=1
...

and then the command-line would be something like

$(CXX) $(CXXFLAGS) @$(DUNE_COMMON_PATH)/dune-common-config.res ...

I really like this idea.

Update... works for

and for sure for msvc, which we don't care about...

Trilinos mentions some restrictions of this technique and reasons why they do not activate this by default. Unfortunately, cmake does not generate these response files for all compile flags, but only for include directories.

So, we would be responsible for generating this file within our cmake framework.

Do these response files also support comments starting with # or //?

Maybe we could do a similar procedure as for generating the config.hh include files for generating a .res file, i.e., simply providing a dune-common-config.res.in template that is configured by cmake automatically.

You might want to have a look at this (very short) thread: https://cmake.org/pipermail/cmake/2007-May/014292.html

It shows how you can use cmake to generate a response file.

And at least for icc and gcc they can contain comments (prefixed with #).

This thread just shows how to generate a list of link objects and libraries, I think.

Yes, but the way it generates these doors not depend on the type of flags.

This response file idea could be an addon to the compile-flags proposal, i.e., we communicate all flags as compile flags using target_compile_definitions(dunecommon PUBLIC HAVE_FOO) and on request, we generate a response file similar to the config file generation that is performed now, or like I generate a module-dependent config file. For the compile-definitions proposal experimental branch, I have already added a function dune_target_compile_definitions that could add the definition to an additional target property that is transformed into that @file.

What needs to be checked is, whether we can communicate this response file property to installed targets. I mean, we have to install the response file and add a corresponding compile option also for the installed exported target.

I have also made some tests. GCC seems not to support comments (prefixed by #) in response files. Installation is not a problem. Also it is allowed to have multiple of these files.

mentioned in issue #199 (closed)

Independent of the way we choose to go with the config.h replacement, we need to cleanp the current file. Some merge requests are related:

!951 (merged) (Move definition of LAPACK_MANGLE and FC_FUNC to fmatrixev.cc)
!950 (merged) (Remove unused and undefined cmakedefines from config.h)
!949 (merged) (Remove HAVE_MPROTECT cmake check)
!934 (merged) (Remove cxa demangle cmake check)
!933 (merged) (Remove cxx feature checks that are already in c++17 standard library)

mentioned in issue #252 (closed)

I was thinking a bit more about the config.h topic and the two solutions we have discussed (module-config.h or compile flags). Somehow I'd like to get rid of the ENABLE_FOO flags. Currently those are handled as additional compile flags. If we would have something like module-config.h containing only those HAVE_FOO (and other configuration) defines that cannot/should not be changed by the user and additionally add the HAVE_BAR flag for only those packages that had an ENABLE_BAR before, we wouldn't have more parameters on the commandline and also the config.h file is reduced. This does not solve the problem of adding a new include statement into all header files, but it would allow to handle some packages directly with cmake target properties.

Note, this could even be started now with the global config.h file. We wouldn't change much in the workflow but simply replace the ENABLE_FOO by HAVE_FOO in the compile flags and remove the #cmakedefine HAVE_FOO ENABLE_FOO from the config.h file.

See also #252 (closed) for a discussion of alternatives to the ENABLE_FOO magic.

I checked up this again. I think the solution with header files is actually very is simple to implement (if we exclude the dune-grid magic):

Generate a public and a private configuration files for each module (easy to automate since config files already have those definitions)
- Public: Contains the definitions needed for its usage in downstream modules, including upstream public configuration files (i.e. #include <dune-grid-config.h>). This header is guarded to be included once.
- Private: Includes the public header (e.g., #include <dune-grid-config.h>), and the module information that should not be exported nor be visible outside of the module (i.e. #include "config.h").
The module only installs the public header.
Downstream modules consume all files of immediate dependencies.

NOTE: This requires to change the generation of config.h.cmake from downstream to upstream configuration time. This is needed anyways since this is incorrect! If CMake resolves different compiler definitions between upstream and downstream module, this set up will make upstream binaries incompatible with the new headers created in downstream. Notice that all other proposals also imply this change.

Before anything can be implemented, one has to decide how to handle the dune-grid type magic: It turns out that the grid type magic is dependent on the order of definition. They must be included after all other modules have set up its own definitions. See flyspray/FS#1713 (closed) and c35b018a. Thus, none of the solutions above will work until this is resolved outside of the configuration files.

Ok, I checked the logic. This is necessary because the config.h is resolved in backwards direction (i.e. downstream module information is written first). If we treat them as normal public/private header files as described in #234 (comment 128662) this logic is inverted, then, the problem is solved.

Out of the core and extensions modules, the following definitions in dune-common are a problem:

/* does the standard library provide experimental::is_detected ? */
#cmakedefine DUNE_HAVE_CXX_EXPERIMENTAL_IS_DETECTED 1

/* does the language support lambdas in unevaluated contexts ? */
#cmakedefine DUNE_HAVE_CXX_UNEVALUATED_CONTEXT_LAMBDA 1

/* does the standard library provide identity ? */
#cmakedefine DUNE_HAVE_CXX_STD_IDENTITY 1

With all options in this proposal, these definitions will be resolved in the upstream module (i.e., dune-common). That's OK for binaries created within the upstream module. However, these definitions may not have been resolved to the same value on the downstream anymore (e.g. dune core installed with package manager, and user finding a newer compiler available in her environment). In a true modular design, they have to resolved again with a different name for each module that needs them (e.g. DUNE_<module>_HAVE_CXX_EXPERIMENTAL_IS_DETECTED). It is fine that they resolve to different things for each module as long as they do not cross the ABI boundaries (which is relatively easy to check in the core modules). Also, as I pointed above, the current state is also not correct as binaries may end up having different linkage names or ABI altogether. Since DUNE is mostly header only library, ABI errors are more unlikely and we didn't see this problems explicitly, but resolving these definitions in the upstream module and using them in the downstream module would have more surface for errors.

Thus, I suggest that these cross-module definitions need to be deprecated in favor of module specific ones.

On the one hand you are absolutely right: these flags depend on the actual compiler used. That is why we said: changing the compiler or any compiler flag (e.g. debug options) in between compiled modules is undefined behavior and should not be done. Unfortunately, this is not always in our hand. An example is: some modules are debian packages (compiler with some standard debian compiler) and those might be mixed with user provided modules (compiled with a possibly different compiler)

If we would make these flags module dependent, i.e., include the module name into the flags, might also not work. What do you write as flag in your implementation? You can only write the flag name from the current module, not from any possible downstream module. But there it might be used. We have already the same issue with the compiler flags like NDEBUG. You could, theoretically, change this between modules. Then some functions are compiled (and maybe put into a library) with and in the next module without debug option - could this result in different code? Probably yes. In this way, using assert(...) macros in any inline/template code might thus be undefined behavior as well.

The only valid option I see is: Do not change your compiler environment during compilation of different modules.

Hmmm yes, that's right. The module specific flags won't work either. Damn.

Fortunately, the problem is only for these three variables I listed above. How about unconditionally providing Dune::identity and Dune::is_detected? That free us from wrong preprocessor branching. I introduced the DUNE_HAVE_CXX_UNEVALUATED_CONTEXT_LAMBDA for the grid concepts, we would need to be re-discover it on each module, but I am pretty sure we can find an alternative solution for that specific one (e.g., we could hand-write this in the header file since we know which compilers work).

Then some functions are compiled (and maybe put into a library) with and in the next module without debug option - could this result in different code? Probably yes. [...] The only valid option I see is: Do not change your compiler environment during compilation of different modules.

Sorry, I still do not buy this. Different compilers should work as long as we respect ABI boundaries. We do this essentially every day for the standard library when we use different compilers (think about std::string, std::lock, iostreams, etc.), yet it works. Why should this not work for us if DUNE ABI is bare to nothing?

I think, std::experimental::is_detected will never get into the standard library. For std::identity I have no idea. So, yes it would probably be fine to just use our implementation as we do for the last few years already.

mentioned in merge request !1261

mentioned in merge request !1262 (merged)

mentioned in merge request !1268 (closed)

added buildsystem label

changed milestone to %CMake Modernization

closed with merge request !1262 (merged)

mentioned in commit 45fa74f0

mentioned in issue #379 (closed)

handling of `config.h` (proposal for dev-meeting 22.1.2021)

Long term perspective to remove `config.h`

Discussion

Possible solutions

`HAVE_FOO` defines

Optional: rename `HAVE_FOO`

`ENABLE_FOO` magic

additional macros

Final proposal

Designs

Child items ...

Activity

handling of `config.h` (proposal for dev-meeting 22.1.2021)

Long term perspective to remove config.h

Discussion

Possible solutions

HAVE_FOO defines

Optional: rename HAVE_FOO

ENABLE_FOO magic

additional macros

Final proposal

Activity

Long term perspective to remove `config.h`

`HAVE_FOO` defines

Optional: rename `HAVE_FOO`

`ENABLE_FOO` magic