wjakob / tbb

Intel TBB with CMake build system
Apache License 2.0
369 stars 161 forks source link

Export CMake package configuration #37

Closed tschw closed 4 years ago

tschw commented 7 years ago

Why is this desirable?

Simple:

# only needed once per dependent build tree:
find_package(tbb CONFIG)

# now to let my_project use tbb_static:
target_link_libraries(my_project PRIVATE tbb::tbb_static)

# that's all - no need to mess with directories, files, #defines, transitive dependencies

Note this is about installed libs. In some projects, you wouldn't want to always build everything using add_subdirectory, but instead use ExternalProject and the like.

tschw commented 7 years ago

Oh well, your CI relies on messy "in-core" builds and I made them an error :/

tschw commented 7 years ago

OK, that looks good, now: The remaining CI failures are already present on master, because the build hosts do not have a sufficiently new version of CMake installed (the minimum version required for this stuff would be 2.8.11, as far as I know. The top-level CMakeLists.txt was already on 2.8.12).

"In-core" builds are risky and usually discouraged, because it's up to the generator what files it creates (it may overwrite some sources by coincidence). I initially turned them into an error condition in order to remove the .gitignore entries and check the package config file into the SCM, which happens to have a (.gitignore'd) .cmake extension.

Now, due to the revert of that commit (in order to please the CI), the file is now checked-in AND ignored. I leave it up to you to deal with that issue - you may want to consider updating your build hosts to run "out-of-core" builds.

wjakob commented 7 years ago

OK, that looks good, now

Are you sure? The Travis builds on master pass all tests on master, and this doesn't.

"In-core" builds are risky and usually discouraged

The TBB builds should work for both in and out-of-core builds rather than lecturing the user about a preferable style :P

Please don't add debug prefixes or postfixes. Modern build systems (including any reasonable CMake-based one) use different output directories for release and debug builds, which obviates the use of different names.

tschw commented 7 years ago

OK, that looks good, now

Are you sure? The Travis builds on master pass all tests on master, and this doesn't.

Yes, I am certain that the problem exists on master. See here:

https://github.com/wjakob/tbb/blob/master/CMakeLists.txt#L1

I'm seeing an error message that the CMake version on the build host is way older than what's required by the buildfiles on master!

Here is what the Travis builds say:

$ cmake -DCMAKE_CXX_COMPILER=x86_64-w64-mingw32-g++ \
    -DCMAKE_TOOLCHAIN_FILE=build/mingw_cross_toolchain.cmake \
    -DGNU_HOST=x86_64-w64-mingw32

CMake Error at CMakeLists.txt:1 (cmake_minimum_required):

  CMake 2.8.12 or higher is required.  You are running version 2.8.7

Please don't add debug prefixes or postfixes. Modern build systems (including any reasonable CMake-based one) use different output directories for release and debug builds, which obviates the use of different names.

As explained in that commit, it's not about building, but about a conflict when installing the files. There is no convention like that for typical install trees, and rolling your own makes things more complicated for worse results. Ever seen /usr/local/libdebug on Linux?

Nowadays, I expect a well-written CMake file targeting 2.8.11+ to install correctly, including CMake target exports to easily use the package in other builds.

Just using add_subdirectory and adding the third-party libs to your code base doesn't scale too well: At some point, you will want to use package installations that can be used via ExternalProject in CMake, or integrate/interface with other build systems. Note that relying on build trees for that purpose would be very fragile (because they depend on the Generator, the build system and their versions), so you want to better use a more defined folder structure - the installation.

Without the postfix, the lib won't install correctly OOTB and you'll end up with invalid target export files for one variant when you install both. It's a cache variable, so the user can still reconfigure the postfix to an empty string if there is no need to install.

jschueller commented 4 years ago

This can be closed now as exports are already available

wjakob commented 4 years ago

Ok, thanks!