chore: generate ffi headers/docs without building the crates#502
Draft
xdustinface wants to merge 1 commit intov0.42-devfrom
Draft
chore: generate ffi headers/docs without building the crates#502xdustinface wants to merge 1 commit intov0.42-devfrom
xdustinface wants to merge 1 commit intov0.42-devfrom
Conversation
Contributor
📝 WalkthroughWalkthroughRefactors FFI header verification to use the cbindgen CLI for header generation; CI pre-commit workflow now installs cbindgen; pre-commit hooks replace the verify-ffi local hook with a clippy slow-checks entry; the Python verification script now invokes cbindgen instead of building crates with cargo. Changes
Sequence Diagram(s)sequenceDiagram
participant CI as CI Workflow
participant Pre as Pre-commit
participant Script as verify_ffi.py
participant CBind as cbindgen CLI
participant FS as Repository FS
CI->>CI: Cache cargo dependencies
CI->>CI: Install cbindgen (cbindgen@0.29.2)
CI->>Pre: Run pre-commit
Pre->>Script: invoke verify_ffi.py
Script->>CBind: check_cbindgen() (CLI present?)
alt cbindgen present
Script->>CBind: regenerate_headers(repo_root)
CBind->>FS: write generated headers
Script->>FS: run docs generation & diff checks
Script-->>Pre: return success/failure
else missing cbindgen
Script-->>Pre: return failure (missing cbindgen)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v0.42-dev #502 +/- ##
==========================================
Coverage 66.83% 66.83%
==========================================
Files 313 313
Lines 64878 64878
==========================================
+ Hits 43363 43364 +1
+ Misses 21515 21514 -1
*This pull request uses carry forward flags. Click here to find out more. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
contrib/verify_ffi.py (1)
13-18: Reuse the resolvedcbindgenpath instead of looking it up twice.
check_cbindgen()already resolves the executable, butregenerate_headers()invokes bare"cbindgen"again after switchingcwd. Thread the absolute path fromshutil.which()through tosubprocess.run()so the selected binary is deterministic and the Ruff S607 warning goes away.Also applies to: 27-32
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contrib/verify_ffi.py` around lines 13 - 18, check_cbindgen() currently calls shutil.which() to resolve the cbindgen binary but regenerate_headers() later invokes the literal "cbindgen" (and similarly in the block around lines 27-32), causing a second lookup and a non-deterministic call; change check_cbindgen() to return the resolved absolute path and update regenerate_headers() and the other subprocess.run invocations to use that returned path (pass the resolved path into regenerate_headers() or store and reuse the value) so subprocess.run calls use the deterministic absolute executable path instead of the bare "cbindgen".
🤖 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/pre-commit.yml:
- Around line 41-42: Replace the unpinned cbindgen install step in the workflow
so CI uses a fixed release: change the `cargo install cbindgen` invocation to
install an exact version (e.g. `cbindgen@0.29` with `--locked`) so generated
headers (referenced by dash-spv-ffi/cbindgen.toml and
key-wallet-ffi/cbindgen.toml which set include_version = true) remain stable and
contrib/verify_ffi.py no longer flags spurious diffs.
---
Nitpick comments:
In `@contrib/verify_ffi.py`:
- Around line 13-18: check_cbindgen() currently calls shutil.which() to resolve
the cbindgen binary but regenerate_headers() later invokes the literal
"cbindgen" (and similarly in the block around lines 27-32), causing a second
lookup and a non-deterministic call; change check_cbindgen() to return the
resolved absolute path and update regenerate_headers() and the other
subprocess.run invocations to use that returned path (pass the resolved path
into regenerate_headers() or store and reuse the value) so subprocess.run calls
use the deterministic absolute executable path instead of the bare "cbindgen".
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ec9c1297-8c66-4562-8487-e4de5b6d7733
📒 Files selected for processing (3)
.github/workflows/pre-commit.yml.pre-commit-config.yamlcontrib/verify_ffi.py
Get rid of the unnecessary build step for ffi header generation to make the ffi header/docs generation basically happening in no time compared to before where it always had to do a build before which slowed things down whenever something changed in the relevant files which is annoying in local development workflows. Since its much faster this way I also moved the step from `pre-push` to `pre-commit` stage.
xdustinface
force-pushed
the
fix/verify-ffi-speed
branch
from
84b12b9 to
de59908
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contrib/verify_ffi.py`:
- Around line 13-18: Introduce a CBINDGEN_VERSION = "0.29.2" constant, rename
check_cbindgen() to resolve_cbindgen() so it returns the resolved cbindgen
binary path (use shutil.which), run that binary with "--version" to verify the
output contains CBINDGEN_VERSION and error if mismatched, and update
regenerate_headers() to accept a cbindgen_path parameter and use that exact
binary (instead of invoking bare "cbindgen") so the same pinned tool is reused
across checks and header regeneration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f9688797-831c-4338-b94e-802843805f68
📒 Files selected for processing (3)
.github/workflows/pre-commit.yml.pre-commit-config.yamlcontrib/verify_ffi.py
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/pre-commit.yml
Comment on lines
+13
to
18
| def check_cbindgen() -> bool: | ||
| """Verify that cbindgen CLI is installed.""" | ||
| if shutil.which("cbindgen") is None: | ||
| print("cbindgen not found. Install with: cargo install cbindgen", file=sys.stderr) | ||
| return False | ||
| return True |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python - <<'PY'
from pathlib import Path
import re
workflow = Path(".github/workflows/pre-commit.yml").read_text()
match = re.search(r'cargo install cbindgen@([0-9][^ ]*) --locked', workflow)
print("workflow_pinned_cbindgen:", match.group(1) if match else "not found")
PY
cbindgen --versionRepository: dashpay/rust-dashcore
Length of output: 164
🏁 Script executed:
cat -n contrib/verify_ffi.pyRepository: dashpay/rust-dashcore
Length of output: 5054
Add version pinning and reuse the resolved cbindgen binary path.
CI installs cbindgen@0.29.2 --locked, but this script accepts any cbindgen on PATH without version verification and then invokes bare cbindgen on line 28. This creates non-reproducibility: developers with a different cbindgen version generate different headers locally than CI produces, causing false hook passes/failures.
Suggested fix: Define CBINDGEN_VERSION = "0.29.2" as a constant, rename check_cbindgen() to resolve_cbindgen() to return the resolved binary path and verify the version via cbindgen --version, then pass the path to regenerate_headers() to reuse the same binary.
Suggested fix
+CBINDGEN_VERSION = "0.29.2"
+
-def check_cbindgen() -> bool:
- """Verify that cbindgen CLI is installed."""
- if shutil.which("cbindgen") is None:
- print("cbindgen not found. Install with: cargo install cbindgen", file=sys.stderr)
- return False
- return True
+def resolve_cbindgen() -> str | None:
+ """Resolve the pinned cbindgen CLI."""
+ cbindgen = shutil.which("cbindgen")
+ if cbindgen is None:
+ print(
+ f"cbindgen {CBINDGEN_VERSION} not found. Install with: "
+ f"cargo install cbindgen@{CBINDGEN_VERSION} --locked",
+ file=sys.stderr,
+ )
+ return None
+
+ version = subprocess.run([cbindgen, "--version"], capture_output=True, text=True)
+ if version.returncode != 0 or f" {CBINDGEN_VERSION}" not in version.stdout:
+ print(
+ f"Expected cbindgen {CBINDGEN_VERSION}, got "
+ f"{version.stdout.strip() or 'unknown'}. "
+ f"Install with: cargo install cbindgen@{CBINDGEN_VERSION} --locked",
+ file=sys.stderr,
+ )
+ return None
+
+ return cbindgen
-def regenerate_headers(repo_root: Path) -> bool:
+def regenerate_headers(repo_root: Path, cbindgen: str) -> bool:
"""Regenerate C headers by calling cbindgen directly (no cargo build needed)."""
for crate in FFI_CRATES:
crate_dir = repo_root / crate
header = f"include/{crate.replace('-', '_')}.h"
print(f" Generating {crate} header...")
result = subprocess.run(
- ["cbindgen", "--config", "cbindgen.toml",
+ [cbindgen, "--config", "cbindgen.toml",
"--crate", crate, "--output", header],
cwd=crate_dir,
capture_output=True, text=True,
)
@@
- if not check_cbindgen():
+ cbindgen = resolve_cbindgen()
+ if cbindgen is None:
sys.exit(1)
- if not regenerate_headers(repo_root):
+ if not regenerate_headers(repo_root, cbindgen):
sys.exit(1)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contrib/verify_ffi.py` around lines 13 - 18, Introduce a CBINDGEN_VERSION =
"0.29.2" constant, rename check_cbindgen() to resolve_cbindgen() so it returns
the resolved cbindgen binary path (use shutil.which), run that binary with
"--version" to verify the output contains CBINDGEN_VERSION and error if
mismatched, and update regenerate_headers() to accept a cbindgen_path parameter
and use that exact binary (instead of invoking bare "cbindgen") so the same
pinned tool is reused across checks and header regeneration.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Get rid of the unnecessary build step for ffi header generation to make the ffi header/docs generation basically happening in no time compared to before where it always had to do a build before which slowed things down whenever something changed in the relevant files which is annoying in local development workflows.
Since its much faster this way I also moved the step from
pre-pushtopre-commitstage.Summary by CodeRabbit