Repository navigation
Add initial C FFI bindings and error inspection - #5618
Conversation
github-actions
Bot
added
C-Dependencies
Test262 conformance changes
Tested main commit: |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5618 +/- ##
===========================================
+ Coverage 47.24% 63.16% +15.92%
===========================================
Files 476 538 +62
Lines 46892 60609 +13717
===========================================
+ Hits 22154 38286 +16132
+ Misses 24738 22323 -2415 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
jedel1043
left a comment
jedel1043
left a comment
There was a problem hiding this comment.
Implementation-wise looks great! I especially like how you represented JsError through the FFI without losing many of its semantics.
I have a couple of comments but nothing too critical.
| - name: Check generated C headers | ||
| if: ${{ matrix.os == 'ubuntu-latest' }} | ||
| run: | | ||
| cargo run --locked --profile ci -p diplomat-gen | ||
| git status --short --untracked-files=all -- ffi/boa_ffi/c/generated/ | ||
| test -z "$(git status --porcelain --untracked-files=all -- ffi/boa_ffi/c/generated/)" | ||
|
|
||
| - name: Test C bindings | ||
| if: ${{ matrix.os != 'windows-latest' }} | ||
| run: ./ffi/boa_ffi/c/tests/run.sh --ci |
There was a problem hiding this comment.
If these only work on Linux and MacOS, it might be worth extracting this into its own job and having a separate matrix for them instead.
There was a problem hiding this comment.
Yep, I've moved both steps into their own Test FFI job with a separate matrix (same runners as the main test job, minus Windows). The header check used to only run on ubuntu-latest, now it runs on all three.
|
|
||
| fn main() -> io::Result<()> { | ||
| let crate_dir = Path::new(env!("CARGO_MANIFEST_DIR")).join("../../ffi/boa_ffi"); | ||
| let headers = crate_dir.join("c/generated"); |
There was a problem hiding this comment.
| let headers = crate_dir.join("c/generated"); | |
| let headers = crate_dir.join("bindings/c"); |
Let's use the same nomenclature that we use on temporal_rs
There was a problem hiding this comment.
Thanks, changed it! I've also moved the C tests from c/tests to tests/c and copied temporal_capi's .gitattributes (marks bindings/ as generated) so the layout matches temporal_rs now.
| diplomat = "=0.15.0" | ||
| diplomat-runtime = "=0.15.1" | ||
| diplomat-tool = "=0.15.0" |
There was a problem hiding this comment.
Let's use the same versioning that we use on temporal_rs:
diplomat-tool = { version = "0.16.0", default-features = false }
diplomat-runtime = { version = ">= 0.15.2, < 0.17.0", default-features = false }
diplomat = { version = "0.16.0", default-features = false }
There was a problem hiding this comment.
Switched to those! Cargo.lock resolves them to 0.16.1 and the regenerated headers came out identical. I also merged main in, since the auto-merged lockfile still had askama_parser pointing at winnow 1.0.4 after main moved to 1.0.5 and that broke --locked. The only extra change in the merge commit is that line.
Match temporal_capi: `#[diplomat::abi_rename = "boa_{0}"]` prefixes every
exported C function to avoid symbol clashes with other libraries, and the
crate now forbids `unwrap`, `expect` and indexing outside tests, because a
panic in an `extern "C"` function aborts the host program. Also drop the
stale Diplomat version from a lint reason.
jedel1043
left a comment
jedel1043
left a comment
There was a problem hiding this comment.
Thank you so much for working on this! Looks great!
github-actions
Bot
removed
the
Waiting On Review
This Pull Request is related to #332, adding the initial Diplomat-based
boa_fficrate modeled ontemporal_capiand theJsErrorC API.eval,try_nativenormalization, and enabling engine features (likeffi/wasmdoes) will follow in the next PR.It changes the following:
ffi/boa_ffiandtools/diplomat-gen(publish = false) with generated C headers inffi/boa_ffi/bindings/c/. Exported C functions use aboa_prefix, liketemporal_capi.BoaContext, minimalJsValuehandles for thrown values, context-free construction ofNativeandOpaqueerrors, and inspection of all threeJsErrorvariants (Native,Opaque,Engine).ffi/boa_ffi/README.md.ffi/boa_ffi/tests/c/(run byrun.sh), and aTest FFICI job on macOS and Linux that checks header freshness and runs the C tests.boa_engineis untouched. Most of the diff comes from generated C headers,Cargo.lock, docs, and tests, while the bridge itself inffi/boa_ffi/src/lib.rsis ~250 lines.