Skip to content

chore: Add Taskfile tasks to lint C++ files. #9

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

Merged
kirkrodrigues merged 2 commits into from
Oct 24, 2024
Merged

chore: Add Taskfile tasks to lint C++ files. #9

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a6bb121
chore: Add Taskfile tasks to lint C++ files.
kirkrodrigues Oct 24, 2024
b8d96d6
Add setup step for C++ linting configs.
kirkrodrigues Oct 24, 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
4 changes: 4 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,9 @@
.task
build

# Generated lint configs
.clang-format
.clang-tidy

# IDE-related directories and files
.idea
23 changes: 17 additions & 6 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,11 @@ Initialize and update submodules:
git submodule update --init --recursive
```

Set up the config files for our C++ linting tools:
```shell
task lint:cpp-configs
```

## Adding files
Certain file types need to be added to our linting rules manually:

Expand All @@ -36,11 +41,17 @@ 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: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++ 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. |

[Task]: https://taskfile.dev
3 changes: 3 additions & 0 deletions lint-requirements.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,5 @@
# 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
gersemi>=0.16.2
yamllint>=1.35.1
89 changes: 88 additions & 1 deletion lint-tasks.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,19 @@ version: "3"

vars:
G_LINT_VENV_DIR: "{{.G_BUILD_DIR}}/lint-venv"
G_LINT_VENV_CHECKSUM_FILE: "{{.G_BUILD_DIR}}/lint#venv.md5"

tasks:
check:
cmds:
- task: "cmake-check"
- task: "cpp-check"
- task: "yml-check"

fix:
cmds:
- task: "cmake-fix"
- task: "cpp-fix"
- task: "yml-fix"

cmake-check:
Expand All @@ -28,6 +31,66 @@ tasks:
vars:
FLAGS: "--in-place"

cpp-configs: "tools/yscope-dev-utils/lint-configs/symlink-cpp-lint-configs.sh"

Comment on lines +34 to +35

@coderabbitai coderabbitai Bot Oct 24, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Missing script file referenced in lint-tasks.yaml

The script tools/yscope-dev-utils/lint-configs/symlink-cpp-lint-configs.sh referenced in the lint-tasks.yaml file does not exist in the repository. Our verification shows that:

  • The script is not present at the specified path
  • No alternative files with similar names exist in the tools directory
  • The directory structure tools/yscope-dev-utils/lint-configs/ is not present

This will cause the cpp-configs task to fail when executed.

🔗 Analysis chain

Verify the existence of the symlink script

The task references an external script. Let's verify its existence and permissions.

Let me verify if the script exists in a different location or if there are any similar files by searching the repository.

Let me check the content of the yscope-dev-utils directory to see if there's any alternative structure or if the script might be named differently.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Verify the symlink script exists and is executable
if [ -x "tools/yscope-dev-utils/lint-configs/symlink-cpp-lint-configs.sh" ]; then
  echo "Script exists and is executable"
else
  echo "Script not found or not executable"
fi

Length of output: 155

Script:

#!/bin/bash
# Search for the script file or similar files
fd "symlink-cpp-lint-configs.sh"

# Search for any cpp lint config related files
fd -t f ".*cpp.*lint.*config.*"

# Check the tools directory structure
fd . tools -t d

Length of output: 108

Script:

#!/bin/bash
# List contents of the tools/yscope-dev-utils directory recursively
ls -R tools/yscope-dev-utils/

# Search for any shell scripts in the tools directory
fd -e sh . tools/

# Check if there are any lint-related files in the tools directory
fd "lint" tools/

Length of output: 93

cpp-check:
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_src_files
- "{{.G_LINT_VENV_CHECKSUM_FILE}}"
- "{{.G_SRC_SPIDER_DIR}}/.clang-format"
- "{{.G_SRC_SPIDER_DIR}}/**/*.cpp"
- "{{.G_SRC_SPIDER_DIR}}/**/*.h"
- "{{.G_SRC_SPIDER_DIR}}/**/*.hpp"
- "{{.TASKFILE}}"
- "tools/yscope-dev-utils/lint-configs/.clang-format"
deps: ["cpp-configs", "venv"]
cmds:
- task: "clang-format"
vars:
FLAGS: "--dry-run"
SRC_DIR: "{{.G_SRC_SPIDER_DIR}}"

cpp-format-fix:
sources: *cpp_format_src_files
deps: ["cpp-configs", "venv"]
cmds:
- task: "clang-format"
vars:
FLAGS: "-i"
SRC_DIR: "{{.G_SRC_SPIDER_DIR}}"

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:
- "{{.G_LINT_VENV_CHECKSUM_FILE}}"
- "{{.G_SRC_SPIDER_DIR}}/**/*.cpp"
- "{{.G_SRC_SPIDER_DIR}}/**/*.h"
- "{{.G_SRC_SPIDER_DIR}}/**/*.hpp"
- "{{.G_SPIDER_CMAKE_CACHE}}"
- "{{.G_SPIDER_COMPILE_COMMANDS_DB}}"
- "{{.TASKFILE}}"
- "Taskfile.yml"
- "tools/yscope-dev-utils/lint-configs/.clang-tidy"
deps: [":config-cmake-project", "cpp-configs", "venv"]
cmds:
- task: "clang-tidy"
vars:
FLAGS: "--config-file=.clang-tidy -p {{.G_SPIDER_COMPILE_COMMANDS_DB}}"
SRC_DIR: "{{.G_SRC_SPIDER_DIR}}"

yml:
aliases:
- "yml-check"
Expand All @@ -45,6 +108,30 @@ tasks:
lint-tasks.yaml \
taskfile.yaml

clang-format:
internal: true
requires:
vars: ["FLAGS", "SRC_DIR"]
cmd: |-
. "{{.G_LINT_VENV_DIR}}/bin/activate"
find "{{.SRC_DIR}}" \
-type f \
\( -iname "*.cpp" -o -iname "*.h" -o -iname "*.hpp" \) \
-print0 | \
xargs -0 --no-run-if-empty clang-format {{.FLAGS}} -Werror

clang-tidy:
internal: true
requires:
vars: ["FLAGS", "SRC_DIR"]
cmd: |-
. "{{.G_LINT_VENV_DIR}}/bin/activate"
find "{{.SRC_DIR}}" \
-type f \
\( -iname "*.cpp" -o -iname "*.h" -o -iname "*.hpp" \) \
-print0 | \
xargs -0 --no-run-if-empty clang-tidy {{.FLAGS}}

Comment on lines +111 to +134

@coderabbitai coderabbitai Bot Oct 24, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider adding parallel execution support

The implementation is solid, but for large codebases, parallel execution could improve performance.

Add parallel execution support to clang-format and clang-tidy:

-          xargs -0 --no-run-if-empty clang-format {{.FLAGS}} -Werror
+          xargs -0 --no-run-if-empty -P $(nproc) clang-format {{.FLAGS}} -Werror
-          xargs -0 --no-run-if-empty clang-tidy {{.FLAGS}}
+          xargs -0 --no-run-if-empty -P $(nproc) clang-tidy {{.FLAGS}}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
clang-format:
internal: true
requires:
vars: ["FLAGS", "SRC_DIR"]
cmd: |-
. "{{.G_LINT_VENV_DIR}}/bin/activate"
find "{{.SRC_DIR}}" \
-type f \
\( -iname "*.cpp" -o -iname "*.h" -o -iname "*.hpp" \) \
-print0 | \
xargs -0 --no-run-if-empty clang-format {{.FLAGS}} -Werror
clang-tidy:
internal: true
requires:
vars: ["FLAGS", "SRC_DIR"]
cmd: |-
. "{{.G_LINT_VENV_DIR}}/bin/activate"
find "{{.SRC_DIR}}" \
-type f \
\( -iname "*.cpp" -o -iname "*.h" -o -iname "*.hpp" \) \
-print0 | \
xargs -0 --no-run-if-empty clang-tidy {{.FLAGS}}
clang-format:
internal: true
requires:
vars: ["FLAGS", "SRC_DIR"]
cmd: |-
. "{{.G_LINT_VENV_DIR}}/bin/activate"
find "{{.SRC_DIR}}" \
-type f \
\( -iname "*.cpp" -o -iname "*.h" -o -iname "*.hpp" \) \
-print0 | \
xargs -0 --no-run-if-empty -P $(nproc) clang-format {{.FLAGS}} -Werror
clang-tidy:
internal: true
requires:
vars: ["FLAGS", "SRC_DIR"]
cmd: |-
. "{{.G_LINT_VENV_DIR}}/bin/activate"
find "{{.SRC_DIR}}" \
-type f \
\( -iname "*.cpp" -o -iname "*.h" -o -iname "*.hpp" \) \
-print0 | \
xargs -0 --no-run-if-empty -P $(nproc) clang-tidy {{.FLAGS}}

cmake:
internal: true
requires:
Expand All @@ -60,7 +147,7 @@ tasks:
venv:
internal: true
vars:
CHECKSUM_FILE: "{{.G_BUILD_DIR}}/{{.TASK | replace \":\" \"#\"}}.md5"
CHECKSUM_FILE: "{{.G_LINT_VENV_CHECKSUM_FILE}}"
OUTPUT_DIR: "{{.G_LINT_VENV_DIR}}"
sources:
- "{{.ROOT_DIR}}/taskfile.yaml"
Expand Down
20 changes: 20 additions & 0 deletions src/spider/.clang-format
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
BasedOnStyle: "InheritParentConfig"

IncludeCategories:
# NOTE: A header is grouped by first matching regex
# Project headers
- Regex: "^<spider"
Priority: 4
# Library headers. Update when adding new libraries.
# NOTE: clang-format retains leading white-space on a line in violation of the YAML spec.
# Ex:
# - Regex: "<(fmt|spdlog)"
# Priority: 3
- Regex: "^<(clp)"
Priority: 3
# C system headers
- Regex: "^<.+\\.h>"
Priority: 1
# C++ standard libraries
- Regex: "^<.+>"
Priority: 2
4 changes: 2 additions & 2 deletions src/spider/spider.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
#include <iostream>

int main() {
std::cout << "Hello, world!" << std::endl;
auto main() -> int {
std::cout << "Hello, world!" << '\n';
return 0;
}
14 changes: 14 additions & 0 deletions taskfile.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,26 @@ includes:

vars:
G_BUILD_DIR: "{{.ROOT_DIR}}/build"
G_BUILD_SPIDER_DIR: "{{.G_BUILD_DIR}}/spider"
G_SPIDER_CMAKE_CACHE: "{{.G_BUILD_SPIDER_DIR}}/CMakeCache.txt"
G_SPIDER_COMPILE_COMMANDS_DB: "{{.G_BUILD_SPIDER_DIR}}/compile_commands.json"
G_SRC_SPIDER_DIR: "{{.ROOT_DIR}}/src/spider"

tasks:
clean:
cmds:
- "rm -rf '{{.G_BUILD_DIR}}'"

config-cmake-project:
internal: true
sources:
- "{{.TASKFILE}}"
- "CMakeLists.txt"
generates:
- "{{.G_SPIDER_CMAKE_CACHE}}"
- "{{.G_SPIDER_COMPILE_COMMANDS_DB}}"
cmd: "cmake -S '{{.ROOT_DIR}}' -B '{{.G_BUILD_SPIDER_DIR}}'"
Comment on lines +19 to +27

@coderabbitai coderabbitai Bot Oct 24, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance task robustness with error handling and prerequisites.

The task configuration looks good, but could benefit from additional error handling and prerequisite checks.

Consider these improvements:

   config-cmake-project:
     internal: true
+    deps:
+      - task: verify-cmake
     sources:
       - "{{.TASKFILE}}"
       - "CMakeLists.txt"
     generates:
       - "{{.G_SPIDER_CMAKE_CACHE}}"
       - "{{.G_SPIDER_COMPILE_COMMANDS_DB}}"
-    cmd: "cmake -S '{{.ROOT_DIR}}' -B '{{.G_BUILD_SPIDER_DIR}}'"
+    cmds:
+      - |
+        if ! command -v cmake >/dev/null 2>&1; then
+          echo "Error: cmake is not installed" >&2
+          exit 1
+        fi
+      - "cmake -S '{{.ROOT_DIR}}' -B '{{.G_BUILD_SPIDER_DIR}}'"

Also consider adding a new task to verify cmake installation:

  verify-cmake:
    internal: true
    cmds:
      - |
        if ! command -v cmake >/dev/null 2>&1; then
          echo "Please install cmake to proceed" >&2
          exit 1
        fi


init:
internal: true
silent: true
Expand Down