Draft: In add_test link against ${ProjectName}_LIBRARIES automatically

Summary

The macro add_test links automatically to dunecommon, but if used in downstream modules one has to add their own module library manually. This MR changes the default to the global property ${ProjectName}_LIBRARIES as suggested in !944 (comment 127787)

The behavior can be controlled by setting the following dune policy to OLD or NEW:

• policy DP0001: With OLD behavior, the ${ProjectName}_LIBRARIES are not linked automatically, but instead just the Dune::Common library. With NEW the introduced behavior is activated. The NEW behavior will be the default starting from dune-common version 2.11

Requirements

• Needs !1269 (merged)

added 1 commit

• b688fd76 - We need to link against Dune::Common as well

Compare with previous version

Since some modules do not provide a project library, we have to link against Dune::Common as a fallback as well.

Actually, I think that every exported module should have a module library - it can be an interface library if no actual library/object is created. This library would then communicate its dependencies.

Yes. Indeed, adding Dune::Common implies that dune-common is an interface requirement of the module library. That may not be the case because you can have libraries that consume dune-common internally but only expose their own interface independent of dune-common (e.g., Pimpl).

What we can do, is to check at dune_add_test if ${ProjectName}_LIBRARIES is empty, if so, warn the user that she needs at least dune library to link against. There are exceptional cases where a CMake project may not create a library and that's fine (e.g. application code very very simple test cases). For those cases case, we can add a flag that silences the warning.

Currently, dune-common is the place for systemwide requirements, like minimal c++ standard. Whenever you include something that uses dune-common includes, you need to provide the dune-common include-directories, communicated via the dune-common target. I think, if a project does not export any dune interface anymore, it is not a dune library and thus should not rely on our dune_add_test macros.

I think, if a project does not export any dune interface anymore, it is not a dune library and thus should not rely on our dune_add_test macros.

I don't see it that way. dune_add_test is just a build system facility that has nothing to do with the dune-common C++ requirements. If a user decide to use the Pimpl idiom to strip all dune C++ header requirements and still use the dune build system (i.e. dune_add_library and dune_add_test), I don't see why she has to be forced into a dune-common c++ target for testing.

I am just saying - it does not yet work that way, since we do not have module libraries that transport their dependencies. For example, dune-istl does not introduce a target and thus does not know its dependencies (at least not in the ${ProjectName}_LIBRARIES. We could easily do this for dune-istl and then we are fine here, but "almost all" dune modules do not have a module library. The main reason is: it is not introduced in the duneproject script and people who are not familiar with the dune buildsystem do not know how to do it.

We could link against ${DUNE_LIBS} ${${ProjectName}_LIBRARIES}

For dune-istl the variable DUNE_LIBS=Dune::Common, so this could work

It would be nice if there would be a variable containing both, the dependencies and the own module libraries. Also, I don't like the ${${ProjectName}_LIBRARIES} syntax. Why is there no ${ProjectLibraries} or ${PROJECT_LIBRARIES} variable? It could be defined for the current parsed cmake project.

Perhaps this is like that because technically there may be more than one dune project per project? See !1264 (merged). They are global properties, exactly what we want to move away from, so I think that they are technical details that should not be exposed outside of dune cmake functions. An idea would be to have an internal target specifically designed for tests that contain those libraries.

I have added a flag to dune_add_test that allows to disable automatic linking against module libraries or dune-common: NO_LINK_PROJECT_LIBRARIES. This flag can be set if you are in the special situation that you want to use the dune buildsystem but not link against any dune library

mentioned in merge request !1247 (merged)

added buildsystem label

added 1 commit

• 335a546d - Extract a global property for Project_INTERFACE_LIBRARIES

Compare with previous version

What is the right global property to use here? ${ProjectName}_LIBRARIES (e.g. dunecommon) or ${ProjectName}_INTERFACE_LIBRARIES (e.g. Dune::Common)

The first one is for internal purposes of the project, the second one for external consumers. Both may be completely disjoint of each other. Since tests aim to check if internals of the library work properly, I think it should be ${ProjectName}_LIBRARIES.

For reference, see discussion #344 (closed)

As in #344 (comment 129306) explained. I am not sure anymore what to link automatically here. If both lists might be completely disjoint, shouldn't we link both of them, then? Or just the exported libraries and manually the internal libraries? We need an example, a usecase, the most general meaningful situation to have a base for a discussion. In the core modules there is only dune-grid that has an extra libraries - albertagrid,

I was thinking the same with #344 (closed). Maybe is not a good idea to link always automatically. Also, the same problem arises if you want to have the testing directory as (perhaps) its own cmake project for build system testing purposes, then, the semantics of dune_add_test would change depending on what is automatically linked against.

But we should also think about the common case. Linking the "project-libraries", which are typically tested in a dune project, automatically makes writing tests much easier. And it reflects the current behavior. We actually link all dune libraries implicitly by the add_dune_all_flags command that is called with the test. When I remove this in !1207 (merged) we do not even have dune-common as a dependency and thus all current tests would fail to compile. Maybe another application of the dune_policy !1269 (merged) approach :-)

