boost-cmake / boost

Cmake-based build of boost
Boost Software License 1.0
58 stars 24 forks source link

Some missing Boost Build constructs in generated tests for CMake for 1.64 branch #8

Open eldiener opened 6 years ago

eldiener commented 6 years ago

In looking at the Boost Build jamfile for tests in preprocessor, tti, and vmd I noticed a number of Boost Build constructs that are left out of the conversion to CMake, but which should have CMake equivalents. I do not know if you are generating the CMakeLIsts.txt files manually for each library or whether you have a script for doing so, but if it is the latter perhaps these conversion facilities from Boost Build to CMake can be added. Or perhaps you can offer some bcm equivalents for some of these things to ease the manual conversion which individual library developers will have to do in generating the correct CMake test scripts from their Boost Build equivalents.

  1. Boost Build has the notion of a 'project' rule. The main use for the 'project' rule is to be able to specifying build requirements to each rule in the jamfile. For testing purposed the use of 'project' rule requirements is largely so that the same requirement for each individual test does not have to be repeated again and again. I believe the equivalent to this in CMake is the ability to set build properties for the entire CMakeLists.txt rather than to set target properties. I know the official Daniel Pfeiffer document frowns on setting global properties as opposed to specific target properties, but this is nonsense as far as I am concerned. If you have a group of tests, why should you have to repeat the same build requirement for each and every test repeatedly when you can set the build requirement once for all the tests. In your conversion from Boost Build to CMakeLists.txt you ignored the 'project' rule entirely.

  2. Boost Build has the notion of conditional requirements using the property:property syntax. This is the equivalent of testing for some value in CMake with an 'if(something)', and then setting some CMake value based on that test. I noticed that you ignored conditional requirements completely in your converted CMakeLists.txt files for library tests.

3) Boost Build has the ability to specify particular compiler and compiler/version pairs directly using the notation 'compiler_id-optional_version_string'. This is the equivalent of CMake variables CMAKE_CXX_COMPILER_ID and CMAKE_CXX_COMPILER_VERSION, but with different compiler ids and with slightly different version string notations. Often in Boost Build the notation is used with conditional requirements to set a property based on the compiler or compiler/version being used in the compile. This is very useful for test when it is known that some compiler will fail a test unless some compiler option is set correctly.

I have brought up these three issues simply to explain why your CMakeLists.txt files for library tests might fail. You have done a great job with bcm and your work of creating CMake files for Boost using bcm, and should be commended. My major concern is that the more library developers ( like me ), who are not CMake experts like you, have to do manually to use CMake for their libraries instead of Boost Build, the less likely they are to want to do anything. Subsequently their tests using CMake might fail using particular toolsets even when their tests using Boost Build will succeed.

Thanks for your efforts !

pfultz2 commented 6 years ago

I do not know if you are generating the CMakeLIsts.txt files manually for each library or whether you have a script for doing so

Part of it is generated, and part of it is written out. Each test case has been manually written out.

Boost Build has the notion of a 'project' rule. The main use for the 'project' rule is to be able to specifying build requirements to each rule in the jamfile. For testing purposed the use of 'project' rule requirements is largely so that the same requirement for each individual test does not have to be repeated again and again.

Yes, and you can use bcm_test_link_libraries for that, which is just links a library but you can use an interface library to build a library that is just a collection of compile flags. For example, the date_time tests do:

add_library(data_time_tests_properties INTERFACE)
target_compile_definitions(data_time_tests_properties INTERFACE BOOST_DATE_TIME_POSIX_TIME_STD_CONFIG)
bcm_test_link_libraries(data_time_tests_properties)

