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.

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

[REVIEW] CMake target cleanup, formatting, linting #604

Merged
merged 25 commits into from
Nov 20, 2020
Merged

[REVIEW] CMake target cleanup, formatting, linting #604

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
2a3b207
Further CMake cleanup, add formatting and linting
kkraus14 Oct 13, 2020
f5b2879
set up pre-commit hook for cmake-format and cmake-lint
kkraus14 Oct 13, 2020
aa4e344
fix cmake breakages, fix all known linting issues
kkraus14 Oct 13, 2020
1b38e54
pin cmake-format version
kkraus14 Oct 13, 2020
62f93eb
remove unused config
kkraus14 Oct 13, 2020
56f0b81
fix function casing
kkraus14 Oct 13, 2020
3dba085
more comment cleanup
kkraus14 Oct 13, 2020
51dd405
match clang-format settings of 100 width, 2 spaces tabs
kkraus14 Oct 13, 2020
02e10e5
changelog
kkraus14 Oct 13, 2020
8e51a74
align pre-commit and conda versions with cudf
kkraus14 Oct 13, 2020
ecec53b
tweak some additional formatting config, fix cmake build issues
kkraus14 Oct 13, 2020
676822e
further config tweak to prevent vertical layout in situations
kkraus14 Oct 13, 2020
7efb644
Move compile definitions down to benchmarks / tests
kkraus14 Oct 14, 2020
1e0cd0a
Remove 'GPU_ARCHS' and move CUDA language down to tests/benchmarks
kkraus14 Oct 14, 2020
8b0f765
Merge branch 'branch-0.17' into cmake_target_cleanup
kkraus14 Oct 15, 2020
2e09862
Merge branch 'branch-0.17' into cmake_target_cleanup
kkraus14 Oct 15, 2020
ee5d025
temporarily upgrade cmake via gpu ci script
kkraus14 Oct 16, 2020
40b8bbc
typo in conda command
kkraus14 Oct 16, 2020
1c4be3b
need to call 'enable_testing' in root cmake
kkraus14 Oct 16, 2020
4f4f537
hook up formatting and linting to CI
kkraus14 Nov 5, 2020
5918222
add cmake-format to style checker conda environment temporarily
kkraus14 Nov 5, 2020
d5fe0f6
Merge branch 'branch-0.17' into cmake_target_cleanup
kkraus14 Nov 5, 2020
b1a1df8
remove 'cmake-format' install from style ci script
kkraus14 Nov 7, 2020
da29d84
Merge branch 'branch-0.17' into cmake_target_cleanup
kkraus14 Nov 16, 2020
080544a
use exclude list instead of include list
kkraus14 Nov 18, 2020
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
60 changes: 35 additions & 25 deletions .pre-commit-config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,27 +1,37 @@
repos:
- repo: https://github.com/timothycrosley/isort
rev: 5.0.4
hooks:
- id: isort
- repo: https://github.com/ambv/black
rev: stable
hooks:
- id: black
- repo: https://gitlab.com/pycqa/flake8
rev: 3.8.1
hooks:
- id: flake8
alias: flake8
name: flake8
args: ["--config=python/.flake8"]
types: [python]
- repo: https://gitlab.com/pycqa/flake8
rev: 3.8.1
hooks:
- id: flake8
alias: flake8-cython
name: flake8-cython
args: ["--config=python/.flake8.cython"]
types: [cython]
- repo: https://github.com/timothycrosley/isort
rev: 5.0.7
hooks:
- id: isort
- repo: https://github.com/ambv/black
rev: 19.10b0
hooks:
- id: black
- repo: https://gitlab.com/pycqa/flake8
rev: 3.8.3
hooks:
- id: flake8
alias: flake8
name: flake8
args: ["--config=python/.flake8"]
types: [python]
- id: flake8
alias: flake8-cython
name: flake8-cython
args: ["--config=python/.flake8.cython"]
types: [cython]
- repo: https://github.com/cheshirekow/cmake-format-precommit
rev: v0.6.11
hooks:
- id: cmake-format
name: cmake-format
args: ["--config-files", "cmake/config.json", "--in-place", "--"]
types: [file] # override `types: [cmake]`
files: \.(cmake(\.in)?)$|CMakeLists\.txt
- id: cmake-lint
args: ["--config-files", "cmake/config.json", "--"]
types: [file] # override `types: [cmake]`
files: \.(cmake(\.in)?)$|CMakeLists\.txt

