Why bcm_test_link_libraries?
ldionne opened this issue · 26 comments
It seems like a bad idea to allow setting anything global. I thought modern CMake was all about explicitly stating what your dependencies are when creating a target. Unless there's a good reason to have this, I would rather not have it at all, because I think I would always consider this an anti-pattern.
Yea, this should probably be a directory scope property instead, so it avoids the problems of a global setting and will work with sub directory/superproject builds.
I personally don't see a need for anything in BCM regarding this. Instead, I would replace the NO_TEST_LIBS option of bcm_test by something like LINK_LIBRARIES that takes a list of libraries to link with that test. I would then use it as
foreach (f IN LISTS UNIT_TEST_FILES)
bcm_test(test.${f} LINK_LIBRARIES xyz)
endforeach()Not all tests can be easily listed in a foreach. Ideally, a function could be created:
function(bcm_create_mytest)
bcm_test(${ARGN} LINK_LIBRARIES xyz)
endfunction()However, I will need to write this function on every module, so instead I just integrated into bcm_test.
I agree that having it be global is bad, so I should move to make it more localized to the directory. What problems do you see with providing directory scope test dependencies?
What problems do you see with providing directory scope test dependencies?
I don't care so long as I don't have to use it. IOW, I'm good if bcm_test is specified such that it checks for a property like BCM_DEFAULT_TEST_LIBRARIES or similar, and uses that if it's set. This way, I'll just avoid using this feature and it won't be put forward as being the de-facto way of setting up tests.
I'll just avoid using this feature
Why? What problems do you see using this feature. Looking at Boost.Hana it does the same thing. So why would you avoid using it?
Alternatively, for each bcm_test* function I could provide a bcm_package_test* function that will link in the declared package(and then just remove bcm_link_libraries).
Looking at Boost.Hana it does the same thing.
No. I add the dependency on Hana explicitly in the boost_hana_set_test_properties macro.
No. I add the dependency on Hana explicitly in the boost_hana_set_test_properties macro.
Which is global and added to every test. Furthermore, the bjam file adds dependencies globally and implicitly including the private dependencies. My question is why you see a problem with a tool that adds the tests that accomplishes the same thing you are doing in Boost.Hana? What problems and concerns do you see with this?
The difference is that I add the dependency on the hana target explicitly by creating the boost_hana_set_test_properties macro, whereas this would be implicit using bcm_test.
Yes, but in the bjam file you add the dependencies implicitly.
The idea is that the bcm_test is implicitly tied to the packaged unless you explicitly opt out. This is better than the bjam file(or even using the "global" cmake settings such as include_directories or link_libraries) as you can opt out for tests that don't need those dependencies, whereas as using a "global" setting this is not possible.
What concerns or problems do you see with that?
Yes, but in the bjam file you add the dependencies implicitly.
Whatever, I don't care about the bjam files and I don't see why they are relevant at all. I just wrote them to satisfy the boost library requirements that we're able to test Hana using Boost.Build, but they're not an example of how to use Boost.Build properly. We're talking about CMake here.
My position is simple: instead of adding the library to the tests implicitly, I would suggest not doing that because I claim that propagating any form of dependency implicitly in bcm_test is wrong. Instead, I expect that most users will want to create their own macro to wrap bcm_test anyway for adding stuff like warning flags, and that is the right place to explicitly make the unit test dependent on the library.
Closing this, as we're not making progress.
Another way to put it is that the bcm_test function links against the BCM testing infrastructure unless the test opts out. I designed it this way since most libraries automatically link against or create a testing infrastructure(like boost_hana_set_test_properties or test_build_and_run). Instead of a library creating their own testing infrastructure, the BCM provides a testing infrastructure to use.
Do you also provide things like warning flags and disabling the use of CXX_EXTENSIONS?
Do you also provide things like warning flags and disabling the use of CXX_EXTENSIONS?
I am planning to add properties for warnings which can be applied at any scope, but I would like to add target version of the properties as well(including for the current properties such as CXX_EXCEPTIONS etc), so then the user sets up the testing infrastructure using bcm_test_link_libraries, something like:
bcm_test_link_libraries(bcm::enable_all_warnings bcm::enable_warnings_as_errors)Or they could create there own target for other settings:
add_library(test_flags INTERFACE)
bcm_test_link_libraries(test_flags)
target_compile_options(test_flags INTERFACE -Weverything -Werror)I would like to avoid having the user create their own add_test function if possible.
Do you also provide things like warning flags and disabling the use of CXX_EXTENSIONS?
What do you mean disabling the use of CXX_EXTENSIONS? Ideally, the CXX_EXTENSIONS should be provided by the toolchain, library authors shouldn't be setting this.
I mean this: https://github.com/boostorg/hana/blob/master/CMakeLists.txt#L71. It controls whether -std=c++14 or -std=gnu++14 is used.
I mean this: https://github.com/boostorg/hana/blob/master/CMakeLists.txt#L71.
Yes that should be set by the toolchain, not by the libraries. And definitely should not be set by the tests as there is no way to test a library with the compiler extensions enabled.
When you say it should be set by the toolchain, do you mean passing it as cmake .. -DCMAKE_CXX_EXTENSIONS=OFF?
Either way, BCM should provide target version for the properties(maybe even for CXX_STANDARD and CXX_EXTENSIONS), as that is a cleaner way of setting compiler flags.
When you say it should be set by the toolchain, do you mean passing it as cmake .. -DCMAKE_CXX_EXTENSIONS=OFF?
Either that or setting it in a cmake toolchain file: cmake -DCMAKE_TOOLCHAIN_FILE=cpp14.cmake ...
Either that or setting it in a cmake toolchain file:
cmake -DCMAKE_TOOLCHAIN_FILE=cpp14.cmake ...
That sounds like a ton of work for something simple, but ok.
That sounds like a ton of work for something simple, but ok.
But the settings can easily be reused across projects. Of course, the simplest and most straightforward way is to just to do CXXFLAGS=-std=c++14 cmake ..
CXXFLAGS=-std=c++14 cmake ..
Uh? I use target_compile_features(library INTERFACE cxx_std_14) and that does it, the only problem is that it then defaults to -std=gnu++14: https://github.com/ldionne/dyno/blob/master/CMakeLists.txt#L12
If you use CXXFLAGS=-std=c++14 cmake .., won't that clash with -std=gnu++14 provided by CMake and whatever appears last on the command line will most likely be kept?
Uh? I use target_compile_features(library INTERFACE cxx_std_14)
The point is not to do this, but rather the user/toolchain/compiler sets this.
If you use CXXFLAGS=-std=c++14 cmake .., won't that clash with -std=gnu++14 provided by CMake and whatever appears last on the command line will most likely be kept?
Yes, exactly, this is why the library should not set this, especially in the interface. Because if the user wants to use c++17 with CXXFLAGS=-std=c++17 cmake ..(or are using a build of clang that defaults to using c++17) in their own project, however, when they link against hana, they will be restricted to c++14.
Instead, a library should just check if the compiler is capable enough during configuration. Boost.Config could help provide this, as it does currently for bjam. The cmake config packages will make this much cleaner.
The point is not to do this, but rather the user/toolchain/compiler sets this.
Uh? The fact that a library requires some version of the standard is clearly a usage requirement, and I don't think it makes any sense to specify that using a toolchain file.
however, when they link against hana, they will be restricted to c++14
What? If another library as cxx_std_17 as a requirement and it links against Hana, I assume the resulting compiler flag will be -std=c++17, no? IOW, cxx_std_14 as a requirement states that C++14 or later is required, not C++14 exactly. Anything else would be a bug in CMake IMO.
What? If another library as cxx_std_17 as a requirement and it links against Hana, I assume the resulting compiler flag will be -std=c++17, no?
If they are using a version of cmake that doesn't have cxx_std_17 or a custom standard version is used(such as -std=c++amp), then the user will add the flags as compile options, which will clash with the usage requirements. Ideally cmake should provide a way for toolchains to define custom compile features and define how they should be resolved.
Regardless, cxx_std_11 cannot be used anyways as it does not work in cmake 3.5.1, which is the minimum version that is supported.