Right. An easy fix for that common case could be a target exclusively used for testing within the project. If you link anything to it, it will be always be used in the tests.

I don't understand your "easy fix". What do you mean by "a target exclusively used for testing"? We don't have such a target. dune-common is also not enough if we want to remove all explicit LINK_LIBRARIES. But maybe we don't want this (?). The dune_add_test utility is a convenient utility that tests dune modules. So, we should link the libraries of these dune modules that are tested. If you need another tool that explicitly does not link these libraries, maybe we simply split up the utility and have one command dune_add_pure_test that is used in dune_add_test but does not link against anything

I had in mind something along these lines:

# interface library linked to all tests created with dune_add_test
add_library(dune-testing-shared INTERFACE)

function(dune_add_test)
   # ...
   # add testing interface to the executable
   target_link_libraries(${ADDTEST_NAME} PRIVATE dune-testing-shared)
   # ...
endfunction()

Now, you can customize whatever you want to be passed to your all tests through dune-testing-shared, rather than linking to everything in dune. For example:

# we always need dune-common for testting
target_link_libraries(dune-testing-shared PUBLIC dune-common)
# we want to have all the warnings enabled for testting
target_compile_options(dune-testing-shared INTERFACE
  $<$<CXX_COMPILER_ID:MSVC>:/W4 /WX>
  $<$<NOT:$<CXX_COMPILER_ID:MSVC>>:-Wall -Wextra -Wpedantic -Werror>
)

# make actual test targets
dune_add_test(...)

Looks simple and flexible to me and how I usually use the testing facilities, but perhaps it's also an over-complication for the core modules

Essentially, then we are back to manually specifying all the dune dependencies in the tests. We can do this already now using the LINK_LIBRARIES flag + the NO_LINK_PROJECT_LIBRARIES would do exactly what you proposed. The point is that we would have to update all downstream modules to define a dune-testing-shared target if we would make this a requirement.

Example of doing this today (with this MR):

target_link_libraries(dune-testing-shared PUBLIC Dune::Common)
target_compile_options(dune-testing-shared INTERFACE ...)

dune_add_test(test-target LINK_LIBRARIES dune-testing-shared NO_LINK_PROJECT_LIBRARIES)

changed the description

added 2 commits

• 0a978003 - Link against Project_INTERFACE_LIBRARIES or Dune::Common
• 7d9120d9 - Remove dunecommon from tests

Compare with previous version

mentioned in merge request !1207 (merged)

added Testing label

mentioned in issue #344 (closed)

added 80 commits

• 7d9120d9...76784f35 - 74 commits from branch master
• 59452ded - In add_test link against ${ProjectName}_LIBRARIES automatically
• b1cb0acb - We need to link against Dune::Common as well
• 7fceec8f - Extract a global property for Project_INTERFACE_LIBRARIES
• 891548c0 - Link against Project_INTERFACE_LIBRARIES or Dune::Common
• afdef3a9 - Remove dunecommon from tests
• 36903b84 - Add documentation

Compare with previous version

I propose to go with this MR. It simplifies writing tests in standard dune modules (those that test the module and thus need to link against the module library or at least against Dune::Common).

Currently we link explicitly only against Dune::Common. This is not enough in downstream modules. There we want to test a module library and thus need to link against the module library. Currently we have to do this manually using LINK_LIBRARIES Dune::<Module>. If a module does not provide a library (and also no interface library), e.g. a header-only module, the global property ${ProjectName}_INTERFACE_LIBRARIES is empty. In this case, the MR proposes to link against Dune::Common explicitly.

You can always opt-out if you do not want any automatic linking. Then simply add NO_LINK_PROJECT_LIBRARIES as flag. If you want to have the behavior for all your tests, simply create a macro

macro(my_dune_add_test)
  dune_add_test(${ARGV} NO_LINK_PROJECT_LIBRARIES)
endmacro()

or overwrite the old macro.

If you want to have finer control over the behavior and a maybe want to choose whether to have to old or the new linking strategy, we could introduce policy system first, see !1269 (merged).

Ok, I will merge the policy branch first. Then it is easier to adapt the behavior.

marked this merge request as draft

changed the description

added 15 commits

• 36903b84...727570d6 - 7 commits from branch master
• 33ce1def - In add_test link against ${ProjectName}_LIBRARIES automatically
• 0a156097 - We need to link against Dune::Common as well
• d3379399 - Extract a global property for Project_INTERFACE_LIBRARIES
• 32f433ec - Link against Project_INTERFACE_LIBRARIES or Dune::Common
• 492fabfe - Remove dunecommon from tests
• dd747612 - Add documentation
• 566e9a30 - Add a policy for linking Project libraries automatically in tests
• 06aca87c - Add a documentation for the new policy DP0001

Compare with previous version

Isn't this superseded by !1481 (merged)?

Yes, I think this MR can be closed.

Closed, because we have chose to follow a different solution with !1481 (merged).

closed