Remove add_dune_all_flags from dune_add_test

Summary

In the current master, the dune_add_test macro automatically adds all registered package flags to each test target. This is almost never necessary. Most of the time we want to test a specific feature. If that feature depends on an optional package, it makes more sense to make that explicit, so that the test itself documents what it needs. This MR removes the add_dune_all_flags call from the dune_add_test macro and adds the flags manually. This new behavior is enabled only if the Dune policy DP_TEST_ADD_ALL_FLAGS is set to NEW.

Details

The idea is to follow the same strategy for tests as for executables. You have to choose what you need and link it. While every required dependency is automatically linked, optional dependencies must be spelled out. If tests do not do this, it is not clear what they actually depend on. It cannot be inspected by looking at the added flags, since silently all found packages and all somewhere registered flags, whether they are relevant to this test or not, are added to all test targets. This could also lead in the future to clashes in the compile flags (and has already done in the past), e.g. if two external optional packages set the same flag, but these external packages are defined in different modules. If we cannot choose which of these packages we actually want, then we are stuck and have to go back to low-level add_test of cmake.

Another aspect: tests have (at least) two meanings, 1. they check that functions and classes are implemented correctly, 2. they show the user how to use these functions and classes. Thus it is also a documentation how to use dune. But if the dune_add_test macro sets silently flags that are needed for a specific tests and thus for a specific feature of the library but without documentation to the user, the second goal of tests is failed. With this MR, we go a little bit into a more expressive direction.

How to write/configure tests

If you write a test of an (optional) feature A, you already have to either check for existence of the feature by HAVE_A in the source file, or explicitly require it by CMAKE_GUARD HAVE_A in the dune_add_test. It is thus just one more step to say: If I require that feature and I check in my source that the feature exists, then I also need to activate that feature, otherwise it cannot be available. This is done by dune_add_A_flags(target).

Example

Let's write a test testA that requires an optional feature A and may additionally use a feature B if it is available, but works perfectly fine without it. Then we write in the corresponding CMAkeLists.txt file

dune_add_test(NAME testA SOURCES testA.cc 
              CMAKE_GUARD HAVE_A)
add_dune_A_flags(testA)
add_dune_B_flags(testA) # not required but can be used if found

and in the source file

#include <config.h>

#if !HAVE_A
#error "Require feature A"
#endif

int main() {
#if HAVE_B
  test_with_feature_B();
#else 
  test_with_feature_A();
#endif
}

New Dune policies

The behavior can be controlled by the Dune policy DP_TEST_ADD_ALL_FLAGS:

• OLD: Call add_dune_all_flags on all test targets automatically.
• NEW: Do not call add_dune_all_flags automatically. Must be called manually for each test target.

TODO

• Check downstream modules where tests fail due to missing optional packages
• Add changelog entry

added Testing buildsystem labels

added 1 commit

• 1eff8ec1 - Add flags for quadmathtest

Compare with previous version

added 1 commit

• bd8329fc - Link against DUNE_LIBS in tests by default

Compare with previous version

mentioned in merge request dune-istl!509 (merged)

mentioned in merge request dune-grid!685

added 8 commits

• bd8329fc...058f0c44 - 4 commits from branch master
• 833026a7 - Remove add_dune_all_flags from add_test
• a6fe09a3 - Add flags and LINK_LIBRARIES to some tests
• c45deb36 - Add flags for quadmathtest
• df4c7ccd - Link against DUNE_LIBS in tests by default

Compare with previous version

marked the checklist item Check downstream modules where tests fail due to missing optional packages as completed

marked this merge request as ready

changed the description

Looks to me like this affects all downstream modules. As we don't have control over downstream modules this would need to allow for some deprecation period. E.g. introducing a (module-level) flag DUNE_ADD_TEST_ADD_DUNE_ALL_FLAGS which defaults to TRUE and emits a deprecation warning when it's TRUE.

