Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRenamed multiple FFI enum variants for clarity, added C header test programs and a header-validation script, updated cbindgen to include an external FFI header, and added a CI step to run header validation. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CI as GitHub Actions
participant Script as validate-headers.sh
participant Build as Cargo (build FFI crates)
participant GCC as gcc (compile tests)
participant Result as Aggregate Result
CI->>Script: run validate-headers.sh
Script->>Build: build FFI crates (produce headers in target/)
Build-->>Script: headers available
Script->>GCC: for each header-test.c, run gcc -c with include paths
GCC-->>Script: success / failure per file
Script->>Result: print Passed/Failed, set aggregate exit code
Script-->>CI: exit with aggregate code
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
ZocoLini
force-pushed
the
test/c-header-validation
branch
from
cde6e3d to
a362c9a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
dash-spv-ffi/src/bin/ffi_cli.rs (1)
1-491: PR title "test(ffi)" may under-represent the scope of changes.Per coding guidelines, verify the PR title accurately describes the changes. This file and
callbacks.rscontain functional code changes (enum variant renames to fix name collisions), not just test additions. Consider whether the PR title should befix(ffi)orrefactor(ffi)to better reflect the symbol renaming, or split the test infrastructure additions from the collision fixes into separate PRs.As per coding guidelines: "Check whether the PR title prefix ... accurately describes the changes. Flag e.g. if a PR labeled 'fix' appears to add new functionality..."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash-spv-ffi/src/bin/ffi_cli.rs` around lines 1 - 491, The PR title "test(ffi)" is misleading because this change includes functional renames (enum variant renames to avoid symbol collisions) in files such as the FFI callbacks (e.g., FFISyncState variants and FFIEventCallbacks usage) and callbacks.rs rather than just tests; update the PR title to accurately reflect the scope (for example "fix(ffi): rename enum variants to avoid collisions" or "refactor(ffi): rename FFI enum variants and callbacks") or split the changes into separate PRs (one for test infra and one for the symbol-renaming fixes) so reviewers can clearly see the functional rename work on symbols like FFISyncState, FFIEventCallbacks, and any renamed variants in callbacks.rs.ffi-c-tests/validate-headers.sh (2)
23-30: Consider adding explicit include path and running the test binary.Two suggestions for robustness:
- Add
-Iflag for header include path rather than relying on relative paths embedded in test sources.- Consider actually running the compiled binary (
./test.bin) to catch runtime link errors—currently only compile-time validation is performed.♻️ Suggested improvements
for file in "$HEADER_TESTS_DIR"/*.c; do - if gcc "$file" -o test.bin -g -L"$LIB_DIR" -ldash_spv_ffi -lkey_wallet_ffi; then + if gcc "$file" -o test.bin -g -I"$TARGET_DIR/debug/include" -L"$LIB_DIR" -ldash_spv_ffi -lkey_wallet_ffi -Wl,-rpath,"$LIB_DIR" && ./test.bin; then echo -e "${GREEN}Passed: $file${NC}" else echo -e "${RED}Failed: $file${NC}" EXIT_CODE=1 fi doneNote: If you add
-Ifor explicit include paths, update the.cfiles to use#include "dash-spv-ffi/dash-spv-ffi.h"instead of the full relative path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ffi-c-tests/validate-headers.sh` around lines 23 - 30, The test loop currently only compiles each C test using gcc in the for loop that references HEADER_TESTS_DIR and LIB_DIR; update the gcc invocation to add an explicit include path (e.g. -I"$HEADER_TESTS_DIR" or another header dir variable) so headers are resolved via -I instead of baked relative paths, update test sources to use `#include` "dash-spv-ffi/dash-spv-ffi.h" as noted, and after a successful compile run the resulting binary (./test.bin) to catch runtime link errors and set EXIT_CODE on failures; ensure the build step still links -L"$LIB_DIR" -ldash_spv_ffi -lkey_wallet_ffi and remove/overwrite test.bin between iterations to avoid stale artifacts.
23-24: Handle case when no.cfiles exist.If the
header-tests/directory is empty or contains no.cfiles, the glob*.cwon't expand and the loop may process the literal string"*.c"causing a confusing error.♻️ Add nullglob or file existence check
+shopt -s nullglob for file in "$HEADER_TESTS_DIR"/*.c; do if gcc "$file" -o test.bin -g -L"$LIB_DIR" -ldash_spv_ffi -lkey_wallet_ffi; thenOr alternatively:
for file in "$HEADER_TESTS_DIR"/*.c; do + [ -e "$file" ] || continue if gcc "$file" -o test.bin -g -L"$LIB_DIR" -ldash_spv_ffi -lkey_wallet_ffi; then🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ffi-c-tests/validate-headers.sh` around lines 23 - 24, The loop over C files using the glob "$HEADER_TESTS_DIR"/*.c can iterate the literal "*.c" when no matches exist; update validate-headers.sh to guard the loop by either enabling nullglob (e.g., set shell option before the loop) or by performing a pre-check such as collecting matches into an array from "$HEADER_TESTS_DIR"/*.c and exiting or skipping if the array is empty; adjust the code around the for file in "$HEADER_TESTS_DIR"/*.c; do and the subsequent gcc invocation so the loop only runs when real .c files exist.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@dash-spv-ffi/src/bin/ffi_cli.rs`:
- Around line 1-491: The PR title "test(ffi)" is misleading because this change
includes functional renames (enum variant renames to avoid symbol collisions) in
files such as the FFI callbacks (e.g., FFISyncState variants and
FFIEventCallbacks usage) and callbacks.rs rather than just tests; update the PR
title to accurately reflect the scope (for example "fix(ffi): rename enum
variants to avoid collisions" or "refactor(ffi): rename FFI enum variants and
callbacks") or split the changes into separate PRs (one for test infra and one
for the symbol-renaming fixes) so reviewers can clearly see the functional
rename work on symbols like FFISyncState, FFIEventCallbacks, and any renamed
variants in callbacks.rs.
In `@ffi-c-tests/validate-headers.sh`:
- Around line 23-30: The test loop currently only compiles each C test using gcc
in the for loop that references HEADER_TESTS_DIR and LIB_DIR; update the gcc
invocation to add an explicit include path (e.g. -I"$HEADER_TESTS_DIR" or
another header dir variable) so headers are resolved via -I instead of baked
relative paths, update test sources to use `#include`
"dash-spv-ffi/dash-spv-ffi.h" as noted, and after a successful compile run the
resulting binary (./test.bin) to catch runtime link errors and set EXIT_CODE on
failures; ensure the build step still links -L"$LIB_DIR" -ldash_spv_ffi
-lkey_wallet_ffi and remove/overwrite test.bin between iterations to avoid stale
artifacts.
- Around line 23-24: The loop over C files using the glob
"$HEADER_TESTS_DIR"/*.c can iterate the literal "*.c" when no matches exist;
update validate-headers.sh to guard the loop by either enabling nullglob (e.g.,
set shell option before the loop) or by performing a pre-check such as
collecting matches into an array from "$HEADER_TESTS_DIR"/*.c and exiting or
skipping if the array is empty; adjust the code around the for file in
"$HEADER_TESTS_DIR"/*.c; do and the subsequent gcc invocation so the loop only
runs when real .c files exist.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 818333fc-b13e-4327-bc19-b10aa1fbac53
📒 Files selected for processing (9)
.github/workflows/ffi-c-tests.ymldash-spv-ffi/cbindgen.tomldash-spv-ffi/src/bin/ffi_cli.rsdash-spv-ffi/src/callbacks.rsffi-c-tests/header-tests/dash-spv.cffi-c-tests/header-tests/key-wallet.cffi-c-tests/header-tests/spv_and_key_wallet.cffi-c-tests/validate-headers.shkey-wallet-ffi/src/transaction.rs
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v0.42-dev #609 +/- ##
=============================================
- Coverage 67.60% 67.57% -0.04%
=============================================
Files 321 321
Lines 68258 68258
=============================================
- Hits 46147 46123 -24
- Misses 22111 22135 +24
|
|
@QuantumExplorer I modified the enum you added yesterday, want to make sure you like it |
xdustinface
left a comment
xdustinface
left a comment
There was a problem hiding this comment.
Makes sense generally but the workflow comment i left should be addressed and maybe have a look at https://docs.rs/cbindgen/latest/cbindgen/struct.EnumConfig.html#structfield.prefix_with_name which could be an option to avoid name conflicts? Or the other available options there.. maybe something useful for us.
.github/workflows/ffi-c-tests.yml
Outdated
| - name: Install C compiler | ||
| run: sudo apt-get update && sudo apt-get install -y build-essential | ||
|
|
||
| - name: Run header validation script |
There was a problem hiding this comment.
This step should be included into build-and-test.yml after running the tests guarded by the ffi CI group to avoid rebuilding the crate every time separately.
There was a problem hiding this comment.
yeah, just did that but it rebuild the crate anyway. CI is building with llvm-cov, creating artifacts in target/llvm-cov-target, but when i build using cargo or cargo rustc the artifacts are being generated in target so it has to rebuild everything from scratch. I did change the script to build as little as possible (only the static lib)
There was a problem hiding this comment.
I guess you can just set TARGET_DIR and LIB_DIR?
ffi-c-tests/validate-headers.sh
Outdated
| LIB_DIR="$TARGET_DIR/debug" | ||
|
|
||
| # Build FFI crates to generate static libraries and header files | ||
| cargo build -p dash-spv-ffi --profile dev |
There was a problem hiding this comment.
This should be guarded by an argument if the check gets moved into the existing workflows.
There was a problem hiding this comment.
tbh, i dont know what you mean here
There was a problem hiding this comment.
I mean like:
# Build FFI crates unless --skip-build is passed (CI already built them)
if [[ "${1:-}" != "--skip-build" ]]; then
cargo build -p dash-spv-ffi --profile dev
cargo build -p key-wallet-ffi --profile dev
fi
github-actions
bot
added
the
merge-conflict
|
This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them. |
ZocoLini
force-pushed
the
test/c-header-validation
branch
from
a362c9a to
f7f8b3c
Compare
github-actions
bot
removed
merge-conflict
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
ffi-c-tests/validate-headers.sh (1)
11-13: RespectCARGO_TARGET_DIRto avoid path brittleness.Lines 11–13 assume
../target. IfCARGO_TARGET_DIRis set, header and library paths can break.♻️ Proposed change
-TARGET_DIR="$SCRIPT_DIR/../target" +TARGET_DIR="${CARGO_TARGET_DIR:-$SCRIPT_DIR/../target}" LIB_DIR="$TARGET_DIR/debug"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ffi-c-tests/validate-headers.sh` around lines 11 - 13, The script hardcodes TARGET_DIR as "$SCRIPT_DIR/../target" which breaks when CARGO_TARGET_DIR is set; change TARGET_DIR to respect that environment variable (e.g. TARGET_DIR="${CARGO_TARGET_DIR:-$SCRIPT_DIR/../target}") and then compute LIB_DIR from TARGET_DIR (e.g. LIB_DIR="$TARGET_DIR/debug" or use a CARGO_PROFILE variable if you need release/profile selection) so header/library lookups work whether or not CARGO_TARGET_DIR is defined.ffi-c-tests/header-tests/dash-spv.c (1)
1-1: Avoid hardcodingtarget/debugin header includes.Line 1 couples this test to a specific target layout/profile. Prefer stable header names and pass include roots from the validator script.
♻️ Proposed change
-#include "../../target/debug/include/dash-spv-ffi/dash-spv-ffi.h" +#include "dash-spv-ffi/dash-spv-ffi.h"# companion change in ffi-c-tests/validate-headers.sh compile command: # add: -I"$LIB_DIR/include"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ffi-c-tests/header-tests/dash-spv.c` at line 1, Replace the hardcoded include path "#include \"../../target/debug/include/dash-spv-ffi/dash-spv-ffi.h\"" with a stable header include such as "#include <dash-spv-ffi/dash-spv-ffi.h>" so the test does not depend on target/debug layout; then update the validator/compile invocation (validate-headers.sh) to pass the include root (e.g. add -I"$LIB_DIR/include") so the compiler can locate the header via the new stable include. Ensure the change touches the include line in dash-spv.c and the compile command in validate-headers.sh.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/build-and-test.yml:
- Around line 93-97: The PR title prefix "test(ffi):" is too narrow because the
change also renames exported FFI enum variants (a fix/refactor), so either
retitle the PR to reflect both testing and code changes (e.g., include
"fix(ffi):" or "refactor(ffi):" alongside testing in the title) or split the
changes into two PRs (one test-only and one that renames enum variants). Update
the pr-title.yml workflow rules if you want to allow combined prefixes, and
ensure the PR title used by the workflow matches the allowed prefixes (reference
the pr-title.yml workflow and the current PR title "test(ffi):..." and the enum
renaming in your FFI changes) so the CI validation and project guidelines are
consistent.
---
Nitpick comments:
In `@ffi-c-tests/header-tests/dash-spv.c`:
- Line 1: Replace the hardcoded include path "#include
\"../../target/debug/include/dash-spv-ffi/dash-spv-ffi.h\"" with a stable header
include such as "#include <dash-spv-ffi/dash-spv-ffi.h>" so the test does not
depend on target/debug layout; then update the validator/compile invocation
(validate-headers.sh) to pass the include root (e.g. add -I"$LIB_DIR/include")
so the compiler can locate the header via the new stable include. Ensure the
change touches the include line in dash-spv.c and the compile command in
validate-headers.sh.
In `@ffi-c-tests/validate-headers.sh`:
- Around line 11-13: The script hardcodes TARGET_DIR as "$SCRIPT_DIR/../target"
which breaks when CARGO_TARGET_DIR is set; change TARGET_DIR to respect that
environment variable (e.g.
TARGET_DIR="${CARGO_TARGET_DIR:-$SCRIPT_DIR/../target}") and then compute
LIB_DIR from TARGET_DIR (e.g. LIB_DIR="$TARGET_DIR/debug" or use a CARGO_PROFILE
variable if you need release/profile selection) so header/library lookups work
whether or not CARGO_TARGET_DIR is defined.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 51258edd-5b6b-4b06-b809-563cd824d8e7
📒 Files selected for processing (9)
.github/workflows/build-and-test.ymldash-spv-ffi/cbindgen.tomldash-spv-ffi/src/bin/ffi_cli.rsdash-spv-ffi/src/callbacks.rsffi-c-tests/header-tests/dash-spv.cffi-c-tests/header-tests/key-wallet.cffi-c-tests/header-tests/spv_and_key_wallet.cffi-c-tests/validate-headers.shkey-wallet-ffi/src/transaction.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- dash-spv-ffi/cbindgen.toml
- dash-spv-ffi/src/bin/ffi_cli.rs
.github/workflows/build-and-test.yml
Outdated
| - name: Validate headers | ||
| if: matrix.group == 'ffi' && inputs.os == 'ubuntu-latest' | ||
| run: | | ||
| sudo apt update && sudo apt install -y build-essential | ||
| bash ffi-c-tests/validate-headers.sh |
There was a problem hiding this comment.
PR title prefix looks too narrow for the actual scope.
test(ffi): ... doesn’t fully reflect that this PR also renames exported FFI enum variants to fix header collisions. Please retitle to include fix/refactor scope (or split if you want a test-only PR).
As per coding guidelines: **: Check whether the PR title prefix allowed in the pr-title.yml workflow accurately describes the changes… and suggest splitting unrelated concerns.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/build-and-test.yml around lines 93 - 97, The PR title
prefix "test(ffi):" is too narrow because the change also renames exported FFI
enum variants (a fix/refactor), so either retitle the PR to reflect both testing
and code changes (e.g., include "fix(ffi):" or "refactor(ffi):" alongside
testing in the title) or split the changes into two PRs (one test-only and one
that renames enum variants). Update the pr-title.yml workflow rules if you want
to allow combined prefixes, and ensure the PR title used by the workflow matches
the allowed prefixes (reference the pr-title.yml workflow and the current PR
title "test(ffi):..." and the enum renaming in your FFI changes) so the CI
validation and project guidelines are consistent.
f7f8b3c to
8e0f2ec
Compare
|
@xdustinface about the link you shared, i know about that option and I already mention in the call we had yesterday that it would be the ideal solution to avoid any collision in the future, but at the same time I think we shouldn't be using it in this PR to fix the issues found by |
ZocoLini
force-pushed
the
test/c-header-validation
branch
2 times, most recently
from
15ba47e to
1aca143
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ffi-c-tests/header-tests/dash-spv.c`:
- Line 1: The test currently hardcodes an environment-dependent include path
("../../target/llvm-cov-target/debug/include/dash-spv-ffi/dash-spv-ffi.h") which
breaks in other environments; change the test source (dash-spv.c) to include the
stable public header name (e.g., "dash-spv-ffi/dash-spv-ffi.h" or the project’s
canonical header path) and remove the hardcoded relative target path, and update
the test build invocation to pass the appropriate -I include directory from the
test script/CI so the compiler can find the header without relying on
"target/.../debug".
In `@ffi-c-tests/header-tests/key-wallet.c`:
- Line 4: The local variable `version` assigned from key_wallet_ffi_version() is
never used and triggers compiler warnings; either mark it explicitly unused
(e.g., cast to void) or perform a simple validation/assertion on it. Update the
test in key-wallet.c where `const char *version = key_wallet_ffi_version();` is
declared so that the variable is consumed—use a void cast or add a basic check
(e.g., ensure version != NULL or length > 0) and handle/report failure to avoid
unused-variable warnings.
In `@ffi-c-tests/header-tests/spv_and_key_wallet.c`:
- Line 8: The local variable `version` assigned from key_wallet_ffi_version() is
unused and causes compiler warnings; update the test to either explicitly mark
it as unused (e.g., cast to void) or perform a minimal validation/assertion on
it (e.g., check it's non-NULL or non-empty) so the value is used; modify the
line with `const char *version = key_wallet_ffi_version();` in
spv_and_key_wallet.c (and add the matching include for assert if you use an
assertion) to eliminate the unused-variable warning.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 366b0e04-eb34-4c8f-bb23-4f5756de95b1
📒 Files selected for processing (6)
.github/workflows/build-and-test.ymlffi-c-tests/header-tests/dash-spv.cffi-c-tests/header-tests/key-wallet.cffi-c-tests/header-tests/spv_and_key_wallet.cffi-c-tests/validate-headers.shkey-wallet-ffi/src/transaction.rs
✅ Files skipped from review due to trivial changes (1)
- ffi-c-tests/validate-headers.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/build-and-test.yml
- key-wallet-ffi/src/transaction.rs
ZocoLini
force-pushed
the
test/c-header-validation
branch
from
1aca143 to
55f247f
Compare
|
@xdustinface done, i basically made the script receive the include dir as the first param and use relative paths to the include directory in the .c files. I hope you like it this way. I also made cbindgen rename all enums to avoid present and future collisions, having to rename variants inside 3 enums was my limit, lets see how this behaves in iOS |
ZocoLini
force-pushed
the
test/c-header-validation
branch
from
55f247f to
451e715
Compare
xdustinface
left a comment
xdustinface
left a comment
There was a problem hiding this comment.
Looks good! I feel like there should be some way to use a cargo env var which the script could read instead of having the hard coded path but it works like this for now.
2 participants
Summary by CodeRabbit