Re: [boost] Boost CMake update

Subject: Re: [boost] Boost CMake update
From: paul (pfultz2_at_[hidden])
Date: 2017-10-03 21:08:48

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:
> > 
> > https://github.com/boost-cmake/boost/tree/1.64
> 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

.