mentioned in merge request !1268 (closed)

added 165 commits

• df4c7ccd...48a5251d - 161 commits from branch master
• c1663bc0 - Remove add_dune_all_flags from add_test
• cd212386 - Add flags and LINK_LIBRARIES to some tests
• 06598f39 - Add flags for quadmathtest
• 49a29775 - Link against DUNE_LIBS in tests by default

Compare with previous version

added 14 commits

• 49a29775...0a978003 - 4 earlier commits
• 7d9120d9 - Remove dunecommon from tests
• 4e4f72f7 - Fix typo in global property add_library
• 5b27ee74 - Extract the namespace from the EXPORT_NAME
• 5da5f93e - Reverse search for the export name in the full target name
• f91988f6 - Remove add_dune_all_flags from add_test
• 1e8bbf49 - Add flags and LINK_LIBRARIES to some tests
• 2c3b6c7c - Add flags for quadmathtest
• 5d646d0d - Link against DUNE_LIBS in tests by default
• e9efe60d - Rebase to add-test-link-project-libraries branch
• 1c293564 - Remove unnecessary dunecommon link library

Compare with previous version

changed the description

added 1 commit

• 952257cd - Add global flag to activate the old behavior

Compare with previous version

mentioned in merge request !1263 (closed)

added 87 commits

• 952257cd...76784f35 - 74 commits from branch master
• 76784f35...7725e378 - 3 earlier commits
• 1cb66a02 - Link against Project_INTERFACE_LIBRARIES or Dune::Common
• d4706adc - Remove dunecommon from tests
• 56b73801 - Remove add_dune_all_flags from add_test
• f9531fd6 - Add flags and LINK_LIBRARIES to some tests
• 906fcf19 - Add flags for quadmathtest
• 31dd6155 - Link against DUNE_LIBS in tests by default
• f6b319c9 - Rebase to add-test-link-project-libraries branch
• c719fe62 - Remove unnecessary dunecommon link library
• 3e885207 - Add global flag to activate the old behavior
• 042d14cd - Improve documentation and add flag NO_ADD_DUNE_ALL_FLAGS

Compare with previous version

marked this merge request as draft

changed milestone to %DUNE 2.10.0

Market for milestone 2.10. I hope we get this merged soon. Any help or decision needed?

mentioned in merge request !1391 (closed)

changed milestone to %DUNE 2.11.0

mentioned in issue #386

This probably needs to be updated to use the fact that we now link against a desired target directory-wise !1481 (merged) and we use policies to determine changes of behavior !1269 (merged).

added 414 commits

• 042d14cd...3ff22a8e - 412 commits from branch master
• 79ce41c1 - Do not add all flags in tests automatically
• f215c35e - Add a policy to deactivate add_all_flags

Compare with previous version

added 1 commit

• c143d2fe - Add a policy to deactivate add_all_flags

Compare with previous version

added 1 commit

• 12de4e59 - Add a policy to deactivate add_all_flags

Compare with previous version

changed the description

added 2 commits

• 978daa48 - Improve the documentation
• f6f76cce - Add a changelog entry

Compare with previous version

marked this merge request as ready

added 3 commits

• 4698a045 - Add a policy to deactivate add_all_flags
• d48f02d1 - Improve the documentation
• a73d24fe - Add a changelog entry

Compare with previous version

marked the checklist item Add changelog entry as completed

resolved all threads

added 2 commits

• a9cb8e74 - Add a policy to deactivate add_all_flags
• 3eaacb22 - Add a changelog entry

Compare with previous version

requested review from @santiago.ospina

assigned to @simon.praetorius

added 1 commit

• efffb029 - Remove the NO_ADD_ALL_FLAGS argument to dune_add_test

Compare with previous version

resolved all threads

approved this merge request

changed the description

enabled an automatic merge when all merge checks for efffb029 pass

merged

mentioned in commit fb867246