Re: [boost] [cmake] Minimum viable cmakeification for Boost

Subject: Re: [boost] [cmake] Minimum viable cmakeification for Boost
From: P F (pfultz2_at_[hidden])
Date: 2017-06-20 13:32:22

> On Jun 20, 2017, at 7:17 AM, Niall Douglas via Boost <boost_at_[hidden]> wrote:
> 
> On 20/06/2017 07:27, P F wrote:
>> 
>>> On Jun 19, 2017, at 6:42 PM, Niall Douglas via Boost
>>> <boost_at_[hidden]> wrote:
>>> 
>>> I've finished the mockup which can be studied at:
>>> 
>>> https://github.com/ned14/boost-bmv-cmake
>>> 
>>> Just Boost.System was cmakeified. Extensive comments are throughout
>>> to explain every single line of cmake and why it's there. A small
>>> sample program is in the root of the repo to show how this cmake
>>> would be used by end user programs.
>> 
>> Here some feedback, I have:
>> 
>> - Having things like `::hl` or `::sl` is quite strange. In cmake, I
>> set `BUILD_SHARED_LIBS=On` when I want a shared library or I set it
>> to `BUILD_SHARED_LIBS=Off` when I want a static library. There should
>> only be one target `boost::config` or `boost::system` that I use.
> 
> BUILD_SHARED_LIBS is a cmake2-ism and should not be present in modern
> cmake.

No, cmake best practice including cmake 3:

* Leave the control of `BUILD_SHARED_LIBS` to your clients[1]

> Moreover, cmake now supports header only libraries, and
> BUILD_SHARED_LIBS causes problems with eventual C++ Modules support. The
> ::hl suffix means "link against the header only library edition", the
> ::sl suffix means "link against the static library edition" and the ::dl
> suffix means "link against the dynamic library edition".
> 
> Some bikeshedding over those names occurred off list. They don't hugely
> matter, they could be boost::system::header, boost::system::static and
> boost::system::shared if you wanted.
> 
> Some bikeshedding also occurred over what boost::system with no suffix
> should mean. I recommend "whatever library edition the maintainer
> recommends as the best one" which is NOT necessarily the header only
> edition. For example, in the future with C++ Modules support the
> additional suffixs ::hlm, ::slm and ::dlm will be needed. Or longer
> named, boost::system::header_module, boost::system::static_module and
> boost::system::shared_module.
> 
> So if C++ Modules were available, the maintainer might recommend the
> ::dlm variety, but recommend the ::hl variety if they weren't available.
> I think that will hugely vary per library as C++ Modules won't
> necessarily be always a win. Also, end users may wish to assemble all of
> Boost into a single C++ Module. We don't want to get in the way.
> 
>> - Each project do things like
>> `include("${CMAKE_CURRENT_SOURCE_DIR}/../../cmake/BoostVersion.cmake”)`
>> which is broken when built on its own.
> 
> That was intentional. Some bikeshedding occurred over that too with
> respect to which Boost library guaranteed to be a dependency for all
> other libraries ought to host the version setting script. Boost.Config
> is most likely. So the above is a placeholder.

Yes, but this breaks cmake best practice that says:

* Make sure all your projects can be built as standalone and as a subproject of another project.

> 
>> - Doing `if(NOT TARGET boost::assert::hl)` is wrong. A target does
>> not have to exists to be able to link against it. Plus, it should
>> call `find_package(boost_assert)` to bring in the target for when its
>> not built in the superproject.
> 
> That part is there solely to enable modular build. If you don't want to
> bother with modular build as this is a bare minimum viable
> cmakeification, you can eliminate it.

What do you mean by modular build? There is only two ways it can be as standalone or as a modular build.

> 
>> - There is no installation of the targets
> 
> Not necessary right now, so out of scope.

It is necessary.

> 
>> - Doing `target_include_directories(boost_core-hl INTERFACE
>> "include”)` is wrong as this will only add the include directories
>> for the build. The include directories should be added for the build
>> and for the installation.
> 
> As I've repeatedly explained now, installation logic is best implemented
> by a rootmost CMakeLists. Not in the per-library CMakeLists. We do tell
> cmake what targets are used by installation via install(TARGETS ...).
> That's the bare minimum needed for other cmake to implement the
> remaining installation logic.

Even if were implemented in the topmost, it would be broken when using `find_package` as the target is using the include directories in the build directory and not the install directory.

> 
> (On wider picture stuff, install paths need to be absolute I've found,
> whereas build paths can usually be relative. Relative paths are much
> nicer during build. In a rootmost CMakeLists which implements some
> installation logic, you'd iterate the targets for the Boost libraries
> and configure each programmatically for the appropriate install for that
> library and that type of library. All the metadata you need to make
> decisions is available)

I dont what you are saying here. Install paths should be relative or they should use the `$<INSTALL_PREFIX>` generator expression in order to make the installation relocatable.

[1]: https://de.slideshare.net/DanielPfeifer1/cmake-48475415