On Tue, 2017-10-03 at 19:58 +0300, Peter Dimov via Boost wrote:
paul wrote:
Hi all,
So I have been working at converting more of boost tests to cmake utilizing the BCM library, here:
This looks pretty good.
Thanks.
A few comments:
include(bcm/share/bcm/cmake/BCMConfig.cmake)
bcm/share/bcm doesn't seem the right place. tools/bcm, more likely.
Yea, I will look into moving that.
Looking at https://github.com/boost-cmake/boost/blob/1.64/libs/core/CMakeLists.txt as an example, clean and tidy; but I'd like to know what is the suggested way of keeping this file up to date. At minimum, this line:
bcm_setup_version(VERSION 1.64.0)
needs to be updated per each release; how is this to be done?
I had the version in a separate version.cmake file for this reason; so that version.cmake could be mass-updated by a script after each release, without having to either regenerate all CMakeLists files - implying that all local changes would be lost - or requiring maintainers to keep the version up to date - which they will not.
Yea I can set it up to read the version from a file.
A similar question arises with the sources and the dependencies, although those do not change as often or on such a fixed schedule. (I had those in separate files as well, for similar reasons, although one could go either way here.)
find_package(boost_assert)
As I already mentioned a number of times, this in my opinion must be
find_package(boost_assert VERSION 1.64.0 EXACT)
because there is never, ever, a scenario in which I want boost_core 1.64.0 to link to boost_assert 1.58.0.
The user may want to use boost_core 1.64.0 with boost_assert 1.64.1 or 1.65 because perhaps the newer version is from develop that fixes a problem they are having.
(Also, REQUIRED, as otherwise errors are not as informative.)
True, although some dependencies are optional, which I dont really account for yet.
target_link_libraries(boost_core INTERFACE boost::assert)
This manual specification of dependencies duplicates the find_package. These two could potentially be folded into something like
bcm_dependency(assert INTERFACE)
So this would be something like: function(bcm_dependency name type) find_package(boost_${name}) target_link_libraries(${PROJECT_NAME} ${type} ${name}) endfunction() I am not sure how well that works if the user wants to make the dependency optional or required, but I think it could be made to work. I have considered adding a function like: bcm_boost_depends(boost_core assert config) In general, I was trying to avoid abstracting out too much cmake, as it makes it unfamiliar to cmake users. Plus, I would like utilities that can be eventually merged to cmake upstream. However, if there are a lot of developers who would really like to write as a above, I could definitely add it.
Moving on to https://github.com/boost-cmake/boost/blob/1.64/libs/core/test/CMakeLists.txt ,
bcm_test(NAME core_addressof_test SOURCES addressof_test.cpp)
In the Jamfile, this is
run addressof_test.cpp ;
which automatically generates a name for the test. It would probably be more convenient if bcm_test could do that itself as well, including adding a prefix, so that
bcm_test(SOURCES addressof_test.cpp)
declared core_addressof_test, and
bcm_test(SOURCES addressof_test.cpp NAME addressof_test_name)
declared core_addressof_test_name so that one did not have to remember to prefix everything with core_.
That is something I am seriously considering. Although, if you want to change the properties, a name would be needed, so for something like: bcm_test(NAME core_typeinfo_test_no_rtti SOURCES typeinfo_test.cpp) set_target_properties(core_typeinfo_test_no_rtti PROPERTIES CXX_RTTI Off) The name would probably need to stay. I am also considering having `bcm_test` enforce that names start with the project name, so it better avoids name conflicts.
Apart from that, nice work, CMake fans ought to be happy. Needs to be brought up to date with master though. :-)
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boo st
.