10000 Sankel feedback on BCM by camio · Pull Request #4 · boost-cmake/bcm · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Sankel feedback on BCM #4

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Sankel feedback on BCM #4

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions doc/src/BCMInstallTargets.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,10 @@ bcm_install_targets
This installs the targets specified. The directories will be installed according to GNUInstallDirs.
It will also install a corresponding cmake package config(which can be found with ``find_package``) to link against the library targets.

.. It doesn't "install the targets specified" as much as it "generates
installation rules" which install the specified targets. The wording in
'cmake --help-command install' is a bit more clear.

.. option:: TARGETS <target-name>...

The name of the targets to install.
Expand Down
18 changes: 18 additions & 0 deletions doc/src/BCMPackage.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,11 @@ bcm_find_package

This works the same as cmake's ``find_package`` except in addition, it will keep track of the each call to ``bcm_find_package`` in the project. Functions like ``bcm_package`` and ``bcm_boost_package`` will include these dependencies automatically in cmake's package config that gets generated.

.. I don't understand why 'bcm_find_package' needs to do anything other than
what the standard find_package does. Can't the targets exported by
'find_package' carry all the "package config" information needed for '.pg'
file generation?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

-----------
bcm_package
-----------
Expand All @@ -18,10 +23,17 @@ bcm_package

This setups a non-boost package in cmake. This is similar to ``bcm_boost_package`` but with extra flexibility as it does not have boost specific conventions. This function creates a library target that will be installed and exported. In addition, the target will be setup to auto-link for the tests.

.. "setup to auto-link for the tests". This is a bit unclear. Maybe say
"Subsequent uses of 'bcm_test' will automatically include this target as a
dependency" or something along these lines.
Copy link
Member
@pfultz2 pfultz2 Jun 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


.. option:: <package-name>

The name of the package. This is both the name of the library target and the cmake package config that can be used with ``find_package(<package-name>)``.

.. This is inconsistent with 'bcm_boost_package' below. Why does Boost have to
be special?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


.. option:: VERSION <version>

Sets the version of the package.
Expand All @@ -30,6 +42,10 @@ Sets the version of the package.

This will parse the version from a header file. It will parse the macros defined in the header as ``<prefix>_<package-name>_VERSION_MAJOR``, ``<prefix>_<package-name>_VERSION_MINOR``, and ``<prefix>_<package-name>_VERSION_PATCH``. The ``prefix`` by default is the package name in all caps, but can be set using the ``VERSION_PREFIX`` option.

.. I'm a bit unsure about the above and following option. I think many codebases
use their own "version header" format and hardcoding this one is a but
unflexible.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).


.. option:: VERSION_PREFIX <prefix>

This sets the prefix that macros used to define the version will use.
Expand Down Expand Up @@ -74,3 +90,5 @@ The source files to build the library.

This specifies internal boost dependencies, that is, dependencies on other boost libraries. The libraries should not be prefixed with ``boost_`` nor ``boost::``.

.. Why not use 'boost::' here? This special casing will be unobvious for casual
readers of code which uses this library.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

3 changes: 3 additions & 0 deletions doc/src/BCMProperties.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@ CXX_EXCEPTIONS

This property can be used to enable or disable C++ exceptions. This can be applied at global, directory or target scope. At global scope this defaults to On.

.. Why these particular C++ options? If we go down this route, won't someone
always be dissappointed because their particular option won't be here?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


--------
CXX_RTTI
--------
Expand Down
25 changes: 25 additions & 0 deletions doc/src/Intro.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,14 @@ Motivation

This provides cmake modules that can be re-used by boost and other dependencies. It provides modules to reduce the boilerplate for installing, versioning, setting up package config, and creating tests.

.. It isn't immediately clear what this is going to provide for the user.
Perhaps something like this: BCM provides CMake functions for building and
releasing software packages. It is a simplification of CMake's builtin
functions and additionally incorporates pkg-config support.

.. This section should be expanded as well. Perhaps with a side-by-side
comparison of the CMake way of doing things in comparison to the BCM way.

=====
Usage
=====
Expand Down Expand Up @@ -44,6 +52,10 @@ The BCM modules provide some high-level cmake functions to take care of all the

bcm_auto_pkgconfig()

.. The above snippet is mysterious for folks not accustomed to this library.
Where are the '.cpp' files being added? Where are the include files? What
do these functions do?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


This sets up the Boost.Config cmake with the version ``1.61.0``. More importantly the user can now install the library, like this::

mkdir build
Expand All @@ -63,6 +75,8 @@ Or if the user isn't using cmake, then ``pkg-config`` can be used instead::

g++ `pkg-config boost_config --cflags --libs` foo.cpp

.. Very nice!

------------
Dependencies
------------
Expand All @@ -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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


This will call ``find_package`` for the dependency and link it in the library. This structured this way to be able to support superbuilds(ie building all libraries together) in the future.

-----
Expand All @@ -86,6 +106,11 @@ The BCM modules provide functions for creating tests that integrate into cmake's

bcm_test(NAME config_test_c SOURCES config_test_c.c)

.. 'that' should be 'which' in the first sentence in the above paragraph I'm
pretty sure.

This will compile the ``SOURCES`` and run them. Also, there is no need to link in the ``boost_config``, as ``bcm_test`` will automatically link it in for us. Also, tests can be specified as compile-only or as expected to fail::

bcm_test(NAME test_thread_fail1 SOURCES threads/test_thread_fail1.cpp COMPILE_ONLY WILL_FAIL)

.. Nice!
0