chore(ffi): move header files generation to the target directory#579
chore(ffi): move header files generation to the target directory#579
Conversation
📝 WalkthroughWalkthroughThe changes refactor FFI build infrastructure for two Rust crates by redirecting generated C header output from local Changes
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 docstrings
🧪 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 #579 +/- ##
=============================================
+ Coverage 66.32% 66.85% +0.52%
=============================================
Files 311 314 +3
Lines 64976 65111 +135
=============================================
+ Hits 43097 43529 +432
+ Misses 21879 21582 -297
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@dash-spv-ffi/build.rs`:
- Around line 5-21: The generated header path uses the hyphenated package name
from CARGO_PKG_NAME which breaks C includes; create a library-safe name (e.g.,
let lib_name = crate_name.replace("-", "_")) and use lib_name when building
include_dir and output_path (replace uses of crate_name in include_dir and
output_path with lib_name) so the produced header is named dash_spv_ffi.h and
placed under target/.../include/dash_spv_ffi/.
In `@key-wallet-ffi/build.rs`:
- Around line 5-21: The generated header path uses CARGO_PKG_NAME (crate_name)
which contains hyphens; convert the package name to the library-style name by
replacing '-' with '_' (e.g., lib_name = crate_name.replace('-', '_')) and use
that lib_name when constructing include_dir and output_path (and any other
places that rely on the header filename) so the produced header is
key_wallet_ffi.h and the include directory uses key_wallet_ffi; keep
fs::create_dir_all(&include_dir) and subsequent logic unchanged except for using
lib_name.
In `@key-wallet-ffi/Cargo.toml`:
- Around line 35-36: Update the pull request title to include an allowed
semantic prefix (e.g., "build:") so it conforms to the pr-title.yml workflow;
change the current title "move header files generation to the target directory"
to "build: move header files generation to the target directory" (this PR only
adjusts header-generation/build plumbing related to cbindgen in Cargo.toml) so
the semantic-pull-request check accepts the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d163ca0a-b161-4cde-8934-34b29c1f25ad
📒 Files selected for processing (7)
.gitignoredash-spv-ffi/Cargo.tomldash-spv-ffi/build.rsdash-spv-ffi/cbindgen.tomlkey-wallet-ffi/Cargo.tomlkey-wallet-ffi/build.rskey-wallet-ffi/cbindgen.toml
💤 Files with no reviewable changes (3)
- .gitignore
- key-wallet-ffi/cbindgen.toml
- dash-spv-ffi/cbindgen.toml
ZocoLini
force-pushed
the
chore/header-generation
branch
from
40ccaa2 to
65528b2
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
dash-spv-ffi/build.rs (1)
5-5:⚠️ Potential issue | 🟠 MajorUse the library-safe header name here, not
CARGO_PKG_NAME.Line 5 pulls the hyphenated package name, and Lines 17-21 reuse it for both the include directory and header filename. That generates
dash-spv-ffi.h, while the C-facing name for this crate isdash_spv_ffi, so the moved header path no longer matches current consumers.Suggested fix
- let crate_name = env::var("CARGO_PKG_NAME").unwrap(); + let crate_name = env::var("CARGO_PKG_NAME").expect("Missing CARGO_PKG_NAME"); + let header_name = crate_name.replace('-', "_"); @@ - let include_dir = target_dir.join("include").join(&crate_name); + let include_dir = target_dir.join("include").join(&header_name); @@ - let output_path = include_dir.join(format!("{}.h", &crate_name)); + let output_path = include_dir.join(format!("{header_name}.h"));Run this to confirm the package/lib names and the header spellings still referenced by the C surface:
#!/bin/bash set -euo pipefail # Expected: package.name is hyphenated, while lib.name and C consumers use the underscored form. python - <<'PY' import pathlib, tomllib cargo = pathlib.Path("dash-spv-ffi/Cargo.toml") data = tomllib.loads(cargo.read_text()) print("package.name =", data["package"]["name"]) print("lib.name =", data.get("lib", {}).get("name")) PY echo rg -n -C1 --glob '*.c' --glob 'README*' 'dash[_-]spv[_-]ffi\.h' .Also applies to: 17-21
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash-spv-ffi/build.rs` at line 5, The build script currently uses the hyphenated package name (crate_name from env::var("CARGO_PKG_NAME")) for the generated C header and include directory; change it to a library-safe identifier by normalizing hyphens to underscores (e.g., let header_name = crate_name.replace('-', "_")) and use header_name for the header filename and include dir (the places where crate_name is reused around the header generation/installation logic, formerly lines ~17-21). Ensure the header produced is "dash_spv_ffi.h" (underscore form) and any include path uses the same normalized name so C consumers keep the expected symbol.
🧹 Nitpick comments (1)
dash-spv-ffi/build.rs (1)
12-21: Extract theOUT_DIRpath mapping into a testable helper.The
ancestors().nth(3)walk is the new behavior in this PR and it is easy to regress for plain vs target-triple layouts. Pulling that mapping into a small helper with#[cfg(test)]cases would make future path changes much safer. As per coding guidelines, "Write unit tests for new functionality in Rust code" and "Place unit tests alongside code with#[cfg(test)]attribute".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash-spv-ffi/build.rs` around lines 12 - 21, Extract the ancestor-walking logic that turns OUT_DIR into the target directory into a small helper function (e.g. fn resolve_target_dir_from_out_dir(out_dir: &str) -> PathBuf) and use that helper where target_dir is computed instead of calling .ancestors().nth(3) inline; add unit tests alongside that helper with #[cfg(test)] to cover the expected layouts (plain target/<PROFILE> and target/<triple>/<PROFILE>) to assert the returned PathBuf, and update usages in build.rs to call the helper with the existing out_dir and keep include_dir, fs::create_dir_all and output_path logic unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@dash-spv-ffi/build.rs`:
- Line 5: The build script currently uses the hyphenated package name
(crate_name from env::var("CARGO_PKG_NAME")) for the generated C header and
include directory; change it to a library-safe identifier by normalizing hyphens
to underscores (e.g., let header_name = crate_name.replace('-', "_")) and use
header_name for the header filename and include dir (the places where crate_name
is reused around the header generation/installation logic, formerly lines
~17-21). Ensure the header produced is "dash_spv_ffi.h" (underscore form) and
any include path uses the same normalized name so C consumers keep the expected
symbol.
---
Nitpick comments:
In `@dash-spv-ffi/build.rs`:
- Around line 12-21: Extract the ancestor-walking logic that turns OUT_DIR into
the target directory into a small helper function (e.g. fn
resolve_target_dir_from_out_dir(out_dir: &str) -> PathBuf) and use that helper
where target_dir is computed instead of calling .ancestors().nth(3) inline; add
unit tests alongside that helper with #[cfg(test)] to cover the expected layouts
(plain target/<PROFILE> and target/<triple>/<PROFILE>) to assert the returned
PathBuf, and update usages in build.rs to call the helper with the existing
out_dir and keep include_dir, fs::create_dir_all and output_path logic
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 767bc8fb-f1fa-452c-9683-a1c4e57c0abc
📒 Files selected for processing (6)
.gitignoredash-spv-ffi/Cargo.tomldash-spv-ffi/build.rsdash-spv-ffi/cbindgen.tomlkey-wallet-ffi/build.rskey-wallet-ffi/cbindgen.toml
💤 Files with no reviewable changes (3)
- .gitignore
- key-wallet-ffi/cbindgen.toml
- dash-spv-ffi/cbindgen.toml
✅ Files skipped from review due to trivial changes (2)
- dash-spv-ffi/Cargo.toml
- key-wallet-ffi/build.rs
ZocoLini
changed the title
ZocoLini
force-pushed
the
chore/header-generation
branch
from
65528b2 to
8cbae41
Compare
|
@dashinfraclaw fulltest |
4 similar comments
|
@dashinfraclaw fulltest |
|
@dashinfraclaw fulltest |
|
@dashinfraclaw fulltest |
|
@dashinfraclaw fulltest |
|
🔧 Infraclaw fulltest results for PR #579
Root cause: pre-existing, not caused by this PR. Platform references This means no rust-dashcore PR can pass fulltest against platform until platform updates its dependency from Automated integration test by @dashinfraclaw |
3 participants
With this PR I make the build.rs scripts ALWAYS generate the headers and, I change the location to the target directory.
Why I made this?? In the swift-sdk, the build script was making a mess to get the headers and merge all the libraries into one, to fix that I need rust-dashcode to generate the headers in the target directory, that way I can grab the headers from there without having to clone this repo.
Also, the dash-spv headers and key_wallet headers used a
_in the file name but now they use-because I just read the crate's name in build.rs script and use that to name the files, let me know if you think we shouldn't do that.Othe changes I made are:
Summary by CodeRabbit