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

Simplify CMake linting with cmake-format #913

Merged
merged 5 commits into from
Nov 18, 2021

Conversation

vyasr
Copy link
Contributor

@vyasr vyasr commented Nov 10, 2021

This PR adds a script to find the cmake-format-rapids-cmake.json file in a standard location and run the cmake-format or cmake-lint programs with that config file. The script fails gracefully when the file cannot be found and is therefore suitable for use as a pre-commit hook in scenarios where no build directory (containing the config file) exists yet. A corresponding pre-commit configuration is added here as well, replacing the old cmake-format hook which did not use the rapids-cmake config file.

Resolves #903.

@vyasr
Copy link
Contributor Author

vyasr commented Nov 10, 2021

CC @robertmaynard

@harrism
Copy link
Member

harrism commented Nov 10, 2021

Please target 22.02 @vyasr .

@harrism harrism added CMake gpuCI improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Nov 10, 2021
scripts/run-cmake-format.sh Outdated Show resolved Hide resolved
@vyasr vyasr changed the base branch from branch-21.12 to branch-22.02 November 11, 2021 17:12
@vyasr vyasr requested review from a team as code owners November 11, 2021 17:12
@vyasr
Copy link
Contributor Author

vyasr commented Nov 11, 2021

I retargeted 22.02 before I force pushed the rebase, so a bunch of extra reviewer groups were requested that shouldn't be necessary.

.pre-commit-config.yaml Outdated Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
@ajschmidt8
Copy link
Member

rerun tests

@ajschmidt8 ajschmidt8 removed the request for review from a team November 17, 2021 18:09
@ajschmidt8
Copy link
Member

Removing ops-codeowners from the required reviews since it doesn't seem there are any file changes that we're responsible for. Feel free to add us back if necessary.

@harrism
Copy link
Member

harrism commented Nov 18, 2021

@vyasr if this is ready, go ahead and merge.

@vyasr
Copy link
Contributor Author

vyasr commented Nov 18, 2021

@gpucibot merge

1 similar comment
@ajschmidt8
Copy link
Member

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 27715b1 into rapidsai:branch-22.02 Nov 18, 2021
@ajschmidt8
Copy link
Member

@gpucibot merge

@vyasr, looks like you only have read permissions on this repo, which is why it didn't merge for you

rapids-bot bot pushed a commit to rapidsai/cudf that referenced this pull request Nov 20, 2021
This PR ports some improvements from rapidsai/rmm#913.

Authors:
  - Vyas Ramasubramani (https://github.com/vyasr)

Approvers:
  - Nghia Truong (https://github.com/ttnghia)
  - Bradley Dice (https://github.com/bdice)
  - Robert Maynard (https://github.com/robertmaynard)

URL: #9723
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEA] Make CMake style check used in CI easy to run locally
5 participants