Skip to content
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.

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

CMake: add module TBBInstallConfig #126

Closed
wants to merge 7 commits into from
Closed

CMake: add module TBBInstallConfig #126

wants to merge 7 commits into from

Conversation

AlexVeprev
Copy link
Contributor

It adds TBBInstallConfig module which can generate TBBConfig.cmake and TBBConfigVersion.cmake for different TBB installations. It is a new functionality which doesn't affect legacy interfaces.

It partly addresses the issues raised in PR #119 according to this comment.

Please find detailed documentation in the TBBInstallConfig section of cmake/README.rst.

Examples of the generated files can be found in a temporary folder cmake/demo:

How these files were created:

cd /tmp/tbb/cmake
mkdir demo
cmake -DINSTALL_DIR=./demo -DSYSTEM_NAME=Linux -DTBB_VERSION_FILE=../include/tbb/tbb_stddef.h -P tbb_config_installer.cmake

@hjmjohnson, I would really appreciate if you can take a look and provide some feedback.

@hjmjohnson
Copy link

@AlexVeprev I'll try really hard to look at this in the afternoon today.

Hans

PS: Thanks for looking into this.

@hjmjohnson
Copy link

@AlexVeprev This is OK. Do your expect that a user first uses the build.py program and then makes this call?

I still do not see a straight forward path from "git clone" to " installed package with standard install layout".

Copy link

@hjmjohnson hjmjohnson left a comment

Choose a reason for hiding this comment

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

ERRORS in the file need fixing:

cat 0001-BUG-CMAKE_CURRENT_LIST_FILE-should-be-CMAKE_CURRENT_.patch 
From d75afaa3896f1de83dfc2f24adac35a2ff1669f9 Mon Sep 17 00:00:00 2001
From: Hans Johnson <[email protected]>
Date: Wed, 27 Feb 2019 18:29:02 -0600
Subject: [PATCH] BUG: CMAKE_CURRENT_LIST_FILE should be CMAKE_CURRENT_LIST_DIR

File paths should be constructed from the CMAKE_CURRENT_LIST_DIR
variable.
---
 cmake/demo/TBBConfig.cmake                  | 6 +++---
 cmake/templates/TBBConfig.cmake.in          | 9 +++++----
 cmake/templates/TBBConfigForSource.cmake.in | 2 +-
 cmake/templates/TBBConfigInternal.cmake.in  | 2 +-
 4 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/cmake/demo/TBBConfig.cmake b/cmake/demo/TBBConfig.cmake
index 42d4823e..f0d64903 100644
--- a/cmake/demo/TBBConfig.cmake
+++ b/cmake/demo/TBBConfig.cmake
@@ -39,13 +39,13 @@ endif()
 unset(_tbbmalloc_proxy_ix)
 
 foreach (_tbb_component ${TBB_FIND_COMPONENTS})
