-
Notifications
You must be signed in to change notification settings - Fork 7
build: Migrate to task-based dependency management: #115
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments.
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (5)
.github/workflows/build.yaml (2)
45-48
: Consider caching the Task CLI to speed up workflow runs
Installing@go-task/cli
on every run can add latency. You could cache the npm package directory:- - name: "Install task" - shell: "bash" - run: "npm install -g @go-task/cli" + - name: "Install task" + shell: bash + run: | + npm ci -g --cache "${{ runner.temp }}/npm-cache" @go-task/cli + - name: "Cache Task CLI" + uses: actions/cache@v3 + with: + path: ${{ runner.temp }}/npm-cache + key: ${{ runner.os }}-task-cli-${{ hashFiles('taskfile.yaml') }}
56-60
: Run all example binaries
Currently onlyintersect-test
is run. If there are additional examples, consider iterating through all built binaries to validate them. For example:for bin in ./build/examples/${{matrix.build_type}}/*; do "$bin" doneREADME.md (3)
24-24
: Add language identifier to fenced code block
The code block starting at line 24 lacks a language specifier. For better syntax highlighting, specify a language (e.g.text
):- ``` + ```text
98-103
: Use language-specific fenced code blocks
Multiple code blocks for shell commands are missing a language identifier. Addshell
after the backticks:- ``` + ```shellThis improves readability and syntax highlighting throughout the guide.
Also applies to: 109-111, 126-127
124-127
: Include both debug and release test commands
The guide currently shows only the debug test command. You may want to document the release variant as well:- task test:debug + task test:debug + task test:release
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.github/workflows/build.yaml
(3 hunks)CMakeLists.txt
(4 hunks)README.md
(6 hunks)examples/CMakeLists.txt
(2 hunks)examples/README.md
(1 hunks)taskfile.yaml
(1 hunks)taskfiles/build.yaml
(1 hunks)taskfiles/deps.yaml
(1 hunks)taskfiles/lint.yaml
(2 hunks)taskfiles/test.yaml
(1 hunks)tools/yscope-dev-utils
(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- taskfiles/deps.yaml
🚧 Files skipped from review as they are similar to previous changes (8)
- examples/CMakeLists.txt
- taskfiles/lint.yaml
- examples/README.md
- tools/yscope-dev-utils
- taskfiles/test.yaml
- CMakeLists.txt
- taskfile.yaml
- taskfiles/build.yaml
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
README.md
34-34: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (macos-15, release)
🔇 Additional comments (6)
.github/workflows/build.yaml (4)
42-43
: Checkout submodules recursively
The change to fetch submodules withsubmodules: "recursive"
is correct and ensures all submodules are available during the build.
49-50
: Invoke Task runner for unit tests
The switch from direct CMake commands totask test:${{matrix.build_type}}
aligns well with the new Task-based build system.
53-55
: Verify test log path on failure
Confirm that the log path./build/log-surgeon/Testing/Temporary/LastTest.log
matches where yourtest
task emits CTest logs. If the directory structure has changed under Task, this step may not find the file.
6-12
:✅ Verification successful
Verify workflow trigger patterns match actual Taskfile names
Please ensure that"taskfile.yaml"
and the"taskfiles/**/*"
path globs accurately reflect the filenames in the repository (e.g.Taskfile.yaml
vstaskfile.yaml
). If the casing or filename differs, the workflow may not trigger as expected.Also applies to: 15-21
🏁 Script executed:
#!/bin/bash set -e echo "Searching for Taskfile.yaml variants:" find . -type f | grep -E -i '/taskfile\.ya?ml$' || echo "No Taskfile.yaml files found" echo "" echo "Searching for 'taskfiles' directory:" find . -type d | grep -i '/taskfiles$' || echo "No 'taskfiles' directory found" echo "" echo "Listing YAML files under 'taskfiles' directory:" find . -type f | grep -i '^./taskfiles/.*\.ya?ml$' || echo "No YAML files under 'taskfiles' directory"Length of output: 586
Workflow trigger patterns correctly match existing Taskfile names
- The file
taskfile.yaml
exists in the repository root and matches the"taskfile.yaml"
pattern.- The directory
taskfiles/
exists and is correctly covered by the"taskfiles/**/*"
glob.No changes needed here.
README.md (2)
95-99
: Verify install command environment variable
The example usestask install:release INSTALL_PREFIX="~/.local"
. Confirm that your Taskfile defines and honoursINSTALL_PREFIX
(not another variable likePREFIX
) so this step installs to the desired location.
204-206
: Acknowledged addition of links for fmt and GSL
Adding explicit references forfmt
andGSL
is very helpful for new contributors.
os: | ||
- "macos-15" | ||
- "ubuntu-22.04" | ||
build_type: | ||
- "debug" | ||
- "release" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Invalid GitHub runner name: "macos-15"
As of now, GitHub Actions supports macos-13
(Ventura) or the macos-latest
alias. There is no macos-15
runner, so macOS jobs will fail.
Apply this diff to correct the runner:
- os:
- - "macos-15"
+ os:
+ - "macos-13"
- "ubuntu-22.04"
📝 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.
os: | |
- "macos-15" | |
- "ubuntu-22.04" | |
build_type: | |
- "debug" | |
- "release" | |
os: | |
- "macos-13" | |
- "ubuntu-22.04" | |
build_type: | |
- "debug" | |
- "release" |
🤖 Prompt for AI Agents
In .github/workflows/build.yaml at lines 33 to 38, the runner name "macos-15" is
invalid and will cause the macOS jobs to fail. Replace "macos-15" with a valid
runner name such as "macos-13" or "macos-latest" to ensure the workflow runs
correctly on macOS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the PR title, how about:
build: Migrate to task-based dependency management:
* Add new tasks for:
* installing dependencies (replacing library installation scripts and uses of `FetchContent`).
* building and installing debug or release versions of log-surgeon.
* building and running the unit tests.
* building and running the examples using an installed version of log-surgeon.
* Update build.yaml workflow to use the new tasks.
* Use `find_package` to find log-surgeon in `examples/CMakeLists.txt` to test log-surgeon's install process.
* Update taskfiles to follow IWYU.
(Assuming that's accurate.)
Co-authored-by: kirkrodrigues <[email protected]>
FetchContent
).find_package
to find log-surgeon inexamples/CMakeLists.txt
to test log-surgeon's install process.Description
Over time the best practices for task and CMake have changed, but log-surgeon has not been updated to keep up.
This PR primarily handles switching to task-based dependency management, but includes other various related CMake/dependency/task changes.
Checklist
breaking change.
Validation performed
Ran tasks locally and they are used in the updated CI.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores
Refactor