So every test created with bcm_test will include the BOOST_DATE_TIME_POSIX_TIME_STD_CONFIG definition. Although, some cases the flags are added to the original library rather than the tests(because currently boost doesn't provide usage requirements for header-only libraries).

I noticed that you ignored conditional requirements completely in your converted CMakeLists.txt files for library tests.

Yes, those need work, as the conditions aren't always equivalent, so I need to figure out what the Boost.Build version does and convert it to a cmake version.

Often in Boost Build the notation is used with conditional requirements to set a property based on the compiler or compiler/version being used in the compile.

Yes, but some of those things should not happen. For example, tests should not be adding -std=c++11 flags, as then its impossible to run the tests on newer c++ versions. What needs to happen is make sure a certain level of C++ support is available to run the tests. Boost.MP11 is a good example of how to do it, but it needs Boost.Config to check language support. This is something on my todo to finish.

Also, some of the other flags that the tests add are actually usage requirements and so they should be added to the library not the tests.

My major concern is that the more library developers ( like me ), who are not CMake experts like you, have to do manually to use CMake for their libraries instead of Boost Build, the less likely they are to want to do anything.

But I also am not a Boost.Build expert, so I dont understand all the subtle settings that are going on. I am trying to get as much done as possible, and hopefully the authors can fill in the details.

eldiener commented 6 years ago

Yes, and you can use bcm_test_link_libraries for that, which is just links a library but you can use an interface library to build a library that is just a collection of compile flags.

This does not work for tests. The tests are not libraries, but bcm_tests. There is no library involved. My point is that in Boost Build the project requirements are default requirements for each test, but there is no equivalent to this except for CMake settings which are set for the entire CMakeLists.txt file as opposed to CMake target settings which are set for each test as a target. To put it another way the requirements at the 'project' level in Boost Build apply to all virtual targets in the Boost Build jamfile itself.

Yes, but some of those things should not happen. For example, tests should not be adding -std=c++11 flags, as then its impossible to run the tests on newer c++ versions.

This is wrong. If I as the library developer know that a particular test needs -std=c++11 in order to have any chance to succeed, so I add it to that test, that cannot be incorrect. Forget about the particular requirement of -std=c++11 for example, and think in terms of any compiler flag, and you will see that the library developer has the perfect right to add some flag without which the test must fail. So please do not foolishly claim that "those things should not happen". You cannot possible know, in a point blank way, what compiler setting is necessary in order to even have a chance to run a test successfully for a particular Boost library.

pfultz2 commented 6 years ago

This does not work for tests.

How so?

The tests are not libraries, but bcm_tests. There is no library involved.

A bcm_test is both a cmake test and a target(which can be either an executable or library). The target will link against any libraries that were specified in the bcm_test_link_libraries unless NO_TEST_LIBS is used.

My point is that in Boost Build the project requirements are default requirements for each test, but there is no equivalent to this except for CMake settings which are set for the entire CMakeLists.txt file as opposed to CMake target settings which are set for each test as a target.

No there isn't unless you are using bcm_test infrastructure which does what you describe.

If I as the library developer know that a particular test needs -std=c++11 in order to have any chance to succeed, so I add it to that test, that cannot be incorrect.

It is incorrect, because now there is no way to test your library with a C++14 or C++17 toolchain. Rather, you should just disable the tests when the compiler is not C++11 capable. This is the way mp11 tests work here, but relies on the feature-checking from Boost.Config, which has not been implemented yet.

Alternatively, there may be a way in a future to specify the C++ version as a usage requirement, and there can be some resolution when different version of C++ are specified. Cmake supports that with CXX_STANDARD but it has its limitations, and is not supported in 3.5.

Forget about the particular requirement of -std=c++11 for example, and think in terms of any compiler flag, and you will see that the library developer has the perfect right to add some flag without which the test must fail.

Yes, but if that flag is applied to all tests, then it should most likely be a usage requirement of the library, because the library is never tested to work without that flag. There are of course exceptions.

eldiener commented 6 years ago

A bcm_test is both a cmake test and a target(which can be either an executable or library). The target will link against any libraries that were specified in the bcm_test_link_libraries unless NO_TEST_LIBS is used.

My entire point is that I do not want to have to repeat the exact same requirement for every bloody test. In Boost Build I do not have to do this. But you keep insisting that using bcm_test and doing this for every single test is a good way to use CMake. If it is the only way to use CMake I have no argument with that. But as I understand it I can set usage requirements in CMake for the entire CMakeLists.txt file rather than each target in the file. If I am wrong about this please point this out to me.

It is incorrect, because now there is no way to test your library with a C++14 or C++17 toolchain. Rather, you should just disable the tests when the compiler is not C++11 capable. This is the way mp11 tests work here, but relies on the feature-checking from Boost.Config, which has not been implemented yet.

You just do not know what you are talking about. First, creating some tests which need some level of c++ compliance and then having the tests fail because the end-user runs those particular tests without that level of compliance, is just going to confuse the end-user. What is the end-user supposed to do. Run test X with C++11 compliance, then run test Y with C++14, and then run test Z with C++03 compliance, and on and on with this sort of ludicrousness just to run tests successfully ? That is totally absurd. Testing should be made as easy as possible for end-users and test scenarios like the Boost regression tests, not as arcane and confusing as it can be.

It is also possible to have the same tests run more than once with different levels of c++ compliance for a library that can run successfully with difference compliance levels. So naturally I run the same test specifying different levels of compliance in order to test the functionality. Please don't natter on saying that this is incorrect or unacceptable to your way of thinking. This is a reality, and no matter what you feel about it you are no judge of how a library developer wants to test his library.

Yes, but if that flag is applied to all tests, then it should most likely be a usage requirement of the library, because the library is never tested to work without that flag. There are of course exceptions.

What library are you talking about ? Do you mean the test executables. That is fine but manually having to add the exact same flag to each test is a PITA. That is why having Boost Build 'project' requirements, even Boost Build 'project' conditional requirements, translated to CMake build requirements for the entire CMakeLists.txt file would be so much easier. If that sort of CMake functionality does not exist, for whatever arcane reason, I can accept that. But if it does exist, and you are arguing for repeating the same requirement for every single test through bcm_test I am afraid I can not understand the reason for such unnecessary repetition.

pfultz2 commented 6 years ago

My entire point is that I do not want to have to repeat the exact same requirement for every bloody test.

Yes, thats why you use bcm_test_link_libraries so you don't have to repeat it on every test.

But you keep insisting that using bcm_test and doing this for every single test is a good way to use CMake.

I never said "doing this for every single test is a good way to use CMake".

But as I understand it I can set usage requirements in CMake for the entire CMakeLists.txt file rather than each target in the file.

No you can't. You can set some properties at directory or global scope, which can affect how targets are build, but its mainly used for defining toolchains because it doesn't affect usage requirements. Its better to just use bcm_test_link_libraries to set usage requirements for all tests in a directory as this only affects tests and it can be opt-out if necessary for certain test cases.

You just do not know what you are talking about.

I really don't appreciate the tone.

First, creating some tests which need some level of c++ compliance and then having the tests fail because the end-user runs those particular tests without that level of compliance, is just going to confuse the end-user.

I totally agree, which why the tests should be disabled using feature-checking.

Run test X with C++11 compliance, then run test Y with C++14, and then run test Z with C++03 compliance, and on and on with this sort of ludicrousness just to run tests successfully ? That is totally absurd. Testing should be made as easy as possible for end-users and test scenarios like the Boost regression tests, not as arcane and confusing as it can be.

I dont follow this at all. My point is that the tests should test against the user's toolchain not some other toolchain. That is, the user sets up a C++14 toolchain and having the tests run against a C++11 toolchain is simply wrong(which is what can happen when adding -std=c++11 to every test).

It is also possible to have the same tests run more than once with different levels of c++ compliance for a library that can run successfully with difference compliance levels.

But then this wouldn't be added to the project requirements.

What library are you talking about ?

The library you are testing with. For example, the tests for Boost.DLL adds -ldl for linux. This is added as project requirements for the tests Jamfile, but this is actually usage requirements for using Boost.DLL. So in the cmake here, its added as usage requirements so all downstream consumers of Boost.DLL will properly use -ldl, including the tests.

Put another way, if library A is only tested using -DFOO=1(which is what can happen by adding FOO=1 as a project requirement), then how can we know that the library works without FOO=1? We really don't, so the usage requirements for library A should have -DFOO=1 so all downstream users are using the tested configuration.

dutow commented 6 years ago

I dont follow this at all. My point is that the tests should test against the user's toolchain not some other toolchain. That is, the user sets up a C++14 toolchain and having the tests run against a C++11 toolchain is simply wrong(which is what can happen when adding -std=c++11 to every test).

I think you are describing two different scenarios here:

eldiener commented 6 years ago

I apologize getting so heated up over a general argument over whether the library developer should be able to set the C++ standard level of compliance for particular tests. The argument was not even relevant to the points I was making in my original post, which made the heated discussion of it on my part even worse.

I did not realize what you were referring to with bcm_test_link_libraries but now I do. I guess the name of 'bcm_test_link_libraries' confused me as I did not realize this referred to any library, whether it is header-only or linked ( built library ).

eldiener commented 6 years ago

Looking at CMake it appears there are properties that can be set on source files using set_source_file_properties. Among these source file properties which can be set are COMPILE_DEFINITIONS and COMPILE_FLAGS. If these properties filter down to tests specified in that source file I believe they would be a better match for the requirements of a Boost Build 'project' rule specified in a bjam file for tests than attaching such values to the library itself using bcm_test_link_libraries.

pfultz2 commented 6 years ago

I am not sure how that helps. For example, in date_time the definition of BOOST_DATE_TIME_POSIX_TIME_STD_CONFIG is added to every test using bcm_test_link_libraries like this:

add_library(data_time_tests_properties INTERFACE)
target_compile_definitions(data_time_tests_properties INTERFACE BOOST_DATE_TIME_POSIX_TIME_STD_CONFIG)
bcm_test_link_libraries(data_time_tests_properties)

bcm_test(NAME testint_adapter SOURCES testint_adapter.cpp)
bcm_test(NAME testtime_resolution_traits SOURCES testtime_resolution_traits.cpp)
bcm_test(NAME testwrapping_int SOURCES testwrapping_int.cpp)
bcm_test(NAME testconstrained_value SOURCES testconstrained_value.cpp)
bcm_test(NAME testgregorian_calendar SOURCES testgregorian_calendar.cpp)
bcm_test(NAME testgeneric_period SOURCES testgeneric_period.cpp)

If you use set_source_file_properties you would need to do:

bcm_test(NAME testint_adapter SOURCES testint_adapter.cpp)
set_source_files_properties(testint_adapter.cpp PROPERTIES BOOST_DATE_TIME_POSIX_TIME_STD_CONFIG)
bcm_test(NAME testtime_resolution_traits SOURCES testtime_resolution_traits.cpp)
set_source_files_properties(testtime_resolution_traits.cpp PROPERTIES BOOST_DATE_TIME_POSIX_TIME_STD_CONFIG)
bcm_test(NAME testwrapping_int SOURCES testwrapping_int.cpp)
set_source_files_properties(testwrapping_int.cpp PROPERTIES BOOST_DATE_TIME_POSIX_TIME_STD_CONFIG)
bcm_test(NAME testconstrained_value SOURCES testconstrained_value.cpp)
set_source_files_properties(testconstrained_value.cpp PROPERTIES BOOST_DATE_TIME_POSIX_TIME_STD_CONFIG)
bcm_test(NAME testgregorian_calendar SOURCES testgregorian_calendar.cpp)
set_source_files_properties(testgregorian_calendar.cpp PROPERTIES BOOST_DATE_TIME_POSIX_TIME_STD_CONFIG)
bcm_test(NAME testgeneric_period SOURCES testgeneric_period.cpp)
set_source_files_properties(testgeneric_period.cpp PROPERTIES BOOST_DATE_TIME_POSIX_TIME_STD_CONFIG)

Which would be the same as adding it to every test with target_compile_definitions:

bcm_test(NAME testint_adapter SOURCES testint_adapter.cpp)
target_compile_definitions(testint_adapter PUBLIC BOOST_DATE_TIME_POSIX_TIME_STD_CONFIG)
bcm_test(NAME testtime_resolution_traits SOURCES testtime_resolution_traits.cpp)
target_compile_definitions(testtime_resolution_traits PUBLIC BOOST_DATE_TIME_POSIX_TIME_STD_CONFIG)
bcm_test(NAME testwrapping_int SOURCES testwrapping_int.cpp)
target_compile_definitions(testwrapping_int PUBLIC BOOST_DATE_TIME_POSIX_TIME_STD_CONFIG)
bcm_test(NAME testconstrained_value SOURCES testconstrained_value.cpp)
target_compile_definitions(testconstrained_value PUBLIC BOOST_DATE_TIME_POSIX_TIME_STD_CONFIG)
bcm_test(NAME testgregorian_calendar SOURCES testgregorian_calendar.cpp)
target_compile_definitions(testgregorian_calendar PUBLIC BOOST_DATE_TIME_POSIX_TIME_STD_CONFIG)
bcm_test(NAME testgeneric_period SOURCES testgeneric_period.cpp)
target_compile_definitions(testgeneric_period PUBLIC BOOST_DATE_TIME_POSIX_TIME_STD_CONFIG)

Which is what you want to avoid.

eldiener commented 6 years ago

That is my misunderstanding then. I thought you could use set_source_file_properties to add the properties to the CMakeLists.txt file itself so that all builds referenced in the CMakeLists.txt file had those properties. That is essentially the effect of the 'project' requirements in a Boost Build jamfile and I was looking for the CMake equivalent. What about setting the same properties for the directory itself using set_directory_properties ? I assume the directory is represented by the CMakeLists.txt file in that directory, so maybe that will be equivalent to the Boost Build 'project' requirements. Looking at the directory properties in the current CMake implementation I see COMPILE_DEFINITIONS and COMPILE_OPTIONS, so that should cover the majority of potential Boost Build 'project' requirements.

pfultz2 commented 6 years ago

What about setting the same properties for the directory itself using set_directory_properties ?

But these properties dont set the properties on the target themselves, which can cause some problems(like with compatible interface properties). Plus, this just doesn't just affect the current directory but every subdirectory as well.

The directory scope properties are the same properties that are set when using the add_compile_options and add_definitions commands in cmake, but, in general, it is considered bad practice to use these in cmake, see here.

eldiener commented 6 years ago

Are you saying that setting directory properties do not set the appropriate properties when creating targets from the files in that directory ( and sub-directories ) ? If that is the case what is the actual purpose of directory properties ?

pfultz2 commented 6 years ago

Are you saying that setting directory properties do not set the appropriate properties when creating targets from the files in that directory ( and sub-directories ) ?

Nope, when cmake builds a target it uses the properties from the target and then the properties from the current(and parent) directory its building.

If that is the case what is the actual purpose of directory properties ?

Its useful for toolchain files, but I don't think that was its original intent. Originally, cmake was very much directory-oriented and had no way to set usage requirements. However, now usage requirements are set with with target properties, and to prevent unexpected surprises those directory properties should, in general, be avoided outside of toolchain files.

eldiener commented 6 years ago

If I therefore set a directory property in a CMakeLists.txt file for tests, that property will filter down to all test targets which are built in that CMakeLists.txt file. This is the same effect that setting a 'project' requirement serves in a Boost Build jamfile. So despite your own feeling that directory properties should be avoided, it serves the purpose of duplicating Boost Build 'project' requirements closer than any other CMake facility.

I will readily admit that Boost Build does not have the notion that CMake can have of treating a header-file only library as an INTERFACE library target. So naturally Boost Build does not have the notion of setting properties on a header-file only library as a target. Whereas setting properties in CMake for an INTERFACE library gets propagated to any other executable or library which uses the INTERFACE library. So I do understand your feeling that setting the properties on the header-file only library itself is a better match for Boost Build 'project' requirements than setting those properties as directory properties.