feat: add build workflow - #4
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: DSFans2014 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
387bb42 to
909f6b1
Compare
Signed-off-by: james <open4pd@4paradigm.com>
📝 WalkthroughWalkthroughUpdates the GitHub Actions workflow to run Rust builds and tests on ChangesCI Pipeline
Estimated code review effort: 2 (Simple) | ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci.yml (1)
1-49: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAdd a top-level
permissionsblock to follow least-privilege principle.The workflow has no
permissions:block, so it inherits the default token permissions, which can be overly broad. Since this workflow only builds and tests code, it only needs read access to repository contents.🔒 Proposed fix: add least-privilege permissions
+permissions: + contents: read + jobs:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 1 - 49, Add a top-level permissions block to the CI workflow granting only read access to repository contents, such as contents: read. Place it alongside the existing name and on configuration so both build and test jobs inherit the least-privilege setting.Source: Linters/SAST tools
🧹 Nitpick comments (3)
.github/workflows/ci.yml (3)
22-22: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valuePin container image by digest for reproducibility.
Using the floating tag
ascendai/cann:8.5.1means the base image can change without notice, potentially breaking CI or introducing supply-chain risk. Pin by digest (e.g.,ascendai/cann:8.5.1@sha256:...) to guarantee bit-for-bit reproducible runs.Also applies to: 38-38
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml at line 22, Update the container image references in the CI workflow to pin ascendai/cann:8.5.1 by its immutable SHA256 digest, including both occurrences, while retaining the existing version tag for readability.
33-33: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse
--lockedfor reproducible CI builds.
cargo buildandcargo testwithout--lockedmay silently updateCargo.lockif it's out of sync withCargo.toml. Adding--lockedensures CI fails fast if the lockfile is stale, preventing unintended dependency changes.♻️ Proposed fix
- cargo build + cargo build --locked- cargo test + cargo test --lockedAlso applies to: 48-48
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml at line 33, Update the Cargo commands in the CI workflow, including the visible cargo build step and the corresponding cargo test step, to pass the --locked flag. Keep the existing commands and workflow behavior unchanged so CI fails when Cargo.lock is out of sync.
35-48: 📐 Maintainability & Code Quality | 🔵 TrivialTest job only runs on x86_64; ARM builds are never tested.
The
buildjob covers both ARM and x86_64, but thetestjob only runs onubuntu-22.04(x86_64). If ARM is a supported target, tests should also run on ARM to catch architecture-specific issues. If ARM testing is intentionally skipped (e.g., CANN runtime limitations), consider adding a comment explaining why.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 35 - 48, The test job currently runs only on x86_64; update the CI workflow’s test job to also execute on the supported ARM target, matching the architecture coverage of the build job. If ARM tests cannot run because of CANN or another runtime limitation, add a concise comment documenting that intentional exclusion instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Line 24: Update both actions/checkout@v4 steps in the workflow to set
persist-credentials to false, including the checkout steps referenced near lines
24 and 40. Preserve the existing checkout behavior and all other job
configuration.
- Around line 6-9: Update the tag patterns in the workflow’s tags filter to use
GitHub Actions fnmatch wildcards instead of regex quantifiers, replacing the
literal “+” usage so version tags such as v1.2 and v1.2.3 match correctly while
preserving the existing version-shape entries.
---
Outside diff comments:
In @.github/workflows/ci.yml:
- Around line 1-49: Add a top-level permissions block to the CI workflow
granting only read access to repository contents, such as contents: read. Place
it alongside the existing name and on configuration so both build and test jobs
inherit the least-privilege setting.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Line 22: Update the container image references in the CI workflow to pin
ascendai/cann:8.5.1 by its immutable SHA256 digest, including both occurrences,
while retaining the existing version tag for readability.
- Line 33: Update the Cargo commands in the CI workflow, including the visible
cargo build step and the corresponding cargo test step, to pass the --locked
flag. Keep the existing commands and workflow behavior unchanged so CI fails
when Cargo.lock is out of sync.
- Around line 35-48: The test job currently runs only on x86_64; update the CI
workflow’s test job to also execute on the supported ARM target, matching the
architecture coverage of the build job. If ARM tests cannot run because of CANN
or another runtime limitation, add a concise comment documenting that
intentional exclusion instead.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 42894013-3681-4184-925a-4dd61ef795ea
📒 Files selected for processing (1)
.github/workflows/ci.yml
| tags: | ||
| - v[0-9]+.[0-9]+.[0-9]+.[0-9]+ | ||
| - v[0-9]+.[0-9]+.[0-9]+ | ||
| - v[0-9]+.[0-9]+ |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Tag patterns use regex quantifiers but GitHub Actions expects glob/fnmatch syntax.
Patterns like v[0-9]+.[0-9]+.[0-9]+.[0-9]+ use + as a quantifier, but GitHub Actions tag filters use fnmatch glob syntax where + is a literal character. These patterns will only match tags literally containing + (e.g., v1+2+3+4), not version tags like v1.2.3.4. Use * or ? glob wildcards instead.
🔧 Proposed fix for tag patterns
on:
push:
branches: [ "main" ]
tags:
- - v[0-9]+.[0-9]+.[0-9]+.[0-9]+
- - v[0-9]+.[0-9]+.[0-9]+
- - v[0-9]+.[0-9]+
+ - v[0-9]*.[0-9]*.[0-9]*.[0-9]*
+ - v[0-9]*.[0-9]*.[0-9]*
+ - v[0-9]*.[0-9]*📝 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.
| tags: | |
| - v[0-9]+.[0-9]+.[0-9]+.[0-9]+ | |
| - v[0-9]+.[0-9]+.[0-9]+ | |
| - v[0-9]+.[0-9]+ | |
| tags: | |
| - v[0-9]*.[0-9]*.[0-9]*.[0-9]* | |
| - v[0-9]*.[0-9]*.[0-9]* | |
| - v[0-9]*.[0-9]* |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ci.yml around lines 6 - 9, Update the tag patterns in the
workflow’s tags filter to use GitHub Actions fnmatch wildcards instead of regex
quantifiers, replacing the literal “+” usage so version tags such as v1.2 and
v1.2.3 match correctly while preserving the existing version-shape entries.
| container: | ||
| image: ascendai/cann:8.5.1 | ||
| steps: | ||
| - uses: actions/checkout@v4 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Set persist-credentials: false on checkout steps.
actions/checkout@v4 persists a GitHub token in .git/config by default. Since neither job performs git pushes, this credential is unnecessary and should be disabled to reduce the attack surface inside the container.
🔒 Proposed fix
- uses: actions/checkout@v4
+ with:
+ persist-credentials: falseApply to both checkout steps (lines 24 and 40).
Also applies to: 40-40
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 24-24: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ci.yml at line 24, Update both actions/checkout@v4 steps
in the workflow to set persist-credentials to false, including the checkout
steps referenced near lines 24 and 40. Preserve the existing checkout behavior
and all other job configuration.
Source: Linters/SAST tools
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/ci.yml (1)
24-24: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winSet
persist-credentials: falseon both checkout steps.
actions/checkout@v4persists a GitHub token in.git/configby default. Neither job performs git pushes, so this credential is unnecessary and should be disabled to reduce the attack surface inside the container.🔒 Proposed fix
- uses: actions/checkout@v4 + with: + persist-credentials: falseApply to both checkout steps (lines 24 and 40).
Also applies to: 40-40
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml at line 24, Update both actions/checkout@v4 steps in the workflow to set persist-credentials to false, preserving the existing checkout behavior while preventing unnecessary GitHub token storage.Source: Linters/SAST tools
🧹 Nitpick comments (2)
.github/workflows/ci.yml (2)
19-19: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove commented-out matrix entry.
The commented-out
ubuntu-22.04(x86_64) platform entry is dead code. If x86_64 support is planned, track it in an issue rather than leaving commented configuration.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml at line 19, Remove the commented-out ubuntu-22.04 matrix entry from the CI workflow; leave the active matrix configuration unchanged.
31-32: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove diagnostic commands before merging.
echo $LD_LIBRARY_PATHand thelddcall are useful for initial debugging but add noise to CI logs. Consider removing them or moving to a separate optional diagnostic step.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 31 - 32, Remove the diagnostic echo of LD_LIBRARY_PATH and the ldd invocation from the CI workflow step. Keep the surrounding setup and build commands unchanged; do not retain these commands in the normal CI path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Line 47: Update the LD_LIBRARY_PATH export in the CI environment setup to use
the ARM-specific Ascend devlib path for the ubuntu-22.04-arm runner, or source
the existing set_env.sh script so it configures the correct paths. Remove the
x86_64-linux/devlib entry while preserving the other required library paths.
---
Duplicate comments:
In @.github/workflows/ci.yml:
- Line 24: Update both actions/checkout@v4 steps in the workflow to set
persist-credentials to false, preserving the existing checkout behavior while
preventing unnecessary GitHub token storage.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Line 19: Remove the commented-out ubuntu-22.04 matrix entry from the CI
workflow; leave the active matrix configuration unchanged.
- Around line 31-32: Remove the diagnostic echo of LD_LIBRARY_PATH and the ldd
invocation from the CI workflow step. Keep the surrounding setup and build
commands unchanged; do not retain these commands in the normal CI path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7f729398-6286-4ddb-a23d-127c9021a951
📒 Files selected for processing (1)
.github/workflows/ci.yml
| uses: Swatinem/rust-cache@v2 | ||
| - name: test | ||
| run: | | ||
| export LD_LIBRARY_PATH=/usr/local/Ascend/cann/lib64:/usr/local/Ascend/ascend-toolkit/latest/lib64:/usr/local/Ascend/cann/x86_64-linux/devlib:$LD_LIBRARY_PATH |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the CANN container's devlib directory structure for the correct arch path.
# Run this inside the ascendai/cann:9.0.0 ARM image if available:
# docker run --rm ascendai/cann:9.0.0 ls /usr/local/Ascend/cann/
fd 'devlib' /usr/local/Ascend/ 2>/dev/null || find /usr/local/Ascend -type d -name 'devlib' 2>/dev/nullRepository: Project-HAMi/hami-vnpu-core
Length of output: 165
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the workflow section around the reported lines.
sed -n '1,120p' .github/workflows/ci.yml
# Look for any other Ascend path conventions in the repo.
rg -n "x86_64-linux/devlib|aarch64-linux/devlib|arm64-linux/devlib|LD_LIBRARY_PATH|ASCEND_TOOLKIT_HOME|ASCEND_DRIVER_HOME" .Repository: Project-HAMi/hami-vnpu-core
Length of output: 1925
🌐 Web query:
ascendai/cann ARM devlib path aarch64-linux x86_64-linux /usr/local/Ascend/cann
💡 Result:
In Ascend CANN environments, the devlib directory contains dynamic link libraries necessary for development [1][2]. Its path depends on the installation directory, the CANN version, and the host architecture [1][2]. The default installation path for the root user is typically /usr/local/Ascend/ascend-toolkit (or /usr/local/Ascend/cann in newer versions like 8.5.0 and later) [3][4][5]. For non-root users, the default is ${HOME}/Ascend/ascend-toolkit [6][5]. The dynamic library path is generally structured as follows: {install_path}/latest/-linux/devlib Where: - {install_path}: The base installation directory (e.g., /usr/local/Ascend/ascend-toolkit) [1][3]. - : The CPU architecture of the host machine, which should be replaced with either aarch64 or x86_64 depending on your system [2][7][6]. Example path for an aarch64-linux system (root user): /usr/local/Ascend/ascend-toolkit/latest/aarch64-linux/devlib [3][2] Example path for an x86_64-linux system (root user): /usr/local/Ascend/ascend-toolkit/latest/x86_64-linux/devlib [2] To correctly configure your environment, it is standard practice to source the provided environment setup script, which automatically handles these path definitions: source {install_path}/set_env.sh [2][6][5] Note that starting with CANN version 8.5.0, the installation directory structure has transitioned from /usr/local/Ascend/ascend-toolkit to /usr/local/Ascend/cann [4]. Always verify your actual installation directory using the echo command or by navigating to the /usr/local/Ascend directory [3][8].
Citations:
- 1: https://www.hiascend.com/document/detail/en/canncommercial/800/devaids/atc/atlasatc_16_0002.html
- 2: https://www.hiascend.com/document/detail/zh/canncommercial/80RC22/softwareinst/instg/instg_0057.html
- 3: https://context7.com/seelevolle/cann-ops/llms.txt
- 4: https://www.hiascend.com/document/detail/en/canncommercial/850/releasenote/releasenote_0001.html
- 5: https://github.com/Ascend/triton-ascend/blob/main/docs/en/installation_guide.md
- 6: https://www.hiascend.com/document/detail/zh/canncommercial/82RC1/softwareinst/instg/instg_0054.html
- 7: https://www.hiascend.com/doc_center/source/zh/canncommercial/80RC2/softwareinst/instg/instg_0057.html
- 8: https://support.huawei.com/enterprise/en/doc/EDOC1100356040/5aa8cc53/deploying-a-container
Use the ARM devlib path here. x86_64-linux/devlib doesn’t match the ubuntu-22.04-arm runner, so cargo test can miss the required libraries. Source set_env.sh here or switch to the ARM-specific devlib path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ci.yml at line 47, Update the LD_LIBRARY_PATH export in
the CI environment setup to use the ARM-specific Ascend devlib path for the
ubuntu-22.04-arm runner, or source the existing set_env.sh script so it
configures the correct paths. Remove the x86_64-linux/devlib entry while
preserving the other required library paths.
1 participant
Summary by CodeRabbit
Tests
Chores