default_language_version:
python: python3
python: python3
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

## Improvements

- PR #604 CMake target cleanup, formatting, linting

## Bug Fixes

# RMM 0.16.0 (Date TBD)
Expand Down
198 changes: 86 additions & 112 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,178 +1,152 @@
#=============================================================================
# =============================================================================
# Copyright (c) 2018, NVIDIA CORPORATION.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
# Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except
# in compliance with the License. You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
#=============================================================================
cmake_minimum_required(VERSION 3.17...3.18 FATAL_ERROR)
# Unless required by applicable law or agreed to in writing, software distributed under the License
# is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
# or implied. See the License for the specific language governing permissions and limitations under
# the License.
# =============================================================================
cmake_minimum_required(VERSION 3.18 FATAL_ERROR)

project(RMM VERSION 0.17.0 LANGUAGES C CXX CUDA)
project(
RMM
VERSION 0.17.0
LANGUAGES C CXX CUDA)
kkraus14 marked this conversation as resolved.
Show resolved Hide resolved

include(cmake/CPM.cmake)
include(cmake/Modules/CPM.cmake)
include(cmake/Modules/RMM_thirdparty.cmake)

###################################################################################################
# - build type
# build type

# Set a default build type if none was specified
set(DEFAULT_BUILD_TYPE "Release")

if(NOT CMAKE_BUILD_TYPE AND NOT CMAKE_CONFIGURATION_TYPES)
message(STATUS "Setting build type to '${DEFAULT_BUILD_TYPE}' since none specified.")
set(CMAKE_BUILD_TYPE "${DEFAULT_BUILD_TYPE}" CACHE
STRING "Choose the type of build." FORCE)
message(STATUS "RMM: Setting build type to '${DEFAULT_BUILD_TYPE}' since none specified.")
set(CMAKE_BUILD_TYPE
"${DEFAULT_BUILD_TYPE}"
CACHE STRING "Choose the type of build." FORCE)
# Set the possible values of build type for cmake-gui
set_property(CACHE CMAKE_BUILD_TYPE PROPERTY STRINGS
"Debug" "Release" "MinSizeRel" "RelWithDebInfo")
set_property(CACHE CMAKE_BUILD_TYPE PROPERTY STRINGS "Debug" "Release" "MinSizeRel"
"RelWithDebInfo")
endif()

###################################################################################################
# - compiler options

set(CMAKE_CXX_STANDARD 14)
set(CMAKE_CXX_STANDARD_REQUIRED ON)

if(CMAKE_CXX_COMPILER_ID MATCHES "GNU|Clang")
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Werror -Wno-error=deprecated-declarations")
endif(CMAKE_CXX_COMPILER_ID MATCHES "GNU|Clang")

###################################################################################################
# - build options
# build options

option(BUILD_TESTS "Configure CMake to build tests" ON)
option(BUILD_BENCHMARKS "Configure CMake to build (google) benchmarks" OFF)

option(DISABLE_DEPRECATION_WARNING "Disable warnings generated from deprecated declarations." OFF)
if(DISABLE_DEPRECATION_WARNING)
set(CMAKE_CUDA_FLAGS "${CMAKE_CUDA_FLAGS} -Xcompiler -Wno-deprecated-declarations")
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-deprecated-declarations")
endif(DISABLE_DEPRECATION_WARNING)
option(PER_THREAD_DEFAULT_STREAM "Build with per-thread default stream" OFF)
kkraus14 marked this conversation as resolved.
Show resolved Hide resolved

# cudart can be statically linked or dynamically linked the python ecosystem wants dynamic linking
option(CUDA_STATIC_RUNTIME "Statically link the CUDA runtime" OFF)

option(PER_THREAD_DEFAULT_STREAM "Build with per-thread default stream" OFF)

###################################################################################################
# find packages we depend on

find_package(CUDAToolkit REQUIRED)

###################################################################################################
# cmake modules

set(CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/cmake" ${CMAKE_MODULE_PATH})

###################################################################################################
# third-party dependencies

include(RMM_thirdparty)

###################################################################################################
# per-thread default stream option
# This needs to be defined first so tests and benchmarks can inherit it.

if(PER_THREAD_DEFAULT_STREAM)
message(STATUS "Using per-thread default stream")
add_compile_definitions(CUDA_API_PER_THREAD_DEFAULT_STREAM)
endif(PER_THREAD_DEFAULT_STREAM)

###################################################################################################
# library targets

add_library(rmm INTERFACE)
add_library(rmm::rmm ALIAS rmm)

target_include_directories(rmm INTERFACE
"$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include>"
"$<INSTALL_INTERFACE:include>"
)
target_include_directories(rmm INTERFACE "$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include>"
"$<INSTALL_INTERFACE:include>")

if(CUDA_STATIC_RUNTIME)
message(STATUS "Enabling static linking of cudart")
message(STATUS "RMM: Enabling static linking of cudart")
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this option even needed anymore? RMM doesn't produce a binary object anymore, so if someone wants to statically link against cudart, they can do that when compiling their binary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we don't have this then the CUDA dependency isn't properly enforced on the RMM target and it becomes the downstream project's responsibility to add it which is problematic.

Copy link
Contributor

Choose a reason for hiding this comment

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

Even though the library is header-only, it still needs to know what libraries to link against when it's being used -- either in the tests or in a downstream project. The choice of static vs dynamic runtime does get recorded together with the target, and picked up downstream.

Maybe another question is whether the static option is still used / needed anywhere in the first place?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You typically want to static link the CUDA libraries if you're not in a conda controlled environment. This allows shipping your application / binary without the user needing the CUDA toolkit installed.

target_link_libraries(rmm INTERFACE CUDA::cudart_static)
else()
target_link_libraries(rmm INTERFACE CUDA::cudart)
endif(CUDA_STATIC_RUNTIME)

target_link_libraries(rmm INTERFACE rmm::Thrust)
target_link_libraries(rmm INTERFACE spdlog::spdlog_header_only)
target_compile_features(rmm INTERFACE cxx_std_14 cuda_std_14)
target_compile_options(rmm INTERFACE $<$<COMPILE_LANG_AND_ID:CXX,GNU,Clang>:-Werror
-Wno-error=deprecated-declarations>)
kkraus14 marked this conversation as resolved.
Show resolved Hide resolved

###################################################################################################
# Set logging level. Must go before including gtests and benchmarks.
if(DISABLE_DEPRECATION_WARNING)
target_compile_options(rmm INTERFACE $<$<COMPILE_LANGUAGE:CUDA>:-Xcompiler
-Wno-deprecated-declarations>)
target_compile_options(rmm INTERFACE $<$<COMPILE_LANGUAGE:CXX>:-Wno-deprecated-declarations>)
endif(DISABLE_DEPRECATION_WARNING)

set(RMM_LOGGING_LEVEL "INFO" CACHE STRING "Choose the logging level.")
# Set the possible values of build type for cmake-gui
set_property(CACHE RMM_LOGGING_LEVEL PROPERTY STRINGS
"TRACE" "DEBUG" "INFO" "WARN" "ERROR" "CRITICAL" "OFF")
message(STATUS "RMM_LOGGING_LEVEL = '${RMM_LOGGING_LEVEL}'.")
if(PER_THREAD_DEFAULT_STREAM)
message(STATUS "RMM: Using per-thread default stream")
target_compile_definitions(rmm INTERFACE CUDA_API_PER_THREAD_DEFAULT_STREAM)
endif(PER_THREAD_DEFAULT_STREAM)

###################################################################################################
# add gtest
# Set logging level. Must go before including gtests and benchmarks.

set(RMM_LOGGING_LEVEL
"INFO"
CACHE STRING "Choose the logging level.")
# Set the possible values of build type for cmake-gui
set_property(
CACHE RMM_LOGGING_LEVEL
PROPERTY STRINGS
"TRACE"
"DEBUG"
"INFO"
"WARN"
"ERROR"
"CRITICAL"
"OFF")
kkraus14 marked this conversation as resolved.
Show resolved Hide resolved
message(STATUS "RMM: RMM_LOGGING_LEVEL = '${RMM_LOGGING_LEVEL}'")

# optionally build tests

if(BUILD_TESTS)
include(CTest)
add_subdirectory(tests)
add_subdirectory(tests)
endif(BUILD_TESTS)

###################################################################################################
# add google benchmark

if(BUILD_BENCHMARKS)
add_subdirectory(benchmarks)
endif(BUILD_BENCHMARKS)

###################################################################################################
# install targets

include(GNUInstallDirs)
set(INSTALL_CONFIGDIR ${CMAKE_INSTALL_LIBDIR}/cmake/rmm)

install(TARGETS rmm
EXPORT rmm-targets)
install(TARGETS rmm EXPORT rmm-targets)

install(DIRECTORY include/rmm
DESTINATION ${CMAKE_INSTALL_INCLUDEDIR})
install(DIRECTORY include/rmm DESTINATION ${CMAKE_INSTALL_INCLUDEDIR})

include(CMakePackageConfigHelpers)
configure_package_config_file(
cmake/rmm-config.cmake.in
${RMM_BINARY_DIR}/rmm-config.cmake
INSTALL_DESTINATION ${INSTALL_CONFIGDIR}
)

write_basic_package_version_file(
${RMM_BINARY_DIR}/rmm-config-version.cmake
COMPATIBILITY SameMinorVersion
)

install(EXPORT rmm-targets
configure_package_config_file(cmake/rmm-config.cmake.in ${RMM_BINARY_DIR}/rmm-config.cmake
INSTALL_DESTINATION ${INSTALL_CONFIGDIR})

write_basic_package_version_file(${RMM_BINARY_DIR}/rmm-config-version.cmake
COMPATIBILITY SameMinorVersion)

install(
EXPORT rmm-targets
FILE rmm-targets.cmake
NAMESPACE rmm::
DESTINATION ${INSTALL_CONFIGDIR}
)

install(FILES
${RMM_BINARY_DIR}/rmm-config.cmake
${RMM_BINARY_DIR}/rmm-config-version.cmake
${RMM_SOURCE_DIR}/cmake/install/FindThrust.cmake
DESTINATION ${INSTALL_CONFIGDIR}
)

###################################################################################################
# make documentation
DESTINATION ${INSTALL_CONFIGDIR})

install(FILES ${RMM_BINARY_DIR}/rmm-config.cmake ${RMM_BINARY_DIR}/rmm-config-version.cmake
${RMM_SOURCE_DIR}/cmake/install/FindThrust.cmake DESTINATION ${INSTALL_CONFIGDIR})

add_custom_command(OUTPUT RMM_DOXYGEN
WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/doxygen
COMMAND doxygen Doxyfile
VERBATIM)
# make documentation

add_custom_target(rmm_doc DEPENDS RMM_DOXYGEN)
add_custom_command(
OUTPUT RMM_DOXYGEN
WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/doxygen
COMMAND doxygen Doxyfile
VERBATIM
COMMENT "Custom command for RMM doxygen docs")

add_custom_target(
rmm_doc
DEPENDS RMM_DOXYGEN
COMMENT "Target for the custom command to build the RMM doxygen docs")
Loading