-    set(_tbb_release_lib "${CMAKE_CURRENT_LIST_FILE}/../../lib${_tbb_component}.so.2")
-    set(_tbb_debug_lib "${CMAKE_CURRENT_LIST_FILE}/../../lib${_tbb_component}_debug.so.2")
+    set(_tbb_release_lib "${CMAKE_CURRENT_LIST_DIR}/../../lib${_tbb_component}.so.2")
+    set(_tbb_debug_lib "${CMAKE_CURRENT_LIST_DIR}/../../lib${_tbb_component}_debug.so.2")
 
     if (EXISTS "${_tbb_release_lib}" OR EXISTS "${_tbb_debug_lib}")
         add_library(TBB::${_tbb_component} SHARED IMPORTED)
         set_target_properties(TBB::${_tbb_component} PROPERTIES
-                              INTERFACE_INCLUDE_DIRECTORIES "${CMAKE_CURRENT_LIST_FILE}/../../../include")
+                              INTERFACE_INCLUDE_DIRECTORIES "${CMAKE_CURRENT_LIST_DIR}/../../../include")
 
         if (EXISTS "${_tbb_release_lib}")
             set_target_properties(TBB::${_tbb_component} PROPERTIES
diff --git a/cmake/templates/TBBConfig.cmake.in b/cmake/templates/TBBConfig.cmake.in
index e210b05f..e7ef6f17 100644
--- a/cmake/templates/TBBConfig.cmake.in
+++ b/cmake/templates/TBBConfig.cmake.in
@@ -20,7 +20,7 @@
 # Handling of TBB_VERSION is in TBBConfigVersion.cmake.
 
 if (NOT TBB_FIND_COMPONENTS)
-    set(TBB_FIND_COMPONENTS "tbb tbbmalloc tbbmalloc_proxy")
+    set(TBB_FIND_COMPONENTS "tbb;tbbmalloc;tbbmalloc_proxy")
     foreach (_tbb_component ${TBB_FIND_COMPONENTS})
         set(TBB_FIND_REQUIRED_${_tbb_component} 1)
     endforeach()
@@ -39,13 +39,13 @@ endif()
 unset(_tbbmalloc_proxy_ix)
 
 foreach (_tbb_component ${TBB_FIND_COMPONENTS})
-    set(_tbb_release_lib "${CMAKE_CURRENT_LIST_FILE}/@TBB_LIB_REL_PATH@/@TBB_LIB_PREFIX@${_tbb_component}.@TBB_LIB_EXT@")
-    set(_tbb_debug_lib "${CMAKE_CURRENT_LIST_FILE}/@TBB_LIB_REL_PATH@/@TBB_LIB_PREFIX@${_tbb_component}_debug.@TBB_LIB_EXT@")
+    set(_tbb_release_lib "${CMAKE_CURRENT_LIST_DIR}/@TBB_LIB_REL_PATH@/@TBB_LIB_PREFIX@${_tbb_component}.@TBB_LIB_EXT@")
+    set(_tbb_debug_lib "${CMAKE_CURRENT_LIST_DIR}/@TBB_LIB_REL_PATH@/@TBB_LIB_PREFIX@${_tbb_component}_debug.@TBB_LIB_EXT@")
 
     if (EXISTS "${_tbb_release_lib}" OR EXISTS "${_tbb_debug_lib}")
         add_library(TBB::${_tbb_component} SHARED IMPORTED)
         set_target_properties(TBB::${_tbb_component} PROPERTIES
-                              INTERFACE_INCLUDE_DIRECTORIES "${CMAKE_CURRENT_LIST_FILE}/@TBB_INC_REL_PATH@")
+                              INTERFACE_INCLUDE_DIRECTORIES "${CMAKE_CURRENT_LIST_DIR}/@TBB_INC_REL_PATH@")
 
         if (EXISTS "${_tbb_release_lib}")
             set_target_properties(TBB::${_tbb_component} PROPERTIES
@@ -68,6 +68,7 @@ foreach (_tbb_component ${TBB_FIND_COMPONENTS})
         set(TBB_${_tbb_component}_FOUND 1)
     elseif (TBB_FIND_REQUIRED AND TBB_FIND_REQUIRED_${_tbb_component})
         message(STATUS "Missed required Intel TBB component: ${_tbb_component}")
+        message(STATUS "  one or both of:\n   ${_tbb_release_lib}\n    ${_tbb_debug_lib}\n   files must exists.")
         set(TBB_FOUND FALSE)
         set(TBB_${_tbb_component}_FOUND 0)
     endif()
diff --git a/cmake/templates/TBBConfigForSource.cmake.in b/cmake/templates/TBBConfigForSource.cmake.in
index 2bccdd9e..59cb44ed 100644
--- a/cmake/templates/TBBConfigForSource.cmake.in
+++ b/cmake/templates/TBBConfigForSource.cmake.in
@@ -38,7 +38,7 @@ endif()
 
 set(TBB_INTERFACE_VERSION @TBB_INTERFACE_VERSION@)
 
-get_filename_component(_tbb_root "${CMAKE_CURRENT_LIST_FILE}" PATH)
+get_filename_component(_tbb_root "${CMAKE_CURRENT_LIST_DIR}" PATH)
 get_filename_component(_tbb_root "${_tbb_root}" PATH)
 
 foreach (_tbb_component ${TBB_FIND_COMPONENTS})
diff --git a/cmake/templates/TBBConfigInternal.cmake.in b/cmake/templates/TBBConfigInternal.cmake.in
index 9094343c..da29690d 100644
--- a/cmake/templates/TBBConfigInternal.cmake.in
+++ b/cmake/templates/TBBConfigInternal.cmake.in
@@ -38,7 +38,7 @@ endif()
 
 set(TBB_INTERFACE_VERSION @TBB_INTERFACE_VERSION@)
 
-get_filename_component(_tbb_root "${CMAKE_CURRENT_LIST_FILE}" PATH)
+get_filename_component(_tbb_root "${CMAKE_CURRENT_LIST_DIR}" PATH)
 get_filename_component(_tbb_root "${_tbb_root}" PATH)
 
 set(_tbb_x32_subdir @TBB_X32_SUBDIR@)
-- 
2.21.0

cmake/templates/TBBConfig.cmake.in Outdated Show resolved Hide resolved
@hjmjohnson
Copy link

I'm discouraged that I'll need to add python as a build requirement for my software package, but I can live with that if necessary. Other packages I work on will likely not use TBB as a mandatory requirement because the build system is non-standard.

This is what I came up with to build & test TBB:

#!/bin/bash
set -ev

## -- TODO BUILD_CACHE=$1
BUILD_CACHE=/tmp/20190226_Test01

SRC_DIR=${BUILD_CACHE}/tbb
INSTALL_PREFIX=${BUILD_CACHE}/tbb-install

SYSTEM_NAME=Darwin
TBB_VERSION_FILE=${SRC_DIR}/include/tbb/tbb_stddef.h
mkdir -p ${INSTALL_PREFIX}

pushd ${BUILD_CACHE}
# Get copy of tbb
if [[ ! -d tbb ]]; then
  git clone https://github.com/AlexVeprev/tbb.git
fi
GIT_TAG=cmake-install-config
pushd tbb
  git checkout ${GIT_TAG}
popd

python ${SRC_DIR}/build/build.py  \
       --tbbroot ${SRC_DIR} \
       --prefix ${INSTALL_PREFIX} \
       --install-libs --install-devel --install-docs
# NOTE:  --install does not work due to dependance on swig.
# TODO: determine if swig is available, and then install python components

cmake -DINSTALL_DIR=${INSTALL_PREFIX}/lib/cmake \
      -DLIB_REL_PATH=../../lib \
      -DINC_REL_PATH=../../include \
      -DSYSTEM_NAME=${SYSTEM_NAME} \
      -DTBB_VERSION_FILE=${TBB_VERSION_FILE} \
      -P ${SRC_DIR}/cmake/tbb_config_installer.cmake


DO_TESTING=1
if [[ ${DO_TESTING} -eq 1 ]]; then

TEST_DIR=${BUILD_CACHE}/tbb_test
mkdir -p ${TEST_DIR}

cat > ${TEST_DIR}/CMakeLists.txt << EOF
# Put this cmake file in a directory, run "cmake ."  finding TBB results in a FATAL_ERROR from cmake
cmake_minimum_required(VERSION 3.0)
find_package(TBB REQUIRED CONFIG)
if(NOT TBB_FOUND)
  message(FATAL_ERROR "NO TBB")
else()
  message(STATUS "FOUND TBB")
endif()
add_executable(tbb_test test.cpp)
target_link_libraries(tbb_test TBB::tbb)
EOF

cat > ${TEST_DIR}/test.cpp << EOF
#include <tbb/task_scheduler_init.h>
#include <iostream>
int main()
{
  std::cout << "Default number of threads: " << tbb::task_scheduler_init::default_num_threads() << std::endl;;
  return 0;
}
EOF

mkdir -p ${TEST_DIR}-bld
pushd ${TEST_DIR}-bld
cmake -DCMAKE_BUILD_TYPE:STRING=Release -DTBB_DIR:PATH=${INSTALL_PREFIX}/lib/cmake  ${TEST_DIR}
make 
./tbb_test

if [[ $? -eq 0 ]]; then
  echo "SUCCESSFUL TESTING"
else
  echo "FAILED TESTING"
fi

fi

@AlexVeprev
Copy link
Contributor Author

AlexVeprev commented Feb 28, 2019

@hjmjohnson, thank you for the review.

I didn't have time to test this PR yesterday, but today I applied fixes for the bugs.

Do your expect that a user first uses the build.py program and then makes this call?

Actually this PR is only about TBBConig files, but not about build/install of TBB itself. It doesn't fully resolve your critique, but I hope it is a step forward.

How this module can be used by package maintainers (more natural way): to generate proper TBB CMake configs for their TBB packages. For example, in homebrew-coreFormula/tbb.rb#L39.

How this module can be used by a user (rather workaround): to install new (or fix existing) TBB CMake configs for TBB installation done trough dnf/apt/yum/brew/etc... See my tests with dnf and apt below.

This module can be used in "download->build[->test]->install" flow of TBB itself. Yes, now this flow is not standard (as you explained above), but this particular PR is only about one small part of this flow, but not about the flow itself.


Tests

I used sub_string_finder TBB example with the following CMakeLists.txt:

cmake_minimum_required(VERSION 3.0.0 FATAL_ERROR)
project(sub_string_finder CXX)
add_executable(sub_string_finder sub_string_finder.cpp)
find_package(TBB REQUIRED tbb)
target_link_libraries(sub_string_finder ${TBB_IMPORTED_TARGETS})

-DCMAKE_NO_SYSTEM_FROM_IMPORTED=True is used to avoid an error:
/usr/include/c++/8/cstdlib:75:15: fatal error: stdlib.h: No such file or directory caused by -isystem option for specifying path to TBB headers (from imported TBB target). https://cmake.org/cmake/help/latest/prop_tgt/NO_SYSTEM_FROM_IMPORTED.html

Ubuntu 18.10 - apt

$> apt install libtbb-dev
$> cmake -DINSTALL_DIR=/usr/lib/cmake/TBB -DSYSTEM_NAME=Linux -DLIB_PATH=/usr/lib/x86_64-linux-gnu -DINC_PATH=/usr/include -P tbb_config_installer.cmake
$> cd /tmp/examples/GettingStarted/sub_string_finder
$> mkdir build && cd build
$> cmake -DCMAKE_NO_SYSTEM_FROM_IMPORTED=True .. && make && ./sub_string_finder > /dev/null && echo "done"
-- The CXX compiler identification is GNU 8.2.0
-- Check for working CXX compiler: /usr/bin/c++
-- Check for working CXX compiler: /usr/bin/c++ -- works
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Configuring done
-- Generating done
-- Build files have been written to: /tmp/examples/GettingStarted/sub_string_finder/build
Scanning dependencies of target sub_string_finder
[ 50%] Building CXX object CMakeFiles/sub_string_finder.dir/sub_string_finder.cpp.o
[100%] Linking CXX executable sub_string_finder
[100%] Built target sub_string_finder
done
$> ldd sub_string_finder | grep tbb
        libtbb.so.2 => /usr/lib/x86_64-linux-gnu/libtbb.so.2 (0x00007f07af767000)

Fedora 29 - dnf

$> dnf install -y tbb-devel
$> cmake -DINSTALL_DIR=/usr/lib64/cmake/TBB -DSYSTEM_NAME=Linux -DLIB_PATH=/usr/lib64 -DINC_PATH=/usr/include -P tbb_config_installer.cmake
$> cd /tmp/examples/GettingStarted/sub_string_finder
$> mkdir build && cd build
$> cmake -DCMAKE_NO_SYSTEM_FROM_IMPORTED=True .. && make && ./sub_string_finder > /dev/null && echo "done"
-- The CXX compiler identification is GNU 8.2.1
-- Check for working CXX compiler: /usr/bin/c++
-- Check for working CXX compiler: /usr/bin/c++ -- works
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Configuring done
-- Generating done
-- Build files have been written to: /tmp/examples/GettingStarted/sub_string_finder/build
Scanning dependencies of target sub_string_finder
[ 50%] Building CXX object CMakeFiles/sub_string_finder.dir/sub_string_finder.cpp.o
[100%] Linking CXX executable sub_string_finder
[100%] Built target sub_string_finder
done
$> ldd sub_string_finder | grep tbb
        libtbb.so.2 => /lib64/libtbb.so.2 (0x00007fe7ab62f000)

@hjmjohnson
Copy link

@AlexVeprev Thank you. First let me state that I sincerely appreciate your efforts. I firmly believe the the libraries of TBB are of great value, and that the paradigms promoted by tbb are of paramount importance for enabling maintainable, performant, and extensible tools that take advantage of modern hardware.

It is still my belief that the build environment used by tbb (custom hard-wired makefiles with non-standard build/configure/install paradigms, and secondary layers of 'build patches' like build.py and the cmake scripts around the core build environment) will continue to be in impediment to wider spread adoption.

I GREATLY appreciate your effort to provide some environment that can be used to successfully build tbb in a wide variety of platforms.

@AlexVeprev
Copy link
Contributor Author

@hjmjohnson thank you! I'll continue to work on improvements in TBB environment to simplify integration.

@hjmjohnson
Copy link

@AlexVeprev Another bug fix was needed to make this work robustly when find_package(TBB) is called multiple times.

See : #127

@anton-malakhov
Copy link

FYI, TBB has Fibonacci/test_all for the sake of light sanity check of the self-built libraries. I used that in tbb-feedstock

@hjmjohnson
Copy link

@AlexVeprev Is there any progress on moving this into the main TBB repository? There now appears to be a merge conflict.

@AlexVeprev
Copy link
Contributor Author

@hjmjohnson it should be incorporated into the next release, I will notify you here.

@hjmjohnson
Copy link

@AlexVeprev Thanks. Would it be correct to tell our community of users that we need to wait until the June release of TBB before a working version will be available for public distribution?

@AlexVeprev
Copy link
Contributor Author

@hjmjohnson I don't know exact date of the next release publication, but it is expected to be published much earlier than June.

@hjmjohnson
Copy link

@AlexVeprev Thanks. I appreciate your help.

@tbbdev
Copy link
Contributor

tbbdev commented Mar 21, 2019

Implemented in TBB 2019 Update 5.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants