rapidsai / cudf

cuDF - GPU DataFrame Library
https://docs.rapids.ai/api/cudf/stable/
Apache License 2.0
8.39k stars 897 forks source link

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

Closed harrism closed 2 years ago

harrism commented 3 years ago

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

vyasr commented 3 years ago

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

harrism commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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).