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

[FEA] Make CMake style check used in CI easy to run locally #9396

Closed
harrism opened this issue Oct 7, 2021 · 7 comments · Fixed by #9484
Closed

[FEA] Make CMake style check used in CI easy to run locally #9396

harrism opened this issue Oct 7, 2021 · 7 comments · Fixed by #9484
Labels
CMake CMake build issue feature request New feature or request

Comments

@harrism
Copy link
Member

harrism commented Oct 7, 2021

Is your feature request related to a problem? Please describe.

It's easy for developers to run the C++ and Python linters that get run in CI locally. I would like the same to be true of cmake style. Right now, fixing cmake style errors is an iterative process of looking at CI output, fixing locally, pushing, and then seeing more errors in the CI check and repeating.

Describe the solution you'd like
Include a shell script in the repo for running cmake style checks, like we have with run-clang-format.sh.

Additional context

I ran into this working on RMM, but it applies to cuDF and other repos as well, so I filed it here for visibility.

CC @robertmaynard

@harrism harrism added feature request New feature or request Needs Triage Need team to review and classify CMake CMake build issue labels Oct 7, 2021
@vyasr
Copy link
Contributor

vyasr commented Oct 7, 2021

I could be wrong, but while I see runs of cmake-format and cmake-lint in rmm's style check CI script, I don't see one in cudf's CI. That's not to say that we shouldn't do that linting, I would definitely be in favor of that.

Both clang-format and all of the linters that we use for cuDF Python (black, isort, flake8) are currently configured to work with pre-commit as well as by running them directly (and in clang-format's case, using the run-clang-format.sh script). I'm currently working on #9309, which aims to unify all Python linting using pre-commit. There are a number of benefits discussed in #9309, but the short version is that it gives us a single source of truth for maintaining desired linter versions while also making it easier for different RAPIDS libs to use different versions of the same linters if necessary. I'm only planning to touch Python linters when resolving #9309 since I expect that switching to pre-commit will be a (slightly) larger energy barrier for libcudf developers who aren't accustomed to it, but I have previously set up cmake-format to run via pre-commit on a different project and would be happy to do that for cudf as a follow-up. This is probably something worth discussing more before making any decision, but in the long run I think it would be preferable to centralize on pre-commit since in addition to making it easy to run (using something like pre-commit run cmake-format --all-files) it also allows (but does not require) users to automatically set up Git hooks for linting

@beckernick beckernick removed the Needs Triage Need team to review and classify label Oct 8, 2021
@harrism
Copy link
Member Author

harrism commented Oct 12, 2021

I prefer format-on-save and automatic in-IDE linter plugins (e.g. clangd). Precommit only lints when I commit, which is not frequently enough IMO.

@vyasr
Copy link
Contributor

vyasr commented Oct 12, 2021

Got it, but isn't that significantly different from the original issue request? The run-clang-format.sh script is entirely user-driven. Format-on-save or IDE linting are orthogonal to that. I was suggesting pre-commit as a drop-in replacement for run-clang-format.sh (and for a potential run-cmake-format.sh). Being able to manually pre-commit run ... at any time is IMO a good substitute for maintaining custom wrapper scripts around linters. It also has the extra benefit of allowing you to set it up to run when you commit, which is nice, but preferred run frequency for linters is somewhat subjective in any case so I wouldn't want to impose any particular approach on all developers.

@harrism
Copy link
Member Author

harrism commented Oct 12, 2021

Yeah, I wasn't asking for save-on-format because I don't know if that plugin is available for my editor (vscode). What I'm asking for is to make it easy to reproduce CI failures locally and exactly.

As I understand it, this is pretty easy to do, we just have to include the cmake format configuration in the repo, or make the script download it when run.

@harrism
Copy link
Member Author

harrism commented Oct 12, 2021

For more clarity (after talking to Vyas offline), in this issue I'm purely asking for the prerequisites to run cmake-format locally. Specifically, that means that the cmake-format-rapids-cmake.json configuration file be available as part of the repo.

Now as I'm typing this, I'm noticing that that file is installed into build/_deps/rapids-cmake-src when you build locally. So perhaps the prerequisites are already here.

@robertmaynard
Copy link
Contributor

For more clarity (after talking to Vyas offline), in this issue I'm purely asking for the prerequisites to run cmake-format locally. Specifically, that means that the cmake-format-rapids-cmake.json configuration file be available as part of the repo.

Now as I'm typing this, I'm noticing that that file is installed into build/_deps/rapids-cmake-src when you build locally. So perhaps the prerequisites are already here.

Yes for developers the requisite files will exist once a build directory exists. CI runs the formatting pass without generating a build directory and therefore needs to fetch the file from the internet explicitly.

@harrism
Copy link
Member Author

harrism commented Oct 13, 2021

OK. In spite of this, a developer has to run cmake-format (and cmake-lint) on each CMakeLists.txt in the repo, and the commands are not short, e.g. the following for each file.

cmake-format --in-place --config-files cmake/config.json build/release/_deps/rapids-cmake-src/cmake-format-rapids-cmake.json -- CMakeLists.txt 

So I think we should provide a script for this in the repo, or a way to do it with pre-commit (which I only just learned about from @vyasr yesterday).

rapids-bot bot pushed a commit that referenced this issue Oct 29, 2021
This PR resolves #9396, making it easy to run `cmake-format` and `cmake-lint` on CMake files in cudf using pre-commit. The one additional complexity associated with these hooks compared to our other pre-commit hooks is the need for the `cmake-format-rapids-cmake.json` configuration file that is maintained in [rapids-cmake](https://github.com/rapidsai/rapids-cmake) and is only added during the build process. We don't want pre-commit to fail outright when the file isn't present, so this PR introduces a custom script wrapping the `cmake-format` and `cmake-lint` hooks in a check for the presence of the file. The script respects an associated environment variable that advanced users can exploit if they store this file in a nonstandard location.

Once this PR is finalized I'll make a follow-up PR that actually applies the formatting to our existing CMake.

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

Approvers:
  - Mark Harris (https://github.com/harrism)
  - Robert Maynard (https://github.com/robertmaynard)

URL: #9484
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CMake CMake build issue feature request New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants