From d4fa8b20c61d218fa6e7da478f3da2f340fbfd93 Mon Sep 17 00:00:00 2001 From: LinZhihao-723 Date: Sun, 1 Dec 2024 22:21:39 -0500 Subject: [PATCH 1/5] Enable clang-tidy --- README.md | 25 +++++++++++++------------ lint-tasks.yml | 15 ++++++++------- 2 files changed, 21 insertions(+), 19 deletions(-) diff --git a/README.md b/README.md index c7bf189..7832a88 100644 --- a/README.md +++ b/README.md @@ -312,18 +312,19 @@ The commands above run all linting checks, but for performance you may want to r if you only changed C++ files, you don't need to run the YAML linting checks) using one of the tasks in the table below. -| Task | Description | -|-------------------------|----------------------------------------------------------| -| `lint:cmake-check` | Runs the CMake linters. | -| `lint:cmake-fix` | Runs the CMake linters and fixes any violations. | -| `lint:cpp-check` | Runs the C++ linters (formatters and static analyzers). | -| `lint:cpp-fix` | Runs the C++ linters and fixes some violations. | -| `lint:cpp-format-check` | Runs the C++ formatters. | -| `lint:cpp-format-fix` | Runs the C++ formatters and fixes some violations. | -| `lint:py-check` | Runs the Python linters. | -| `lint:py-fix` | Runs the Python linters and fixes some violations. | -| `lint:yml-check` | Runs the YAML linters. | -| `lint:yml-fix` | Runs the YAML linters and fixes some violations. | +| Task | Description | +|-------------------------|---------------------------------------------------------| +| `lint:cmake-check` | Runs the CMake linters. | +| `lint:cmake-fix` | Runs the CMake linters and fixes any violations. | +| `lint:cpp-check` | Runs the C++ linters (formatters and static analyzers). | +| `lint:cpp-fix` | Runs the C++ linters and fixes some violations. | +| `lint:cpp-format-check` | Runs the C++ formatters. | +| `lint:cpp-format-fix` | Runs the C++ formatters and fixes some violations. | +| `lint:cpp-static-check` | Runs the C++ linters. | +| `lint:py-check` | Runs the Python linters. | +| `lint:py-fix` | Runs the Python linters and fixes some violations. | +| `lint:yml-check` | Runs the YAML linters. | +| `lint:yml-fix` | Runs the YAML linters and fixes some violations. | [1]: https://github.com/y-scope/clp/tree/main/components/core [2]: https://github.com/y-scope/clp diff --git a/lint-tasks.yml b/lint-tasks.yml index 362bee3..9c78437 100644 --- a/lint-tasks.yml +++ b/lint-tasks.yml @@ -4,7 +4,12 @@ vars: G_LINT_VENV_DIR: "{{.BUILD_DIR}}/lint-venv" G_LINT_VENV_CHECKSUM_FILE: "{{.BUILD_DIR}}/lint#venv.md5" # Linter target dirs - G_CPP_LINT_DIRS: ["{{.CLP_FFI_PY_CPP_SRC_DIR}}"] + G_CPP_LINT_DIRS: + # TODO: before all clang-tidy warnings are resolved, we should maintain a list of files that + # doesn't have clang-tidy warnings, so that clang-tidy can be executed in the limited scope + # without failing existing workflows. + - "{{.CLP_FFI_PY_CPP_SRC_DIR}}/Py_utils.cpp" + - "{{.CLP_FFI_PY_CPP_SRC_DIR}}/Py_utils.hpp" G_PYTHON_LINT_DIRS: ["{{.ROOT_DIR}}/clp_ffi_py", "{{.ROOT_DIR}}/tests"] tasks: @@ -37,22 +42,18 @@ tasks: FLAGS: "--in-place" cpp-configs: - # TODO: remove this once all clang-tidy has been fixed - deps: [":config-cmake-project"] cmds: - "tools/yscope-dev-utils/lint-configs/symlink-cpp-lint-configs.sh" cpp-check: cmds: - task: "cpp-format-check" - # TODO: re-enable this once all clang-tidy has been fixed - # - task: "cpp-static-check" + - task: "cpp-static-check" cpp-fix: cmds: - task: "cpp-format-fix" - # TODO: re-enable this once all clang-tidy has been fixed - # - task: "cpp-static-fix" + - task: "cpp-static-fix" cpp-format-check: sources: &cpp_format_src_files From 7f920a6d4afed6fa735b1263b6042425971fa619 Mon Sep 17 00:00:00 2001 From: LinZhihao-723 Date: Sun, 1 Dec 2024 22:28:37 -0500 Subject: [PATCH 2/5] Use Python3.10 instead --- .github/workflows/build_wheels.yml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.github/workflows/build_wheels.yml b/.github/workflows/build_wheels.yml index a4c978c..2d6224e 100644 --- a/.github/workflows/build_wheels.yml +++ b/.github/workflows/build_wheels.yml @@ -42,7 +42,8 @@ jobs: - uses: "actions/setup-python@v5" with: - python-version: "3.11" + # NOTE: CPython3.10 headers are used to resolve clang-tidy warnings + python-version: "3.10" - run: | pip install --upgrade pip From a92a0f38956c3e98cdeaf64e5aad400b8e5b4adb Mon Sep 17 00:00:00 2001 From: Lin Zhihao <59785146+LinZhihao-723@users.noreply.github.com> Date: Mon, 2 Dec 2024 00:04:01 -0500 Subject: [PATCH 3/5] Apply suggestions from code review Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com> --- .github/workflows/build_wheels.yml | 3 ++- README.md | 2 +- lint-tasks.yml | 6 +++--- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/.github/workflows/build_wheels.yml b/.github/workflows/build_wheels.yml index 2d6224e..6d1713d 100644 --- a/.github/workflows/build_wheels.yml +++ b/.github/workflows/build_wheels.yml @@ -42,7 +42,8 @@ jobs: - uses: "actions/setup-python@v5" with: - # NOTE: CPython3.10 headers are used to resolve clang-tidy warnings + # NOTE: We resolve some of clang-tidy's IWYU violations using CPython 3.10's headers, so + # we need to use the same version of Python when running clang-tidy. python-version: "3.10" - run: | diff --git a/README.md b/README.md index 7832a88..374cdec 100644 --- a/README.md +++ b/README.md @@ -320,7 +320,7 @@ in the table below. | `lint:cpp-fix` | Runs the C++ linters and fixes some violations. | | `lint:cpp-format-check` | Runs the C++ formatters. | | `lint:cpp-format-fix` | Runs the C++ formatters and fixes some violations. | -| `lint:cpp-static-check` | Runs the C++ linters. | +| `lint:cpp-static-check` | Runs the C++ static analyzers. | | `lint:py-check` | Runs the Python linters. | | `lint:py-fix` | Runs the Python linters and fixes some violations. | | `lint:yml-check` | Runs the YAML linters. | diff --git a/lint-tasks.yml b/lint-tasks.yml index 9c78437..1d70854 100644 --- a/lint-tasks.yml +++ b/lint-tasks.yml @@ -5,9 +5,9 @@ vars: G_LINT_VENV_CHECKSUM_FILE: "{{.BUILD_DIR}}/lint#venv.md5" # Linter target dirs G_CPP_LINT_DIRS: - # TODO: before all clang-tidy warnings are resolved, we should maintain a list of files that - # doesn't have clang-tidy warnings, so that clang-tidy can be executed in the limited scope - # without failing existing workflows. + # TODO: Before all clang-tidy violations are resolved, we should only run clang-tidy on the + # files whose violations we've fixed, both to ensure they remain free of violations and so that + # the workflow doesn't fail due to violations in other files. - "{{.CLP_FFI_PY_CPP_SRC_DIR}}/Py_utils.cpp" - "{{.CLP_FFI_PY_CPP_SRC_DIR}}/Py_utils.hpp" G_PYTHON_LINT_DIRS: ["{{.ROOT_DIR}}/clp_ffi_py", "{{.ROOT_DIR}}/tests"] From 092296239554172a7c1659e5778558fd1ce2764e Mon Sep 17 00:00:00 2001 From: LinZhihao-723 Date: Mon, 2 Dec 2024 00:07:30 -0500 Subject: [PATCH 4/5] Fix linter source variable --- lint-tasks.yml | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lint-tasks.yml b/lint-tasks.yml index 1d70854..8201d57 100644 --- a/lint-tasks.yml +++ b/lint-tasks.yml @@ -4,7 +4,8 @@ vars: G_LINT_VENV_DIR: "{{.BUILD_DIR}}/lint-venv" G_LINT_VENV_CHECKSUM_FILE: "{{.BUILD_DIR}}/lint#venv.md5" # Linter target dirs - G_CPP_LINT_DIRS: + G_CPP_LINT_DIRS: ["{{.CLP_FFI_PY_CPP_SRC_DIR}}"] + G_CPP_STATIC_CHECK_SRC_FILES: # TODO: Before all clang-tidy violations are resolved, we should only run clang-tidy on the # files whose violations we've fixed, both to ensure they remain free of violations and so that # the workflow doesn't fail due to violations in other files. @@ -108,7 +109,7 @@ tasks: --config-file "{{.CLP_FFI_PY_CPP_SRC_DIR}}/.clang-tidy" -p "{{.CLP_FFI_PY_COMPILE_COMMANDS_DB}}" SRC_PATHS: - ref: ".G_CPP_LINT_DIRS" + ref: ".G_CPP_STATIC_CHECK_SRC_FILES" VENV_DIR: "{{.G_LINT_VENV_DIR}}" py-check: From 3ad685bc20649efcce2fc840d2dc84e367b40f07 Mon Sep 17 00:00:00 2001 From: LinZhihao-723 Date: Mon, 2 Dec 2024 00:37:08 -0500 Subject: [PATCH 5/5] Don't use a global variable --- lint-tasks.yml | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/lint-tasks.yml b/lint-tasks.yml index 8201d57..c194545 100644 --- a/lint-tasks.yml +++ b/lint-tasks.yml @@ -5,12 +5,6 @@ vars: G_LINT_VENV_CHECKSUM_FILE: "{{.BUILD_DIR}}/lint#venv.md5" # Linter target dirs G_CPP_LINT_DIRS: ["{{.CLP_FFI_PY_CPP_SRC_DIR}}"] - G_CPP_STATIC_CHECK_SRC_FILES: - # TODO: Before all clang-tidy violations are resolved, we should only run clang-tidy on the - # files whose violations we've fixed, both to ensure they remain free of violations and so that - # the workflow doesn't fail due to violations in other files. - - "{{.CLP_FFI_PY_CPP_SRC_DIR}}/Py_utils.cpp" - - "{{.CLP_FFI_PY_CPP_SRC_DIR}}/Py_utils.hpp" G_PYTHON_LINT_DIRS: ["{{.ROOT_DIR}}/clp_ffi_py", "{{.ROOT_DIR}}/tests"] tasks: @@ -109,7 +103,11 @@ tasks: --config-file "{{.CLP_FFI_PY_CPP_SRC_DIR}}/.clang-tidy" -p "{{.CLP_FFI_PY_COMPILE_COMMANDS_DB}}" SRC_PATHS: - ref: ".G_CPP_STATIC_CHECK_SRC_FILES" + # TODO: Before all clang-tidy violations are resolved, we should only run clang-tidy on + # the files whose violations we've fixed, both to ensure they remain free of violations + # and so that the workflow doesn't fail due to violations in other files. + - "{{.CLP_FFI_PY_CPP_SRC_DIR}}/Py_utils.cpp" + - "{{.CLP_FFI_PY_CPP_SRC_DIR}}/Py_utils.hpp" VENV_DIR: "{{.G_LINT_VENV_DIR}}" py-check: