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

Trim unnecessary config.h defines from prerequisite list #319

Merged
merged 3 commits into from
Aug 17, 2013

Conversation

rolk
Copy link
Member

@rolk rolk commented Aug 14, 2013

The lists in opm-*-prereqs.cmake contained a lot of variables that were not in any header file in the modules. Granted, many of these variables are pulled in by the DUNE-modules anyway, but that doesn't mean they belong in the prerequisite list for those modules anyway (the lists aren't transitive -- each module should only declare what itself need).

This changeset prunes the list down to what is actually needed. Note that variables needed only by opm/.c files should rather go in CMakeLists.txt, so they are not propagated to other libraries.

This define is used by the unit tests; there is no reason why it should
be on the list that must be provided by other projects that use our
headers (the other projects don't use our tests).
The modules declare which configuration variables they need to have
present in config.h, not only their own but also defined in projects
using them.

However, a lot of these variables are not actually used in the headers!

This changeset removes all HAVE_XXX variables which is not present in
any opm/*.h* file in these projects, and thus there is no need for the
client to specify.

Note that only the variables used by the module *itself* should be
listed; the build system will setup the complete list from the
prerequisites.
This define is used in a opm/*.c* file, but only there, and should
therefore be in config.h, but not added to the list that only clients
must also provide.
@andlaus
Copy link
Contributor

andlaus commented Aug 14, 2013

HAVE_MPI is not required by opm-material since it is already provided by dune-common. The rest of the opm-material stuff seems okay to me. (some of the config.h its macros there are actually used by dune-common, but I believe dune-common-prereqs.cmake features them now .)

@rolk
Copy link
Member Author

rolk commented Aug 14, 2013

HAVE_MPI is not required by opm-material since it is already provided by dune-common

Yes, but you shouldn't depend on that, just declare what your own .h* files need, and the find macros will setup the complete list.

@andlaus
Copy link
Contributor

andlaus commented Aug 14, 2013

HAVE_MPI is not required by opm-material since it is already provided by dune-common

Yes, but you shouldn't depend on that, just declare what your own .h* files need, and the find macros will setup the complete list.

okay. I'm just wondering whether these variables should also be set by some Find* calls in this case. IMHO there's not much point in setting a config.h variable to an incorrect value...

@andlaus
Copy link
Contributor

andlaus commented Aug 14, 2013

I just noticed that the REQUIRED arguments are missing for dune-common and dune-istl in the opm-material prerequisites. Would you mind adding them in this PR or should I open a new one?

@rolk
Copy link
Member Author

rolk commented Aug 16, 2013

Would you mind adding them in this PR

Topic-wise this is better handled in #322 (where they are included), I think.

@bska
Copy link
Member

bska commented Aug 17, 2013

As all concerns have apparently been addressed, either here or in a follow-up PR (#322), and I've found no trouble in local testing, I'll merge this into master. Thanks a lot.

bska added a commit that referenced this pull request Aug 17, 2013
Trim unnecessary config.h defines from prerequisite list
@bska bska merged commit ed3552c into OPM:master Aug 17, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants