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

Use pre-commit for style checks #336

Merged
merged 16 commits into from
Jan 4, 2023
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
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
63 changes: 63 additions & 0 deletions .pre-commit-config.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
# Copyright (c) 2023, NVIDIA CORPORATION.

repos:
- repo: https://github.com/pre-commit/pre-commit-hooks
rev: v4.4.0
hooks:
- id: trailing-whitespace
- id: end-of-file-fixer
- repo: https://github.com/pre-commit/mirrors-clang-format
rev: v11.1.0
hooks:
- id: clang-format
types_or: [c, c++, cuda]
args: ["-fallback-style=none", "-style=file", "-i"]
- repo: https://github.com/codespell-project/codespell
rev: v2.2.2
hooks:
- id: codespell
exclude: |
(?x)^(
^CHANGELOG.md$
)
- repo: local
hooks:
- id: copyright-check
name: copyright-check
entry: python ./ci/checks/copyright.py --git-modified-only --update-current-year
language: python
pass_filenames: false
additional_dependencies: [gitpython]
- id: cmake-format
name: cmake-format
entry: ./ci/checks/run-cmake-format.sh cmake-format
language: python
types: [cmake]
# Note that pre-commit autoupdate does not update the versions
# of dependencies, so we'll have to update this manually.
additional_dependencies:
- cmakelang==0.6.13
verbose: true
require_serial: true
files: |
(?x)^(
^rapids-cmake/.*$
)
- id: cmake-lint
name: cmake-lint
entry: ./ci/checks/run-cmake-format.sh cmake-lint
language: python
types: [cmake]
# Note that pre-commit autoupdate does not update the versions
# of dependencies, so we'll have to update this manually.
additional_dependencies:
- cmakelang==0.6.13
verbose: true
require_serial: true
files: |
(?x)^(
^rapids-cmake/.*$
)

default_language_version:
python: python3
4 changes: 2 additions & 2 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ Please see https://github.com/rapidsai/rapids-cmake/releases/tag/v23.02.00a for

- nvcomp install rules need to match the pre-built layout (#194) @robertmaynard
- Use target name variable. (#187) @bdice
- Remove uneeded message from rapids_export_package (#183) @robertmaynard
- Remove unneeded message from rapids_export_package (#183) @robertmaynard
- rapids_cpm_thrust: Correctly find version 1.15.0 (#181) @robertmaynard
- rapids_cpm_thrust: Correctly find version 1.15.0 (#180) @robertmaynard

Expand Down Expand Up @@ -241,7 +241,7 @@ Please see https://github.com/rapidsai/rapids-cmake/releases/tag/v23.02.00a for
- Introduce rapids_cpm_libcudacxx (#111) @robertmaynard
- Record formatting rules for rapids_cpm_find DOWNLOAD_ONLY option (#94) @robertmaynard
- rapids_cmake_install_lib_dir now aware of GNUInstallDirs improvements in CMake 3.22 (#85) @robertmaynard
- rapids-cmake defaults to always download overriden packages (#83) @robertmaynard
- rapids-cmake defaults to always download overridden packages (#83) @robertmaynard

## 🛠️ Improvements

Expand Down
4 changes: 2 additions & 2 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
#=============================================================================
# Copyright (c) 2021, NVIDIA CORPORATION.
# Copyright (c) 2021-2023, NVIDIA CORPORATION.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
Expand All @@ -24,7 +24,7 @@
# use rapids-cmake via CPM inside the same global project. In those
# cases it can fail due to CMAKE_MODULE_PREFIX not being exported properly

# Enfore the minimum required CMake version for all users
# Enforce the minimum required CMake version for all users
cmake_minimum_required(VERSION 3.23.1 FATAL_ERROR)

set(rapids-cmake-dir "${CMAKE_CURRENT_LIST_DIR}/rapids-cmake")
Expand Down
6 changes: 3 additions & 3 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ into three categories:

While RAPIDS core provides commonly used scripts we know that they aren't universal and might need to be composed in different ways.

This means that the code we are developing should be designed for composibility, and all side-effects
This means that the code we are developing should be designed for composability, and all side-effects
or CMake behavior changes should be explicitly opt-in.

So when writing new rapids-cmake features make sure to think about how users might want to opt-in, and
Expand Down Expand Up @@ -69,14 +69,14 @@ that don't/can't use `rapids_add_library` to still opt-in to other features.

Please ensure that when you are creating new features you follow the following guidelines:
- Each function should follow the `rapids_<component>_<file_name>` naming pattern
- Each function should go into a separate `.cmake` file in the approiate directory
- Each function should go into a separate `.cmake` file in the appropriate directory
- Each user facing `.cmake` file should have include guards (`include_guard(GLOBAL)`)
- Each user facing `.cmake` file should be documented following the rst structure
- Each user facing function should be added to the `cmake-format.json` document
- Run `cmake-genparsers -f json` on the `.cmake` file as a starting point
- Each function first line should be `list(APPEND CMAKE_MESSAGE_CONTEXT "rapids.<component>.<function>")`
- A file should not modify any state simply by being included. State modification should
only occur inside functions unless absolutely neccessary due to restrctions of the CMake
only occur inside functions unless absolutely necessary due to restrictions of the CMake
language.
- Any files that do need to break this rule can't be part of `rapids-<component>.cmake`.

Expand Down
59 changes: 3 additions & 56 deletions ci/check_style.sh
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
#!/bin/bash
# Copyright (c) 2020, NVIDIA CORPORATION.
# Copyright (c) 2020-2023, NVIDIA CORPORATION.

set -euo pipefail

Expand All @@ -17,58 +17,5 @@ set +eu
conda activate checks
set -u


CMAKE_FILES=(`find rapids-cmake/ | grep -E "^.*\.cmake?$|^.*/CMakeLists.txt$"`)
CMAKE_FILES+=("CMakeLists.txt")

CMAKE_FORMATS=()
CMAKE_FORMAT_RETVAL=0

CMAKE_LINTS=()
CMAKE_LINT_RETVAL=0

for cmake_file in "${CMAKE_FILES[@]}"; do
cmake-format --in-place --first-comment-is-literal --config-files ./cmake-format-rapids-cmake.json ./ci/checks/cmake_config_format.json -- ${cmake_file}
TMP_CMAKE_FORMAT=`git diff --color --exit-code -- ${cmake_file}`
TMP_CMAKE_FORMAT_RETVAL=$?
if [ "$TMP_CMAKE_FORMAT_RETVAL" != "0" ]; then
CMAKE_FORMAT_RETVAL=1
CMAKE_FORMATS+=("$TMP_CMAKE_FORMAT")
fi

TMP_CMAKE_LINT=`cmake-lint --config-files ./cmake-format-rapids-cmake.json ./ci/checks/cmake_config_format.json ./ci/checks/cmake_config_lint.json -- ${cmake_file}`
TMP_CMAKE_LINT_RETVAL=$?
if [ "$TMP_CMAKE_LINT_RETVAL" != "0" ]; then
CMAKE_LINT_RETVAL=1
CMAKE_LINTS+=("$TMP_CMAKE_LINT")
fi
done

# Output results if failure otherwise show pass
if [ "$CMAKE_FORMAT_RETVAL" != "0" ]; then
echo -e "\n\n>>>> FAILED: cmake format check; begin output\n\n"
for CMAKE_FORMAT in "${CMAKE_FORMATS[@]}"; do
echo -e "$CMAKE_FORMAT"
echo -e "\n"
done
echo -e "\n\n>>>> FAILED: cmake format check; end output\n\n"
else
echo -e "\n\n>>>> PASSED: cmake format check\n\n"
fi

if [ "$CMAKE_LINT_RETVAL" != "0" ]; then
echo -e "\n\n>>>> FAILED: cmake lint check; begin output\n\n"
for CMAKE_LINT in "${CMAKE_LINTS[@]}"; do
echo -e "$CMAKE_LINT"
echo -e "\n"
done
echo -e "\n\n>>>> FAILED: cmake lint check; end output\n\n"
else
echo -e "\n\n>>>> PASSED: cmake lint check\n\n"
fi

RETVALS=($CMAKE_FORMAT_RETVAL $CMAKE_LINT_RETVAL)
IFS=$'\n'
RETVAL=`echo "${RETVALS[*]}" | sort -nr | head -n1`

exit $RETVAL
# Run pre-commit checks
pre-commit run --all-files --show-diff-on-failure
Copy link
Member

@ajschmidt8 ajschmidt8 Jan 4, 2023

Choose a reason for hiding this comment

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

I can't make a suggestion for this via the web interface, but can you remove the +e on line 16 of this file?

Loading