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.

Sign up for GitHub

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

Add tasks to run C++ static analysis using clang-tidy; Separate CMake project config into its own task. #18

Merged
merged 7 commits into from
Oct 7, 2024
Merged

Add tasks to run C++ static analysis using clang-tidy; Separate CMake project config into its own task. #18

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d884cc9
Add tasks to run C++ static analysis using clang-tidy.
kirkrodrigues Oct 5, 2024
6a471a2
Activate venv before calling clang-tidy.
kirkrodrigues Oct 5, 2024
10e4764
Fix typo.
kirkrodrigues Oct 5, 2024
4db65bc
Move clang-format and clang-tidy dependencies into caller tasks.
kirkrodrigues Oct 6, 2024
0f76926
Remove run: once where not strictly necessary.
kirkrodrigues Oct 6, 2024
f9ed9ac
Move config-cmake-project to internal tasks section.
kirkrodrigues Oct 6, 2024
b9fa11b
Merge branch 'main' into add-clang-tidy
kirkrodrigues Oct 6, 2024
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
16 changes: 16 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,22 @@ To run all linting checks AND automatically fix any fixable issues:
task lint:fix
```

### Running specific linters
The commands above run all linting checks, but for performance you may want to run a subset (e.g.,
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: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++ static analyzers. |
| `lint:cpp-static-fix` | Runs the C++ static analyzers and fixes some violations. |
| `lint:yml-check` | Runs the YAML linters. |
| `lint:yml-fix` | Runs the YAML linters and fixes some violations. |

[bug-report]: https://github.com/y-scope/clp-ffi-js/issues/new?labels=bug&template=bug-report.yml
[CLP]: https://github.com/y-scope/clp
[emscripten]: https://emscripten.org
Expand Down
29 changes: 21 additions & 8 deletions Taskfile.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ tasks:
CHECKSUM_FILE: "{{.G_CLP_FFI_JS_CHECKSUM}}"
OUTPUT_DIR: "{{.G_CLP_FFI_JS_BUILD_DIR}}"
sources:
- "{{.G_CLP_FFI_JS_BUILD_DIR}}/CMakeCache.txt"
- "{{.G_EMSDK_CHECKSUM}}"
- "{{.TASKFILE}}"
- "CMakeLists.txt"
Expand All @@ -48,14 +49,7 @@ tasks:
CHECKSUM_FILE: "{{.CHECKSUM_FILE}}"
DATA_DIR: "{{.OUTPUT_DIR}}"
cmds:
- "mkdir -p '{{.OUTPUT_DIR}}'"
- |-
cmake \
-G "Unix Makefiles" \
-DCMAKE_TOOLCHAIN_FILE="{{.G_EMSDK_DIR}}/upstream/emscripten/cmake/Modules/Platform/\
Emscripten.cmake" \
-S "{{.ROOT_DIR}}" \
-B "{{.OUTPUT_DIR}}"
- task: "config-cmake-project"
- "cmake --build '{{.OUTPUT_DIR}}' --parallel --target '{{.G_CLP_FFI_JS_TARGET_NAME}}'"
# This command must be last
- task: "utils:compute-checksum"
Expand All @@ -71,6 +65,7 @@ tasks:
OUTPUT_DIR: "{{.G_EMSDK_DIR}}"
sources: ["{{.TASKFILE}}"]
generates: ["{{.CHECKSUM_FILE}}"]
run: "once"
deps:
- "init"
- task: "utils:validate-checksum"
Expand Down Expand Up @@ -131,6 +126,24 @@ tasks:
DATA_DIR: "{{.OUTPUT_DIR}}"
OUTPUT_FILE: "{{.CHECKSUM_FILE}}"

config-cmake-project:
internal: true
sources:
- "{{.G_EMSDK_CHECKSUM}}"
- "{{.TASKFILE}}"
- "CMakeLists.txt"
generates:
- "{{.G_CLP_FFI_JS_BUILD_DIR}}/CMakeCache.txt"
- "{{.G_CLP_FFI_JS_BUILD_DIR}}/compile_commands.json"
deps: ["emsdk"]
cmd: |-
cmake \
-G "Unix Makefiles" \
-DCMAKE_TOOLCHAIN_FILE="{{.G_EMSDK_DIR}}/upstream/emscripten/cmake/Modules/Platform/\
Emscripten.cmake" \
-S "{{.ROOT_DIR}}" \
-B "{{.G_CLP_FFI_JS_BUILD_DIR}}"

init:
internal: true
silent: true
Expand Down
1 change: 1 addition & 0 deletions lint-requirements.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
# Lock to v18.x until we can upgrade our code to meet v19's formatting standards.
clang-format~=18.1
clang-tidy>=19.1.0
yamllint>=1.35.1
82 changes: 69 additions & 13 deletions lint-tasks.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
version: "3"

vars:
G_SRC_CLP_FFI_JS_DIR: "{{.ROOT_DIR}}/src/clp_ffi_js"
G_SRC_DIR: "{{.ROOT_DIR}}/src"
G_SRC_CLP_FFI_JS_DIR: "{{.G_SRC_DIR}}/clp_ffi_js"
G_LINT_VENV_DIR: "{{.G_BUILD_DIR}}/lint-venv"

tasks:
Expand All @@ -19,25 +20,62 @@ tasks:
cmd: "{{.ROOT_DIR}}/tools/yscope-dev-utils/lint-configs/symlink-cpp-lint-configs.sh"

cpp-check:
sources: &cpp_source_files
cmds:
- task: "cpp-format-check"
- task: "cpp-static-check"

cpp-fix:
cmds:
- task: "cpp-format-fix"
- task: "cpp-static-fix"

cpp-format-check:
sources: &cpp_format_source_files
- "{{.G_SRC_CLP_FFI_JS_DIR}}/.clang-format"
- "{{.G_SRC_CLP_FFI_JS_DIR}}/**/*.cpp"
- "{{.G_SRC_CLP_FFI_JS_DIR}}/**/*.h"
- "{{.G_SRC_CLP_FFI_JS_DIR}}/**/*.hpp"
- "{{.TASKFILE}}"
- "tools/yscope-dev-utils/lint-configs/.clang-format"
deps: ["cpp-configs", "venv"]
cmds:
- task: "clang-format"
vars:
FLAGS: "--dry-run"

cpp-fix:
sources: *cpp_source_files
cpp-format-fix:
sources: *cpp_format_source_files
deps: ["cpp-configs", "venv"]
cmds:
- task: "clang-format"
vars:
FLAGS: "-i"

cpp-static-check:
# Alias task to `cpp-static-fix` since we don't currently support automatic fixes.
# NOTE: clang-tidy does have the ability to fix some errors, but the fixes can be inaccurate.
# When we eventually determine which errors can be safely fixed, we'll allow clang-tidy to
# fix them.
aliases: ["cpp-static-fix"]
sources:
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to add G_EMSDK_CHECKSUM here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, my mistake. Let me restructure things to be clearer.

Copy link
Member Author

Choose a reason for hiding this comment

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

Since we plan to move clang-format and clang-tidy into yscope-dev-utils, what do you think about the following? Move clang-format and clang-tidy's dependencies into cpp-format-* and cpp-static-* and make clang-format and clang-tidy idempotent.

# Dependency sources
- "{{.G_CLP_FFI_JS_BUILD_DIR}}/_deps/*-src/**/*"
- "{{.G_SRC_DIR}}/submodules/**/*"

# clp-ffi-js sources
- "{{.G_SRC_CLP_FFI_JS_DIR}}/**/*.cpp"
- "{{.G_SRC_CLP_FFI_JS_DIR}}/**/*.h"
- "{{.G_SRC_CLP_FFI_JS_DIR}}/**/*.hpp"
Comment on lines +66 to +68
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to include all source files? Say if some header in clp-ffi-js' dependency changes, it would also impact the compilation in clp-ffi-js' source code, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

It would, but this task doesn't require compiling clp-ffi-js, right? (I might be misunderstanding your question.)

Copy link
Member

@junhaoliao junhaoliao Oct 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I can't come up with an exact example since I'm not too familiar with clang-tidy rules. However, from my experience working with static analysis, any dependency changes can cause the checks in our code to fail. (In fact, sometimes it's easier to fix violations in the dependencies when there are too many violations due to a same issue in the dependency. )

Please correct me if this imaginary example is impossible - say we recently updated some function signature in CLP core code that some parameter changes its type from int64 to int32, but our clp-ffi-js code remains the same. If we keep passing an int64 value to the modified function, wouldn't a precision / value loss violation be raised?

If we don't include the header, which is used in static checks in the sources list here, the cpp-static-check would not run at all if I'm not wrong.

Copy link
Member Author

@kirkrodrigues kirkrodrigues Oct 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Oh I see what you're saying. Yes, we should include the dependencies' source files.


# Other sources
- "{{.G_CLP_FFI_JS_BUILD_DIR}}/compile_commands.json"
- "{{.ROOT_DIR}}/Taskfile.yml"
- "{{.TASKFILE}}"
- "tools/yscope-dev-utils/lint-configs/.clang-tidy"
deps: [":config-cmake-project", "cpp-configs", "venv"]
cmds:
- task: "clang-tidy"

yml:
aliases:
- "yml-check"
Expand All @@ -57,15 +95,33 @@ tasks:
internal: true
requires:
vars: ["FLAGS"]
deps: ["cpp-configs", "venv"]
cmds:
- |-
. "{{.G_LINT_VENV_DIR}}/bin/activate"
find "{{.G_SRC_CLP_FFI_JS_DIR}}" \
-type f \
\( -iname "*.cpp" -o -iname "*.h" -o -iname "*.hpp" \) \
-print0 | \
xargs -0 clang-format {{.FLAGS}} -Werror
cmd: |-
. "{{.G_LINT_VENV_DIR}}/bin/activate"
find "{{.G_SRC_CLP_FFI_JS_DIR}}" \
-type f \
\( -iname "*.cpp" -o -iname "*.h" -o -iname "*.hpp" \) \
-print0 | \
xargs -0 clang-format {{.FLAGS}} -Werror
Comment on lines +98 to +104
Copy link
Member Author

@kirkrodrigues kirkrodrigues Oct 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Unrelated to the PR, but this is simply to reformat from cmds into cmd.


clang-tidy:
internal: true
vars:
FLAGS: >-
-p {{.G_CLP_FFI_JS_BUILD_DIR}}/compile_commands.json
--config-file=.clang-tidy
cmd: |-
. "{{.G_LINT_VENV_DIR}}/bin/activate"

# Pass emscripten's cflags to clang-tidy by prefixing each one with `--extra-arg`
EXTRA_ARGS=$("{{.G_EMSDK_DIR}}/upstream/emscripten/em++" --cflags \
| tr ' ' '\n' \
| sed 's/^/--extra-arg /' \
| tr '\n' ' ')
find "{{.G_SRC_CLP_FFI_JS_DIR}}" \
-type f \
\( -iname "*.cpp" -o -iname "*.h" -o -iname "*.hpp" \) \
-print0 | \
xargs -0 clang-tidy {{.FLAGS}} $EXTRA_ARGS

venv:
internal: true
Expand Down