Sankel feedback on BCM

[camio]

Overall, I have two big concerns about this approach:

• It is too tied to how Boost builds things and/or has incompatible
  Boost-specific workarounds.
• BCM provides an alternative to the typical CMake conventions. I'd much rather
  see standard CMake commands being used when possible, even if this means
  there'll be more code and repetition. I've seen similar shorthands made in
  several different projects and it has always resulted in harder-to-approach
  code and leaky abstractions. For maintainability, conventional CMake combined
  with a good coding standard seems to work the best.

[pfultz2]

It is too tied to how Boost builds things and/or has incompatible
Boost-specific workarounds.

The bcm_boost_package is very boost-specific, however, the rest of the functions can work with any project. I should provide an overview on how to use these modules in non-boost projects.

I've seen similar shorthands made in several different projects and it has always resulted in harder-to-approach code and leaky abstractions.

I've seen similar things as well(for example trilinos), but do you think these modules suffer the same issues?

I would like for boost libraries to be written in high-level way as it allows for us to generate a lot of things directly from that. For other libraries that are not boost-specific, they can use bcm_package or bcm_install_targets or bcm_auto_export where it makes sense, and use built-in cmake for everything else.

Of course, any abstraction can have the problems you mentioned, I want to make sure this is not the case here. Thanks for the feedback.

[pfultz2]

This is to track the dependency, it works exactly as find_package does except it stores this in a variable, so when 'foo-config.cmake' is generated, it will generate a call find_dependency for each bcm_find_package that is called. This only happens with bcm_boost_package or bcm_package. When using bcm_auto_export the user will still need to explicitly define each dependency.

[pfultz2]

Also, the PACKAGE_FOUND property could be used for this, however, this is a global property, so it will break superproject builds, I believe. Let me test this.

It could possible be named differently as well, since its only to be used for non-build dependencies.

[pfultz2]

Yes I think that sounds better. Although "subsequent uses" is not needed. If you write:

bcm_test(...)
bcm_package(...)

It will still use the package. This is because bcm_test links against the libraries or package at generation time.

[pfultz2]

The bcm_boost_package does it differently to enforce that a boost library's name starts with boost_, the find_package name starts with boost_, and the imported target starts with boost::. Alternatively, we could check that the name starts with boost_, if we want to enforce the naming. This could makes things more consistent and clearer.

[pfultz2]

Yes, but if a project uses a different version header format, then they can parse the header themselves and set the header:

# Parse custom version header
parse_header_version_header(MYVERSION)
bcm_setup_version(VERSION ${MYVERSION})

This format is what is used by some boost projects and kde projects as well(they provided similar functionality in their cmake modules: ecm_setup_version).

[pfultz2]

This is good point. I didn't because it always needs boost dependencies, so it might be a good idea to always require it to make it explicit that its a boost dependency:

bcm_boost_package(boost_core
    VERSION 1.61.0
    DEPENDS
        boost::assert
        boost::config
)

I don't know if it always helps clarity as the package is declared with boost_ and the dependency that is used starts with boost::. Maybe it might be better to say boost_assert instead of boost::assert. I am not sure.

[pfultz2]

This is used by many tests in boosts, and there may need to be more added to support other options as well.

[pfultz2]

Yes, I need to explain this better. This relies on boost's directory structure to find the includes. Of course, Boost.Core does not have any source files, so that is why it is not mentioned here.

[pfultz2]

The bcm_boost_package is setup this way so it can handle both superproject builds and modular builds. Ideally, we could remove DEPENDS and have it work like bcm_package, like this:

bcm_find_package(boost_assert)
bcm_find_package(boost_config)

bcm_boost_package(boost_core
    VERSION 1.61.0
)

target_link_libraries(boost_core boost::assert boost::config)

We will just need to hijack find_package for superproject builds.

Of course, I like the terse version more. I don't think its a problem mixing bcm functions with cmake functions. Its designed to do that. Also, I think it helps authors who are not familiar with cmake.

Finally, there is no tool to ensure the correctness of the above snippet. An author may do:

bcm_find_package(boost_assert)
bcm_find_package(boost_config)

bcm_boost_package(boost_core
    VERSION 1.61.0
)

target_link_libraries(boost_core boost::config)

Which is a mistake, but there is no way to detect this mistake.

Of course, for external dependencies, it needs to be done like above:

bcm_find_package(ZLib)

bcm_boost_package(boost_core
    VERSION 1.61.0
    DEPENDS
        assert
        config
)

target_link_libraries(boost_core Zlib::ZLib)

However, boost has very little external dependencies.

[pfultz2]

On Mon, 2017-07-24 at 05:22 +0000, Michael H. Cox wrote: @mikehcox commented on this pull request. In doc/src/Intro.rst: > @@ -76,6 +90,12 @@ For dependencies on other boost libraries, they can be listed with the ``DEPENDS              config      )   +.. This is interesting. I like the reduction in boilerplate, but I question the +   choice to go in such a different direction from the standard CMake way of +   doing things. It strikes me as a possibly leaky abstraction where folks will +   have a mix of code which uses both the CMake way and the BCM way; maintainers +   will have to be familiar with both. Definitely agree with camio on this. See this Youtube video by Daniel Pfeiffer https://www.youtube.com/watch?v=bsXLMQ6WgIk Having a set of guidelines, templates, and conventions on how to use modern CMake (3.0+) is a better way to go than reinventing CMake's conventions.

I have actually removed this function. I totally agree, I don't want to write functions that wraps cmake's current functions. My original goal was to make something easier for developers who do not want to learn cmake, but this does lead to maintenance problems. I am trying to follow the model the Daniel has outlined in his Effective CMake talk. However, this does lead to repeating the dependencies 3 times. Now I have refactored this, so the user creates the library using standard cmake, and then call `bcm_deloy` which will do the install and package configuration. This does reduce down the repetition without reinventing cmake conventions. You can see a comparison here: http://bcm.readthedocs.io/en/latest/src/Building.html Please let me know if you still think this is too much. I really want to avoid creating cmake functions like what is used in trillinos or llvm that seems to make the build more complicated. The ultimate goal is to build something that can be merged into cmake eventually. So that the function we provide are standard cmake functionality.