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

Add support for including oneCCL in a CMake mono build #83

Open
sogartar opened this issue Nov 4, 2022 · 2 comments
Open

Add support for including oneCCL in a CMake mono build #83

sogartar opened this issue Nov 4, 2022 · 2 comments

Comments

@sogartar
Copy link

sogartar commented Nov 4, 2022

It would be nice to be able to include oneCCL inside another build directly.

add_subdirectory(oneCCL)

This entails that all CMake configuration arguments should be prefixed by something like ONECCL_ to avoid name conflicts.
Also all targets should be prefixed. E.g. oneccl::.

Another thing is to allow for all dependencies like googletest to be excludable from the build an provided externally. They may be defined somewhere else in the super build. E.g. ONECCL_GOOGLE_TEST_EXTERNAL=ON.

@nikitaxgusev
Copy link
Contributor

Hello @sogartar, we'd like to get your case, is it possible for you to show how and which project you include oneccl to reproduce this issue?

@sogartar
Copy link
Author

This is more of a feature request.
Nowadays it is very common for CMake projects to have a superbuild where most/all its dependencies are included in the superbuild and built together with the project.
To achieve this properly you also need to be able to exclude your dependency from your superbuild but have the provided from outside to prevent conflicts with other dependencies' superbuild.
A classic example is gtest because it is often included in the superbuild. OneCCL is also doing this here

add_subdirectory(${GTEST_DIR} gtest_build)

It technically has a superbuild.
It should be something like

if(NOT ONECCL_GOOGLE_TEST_EXTERNAL)
  set(GTEST_DIR ${PROJECT_SOURCE_DIR}/../googletest-release-1.8.1/googletest)
  add_subdirectory(${GTEST_DIR} gtest_build)
if()

I also see there are things like

set(INC_DIRS
    ${GTEST_DIR}/include
    ${GTEST_DIR}/src
    ${EXAMPLES_DIR}/include)

include_directories(${INC_DIRS})

which is not the proper way to do it in CMake >= 3.0. You should use targets wherever possible and let the target provide its include directories and link libraries.

target_link_libraries(mytarget PRIVATE gtest)

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

No branches or pull requests

2 participants