Skip to content

chore(cbind): nim-ffi migration [2/9] — new-library scaffold + CI#2773

Open
gmelodie wants to merge 12 commits into
masterfrom
chore/cbind/ffi/02-scaffold
Open

chore(cbind): nim-ffi migration [2/9] — new-library scaffold + CI#2773
gmelodie wants to merge 12 commits into
masterfrom
chore/cbind/ffi/02-scaffold

Conversation

@gmelodie

@gmelodie gmelodie commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

nim-ffi cbind migration — PR train

  1. chore(cbind): nim-ffi migration [1/9] — deps + pinning scaffold #2772 — deps + pinning
  2. chore(cbind): nim-ffi migration [2/9] — new-library scaffold + CI #2773 — scaffold + CI ← this PR
  3. chore(cbind): nim-ffi migration [3/9] — connectivity & identity #2774 — connectivity
  4. chore(cbind): nim-ffi migration [4/9] — streams & custom protocols (+ echo example) #2775 — streams (+ echo)
  5. chore(cbind): nim-ffi migration [5/9] — pubsub / gossipsub (+ gossipsub example) #2776 — pubsub (+ gossipsub)
  6. chore(cbind): nim-ffi migration [6/9] — kademlia + CID + crypto #2777 — kad + CID + crypto
  7. chore(cbind): nim-ffi migration [7/9] — service discovery + extended peer records #2778 — service discovery
  8. chore(cbind): nim-ffi migration [8/9] — relay, peerstore, metrics (API parity) #2779 — relay / peerstore / metrics
  9. chore(cbind): nim-ffi migration [9/9] — flip to libp2p.nim, delete legacy cbind #2780 — flip + delete legacy

Part 2 of a 9-PR train that replaces the hand-rolled cbind/ C bindings with the generated nim-ffi codegen, landing it domain by domain so each PR stays reviewable. Targets chore/cbind/ffi/01-deps-pinning (stacked).

This PR adds

  • cbind/libp2p_ffi.nim, the new nim-ffi library, developed in parallel to the legacy cbind/libp2p.nim so the old bindings keep shipping until the final flip. It carries the library state (LibP2P, StreamRegistry), declareLibrary, the MaxReadBytes read-ceiling reserved for the stream reads landing in a later PR, the node builder, the lifecycle entry points, and genBindings().
  • The {.ffi.} config types (Libp2pConfig, BootstrapNode, the transport/muxer enums) and their parsing live in cbind/libp2p_ffi/config.nim. config.parse() validates the raw config once and returns a ParsedConfig of ready-to-use Nim values, so the node builder never touches a raw string or ordinal. The file is included rather than imported: nim-ffi reads {.ffi.} object fields from the AST, and exporting the fields would leak their * into the generated field names.
  • Protocol mounting orchestrated by mountProtocols, with Kademlia and service discovery mounting through separate helpers instead of one function branching on config flags.
  • buildffi/genbindings_c/genbindings_cddl nimble tasks, a temporary .#cbind-ffi nix target (which reuses the repo Makefile's nat_libs target), and a temporary c-bindings-ffi CI job — all pointed at the new file. The old .#cbind target and job are untouched.

Lifecycle & shutdown

  • libp2pNew / libp2pStart / libp2pStop / libp2pDestroy. libp2pDestroy owns graceful shutdown: it stops the switch on the FFI worker loop before the threads are joined and the context is freed, so destroy alone is sufficient. libp2pStop is a thin, idempotent wrapper over the same shutdownSwitch path, kept for an explicit, error-reporting stop. A stopped flag makes stop-then-destroy a no-op.
  • This relies on nim-ffi's async teardown hook for {.ffiDtor.}; the ffi dep is re-pinned to feat(ffi): async teardown hook for {.ffiDtor.} logos-messaging/nim-ffi#115 (d1575f0). The FFI build compiles with -d:ffiThreadExitTimeoutMs=5000 to give a graceful switch.stop() room beyond the 1500 ms default before the context is leaked.
  • libp2p_new_default_config() is preserved for hosts that still call it, as a hand-maintained C-ABI mirror struct (the {.ffi.} config crosses as CBOR, not a C struct, so it isn't in the generated bindings).

CI status

  • The c-bindings-ffi job runs on macOS (macos-15 / arm64 / clang) but is currently disabled (if: false): libp2p_ffi.nim is built up domain by domain across PRs 2–9 and only compiles end-to-end at the flip, so a green run isn't meaningful until then. It's re-enabled (drop if: false) in the final PR. The legacy c-bindings job is unchanged and stays green throughout.

Review artifact: the generated libp2p.h appears for the first time here, with just the lifecycle entry points.

@codecov-commenter

codecov-commenter commented Jul 7, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.31%. Comparing base (c1314b6) to head (21d09a0).
⚠️ Report is 2 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2773      +/-   ##
==========================================
- Coverage   81.32%   81.31%   -0.01%     
==========================================
  Files         171      171              
  Lines       31013    31014       +1     
  Branches       12       12              
==========================================
- Hits        25221    25220       -1     
- Misses       5792     5794       +2     

see 7 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@gmelodie gmelodie force-pushed the chore/cbind/ffi/02-scaffold branch from 36ae5a8 to 0b3ac56 Compare July 7, 2026 19:00
@gmelodie gmelodie force-pushed the chore/cbind/ffi/01-deps-pinning branch from 5ed8fb3 to c656e20 Compare July 7, 2026 19:04
@gmelodie gmelodie mentioned this pull request Jul 7, 2026
2 tasks
@gmelodie gmelodie force-pushed the chore/cbind/ffi/02-scaffold branch 2 times, most recently from 3377a04 to 3d864f5 Compare July 7, 2026 19:39
@gmelodie gmelodie marked this pull request as ready for review July 7, 2026 19:40
@gmelodie gmelodie requested a review from a team as a code owner July 7, 2026 19:40
@gmelodie gmelodie requested review from richard-ramos and vladopajic and removed request for a team July 7, 2026 19:40
@gmelodie gmelodie force-pushed the chore/cbind/ffi/01-deps-pinning branch from c656e20 to 8b39fc1 Compare July 7, 2026 19:47
@richard-ramos richard-ramos requested a review from Copilot July 7, 2026 19:48

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR introduces the initial scaffold for a new nim-ffi-based C bindings library (cbind/libp2p_ffi.nim) and wires it into Nix and GitHub Actions so it can be built and have C/CDDL bindings generated in parallel with the legacy cbind/ flow during the migration train.

Changes:

  • Add a new nim-ffi library module (cbind/libp2p_ffi.nim) with lifecycle entry points and config parsing (cbind/libp2p_ffi/config.nim).
  • Add temporary Nix/flake packaging to build the new FFI library and generate/install headers + CDDL (nix/cbind-ffi.nix, flake.nix).
  • Extend the cbind nimble file and CI workflow with parallel tasks/jobs for building and generating bindings (cbind/cbind.nimble, .github/workflows/cbindings.yml).

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
nix/cbind-ffi.nix Adds a temporary Nix derivation to build the nim-ffi shared/static libs and generate/install C + CDDL outputs.
flake.nix Exposes a temporary .#cbind-ffi flake package target for the new derivation.
cbind/libp2p_ffi/config.nim Defines {.ffi.} config shapes and parsing into validated, typed ParsedConfig.
cbind/libp2p_ffi.nim Adds the new nim-ffi library state, protocol mounting, node builder, lifecycle FFI entry points, and genBindings().
cbind/cbind.nimble Adds nimble tasks for building the new FFI library and generating C/CDDL bindings.
cbind/.gitignore Ignores nim-ffi build caches and generated bindings output directories under cbind/.
.github/workflows/cbindings.yml Adds a parallel CI job to build the new nim-ffi library and generate C/CDDL bindings.

Comment thread cbind/libp2p_ffi.nim
Comment thread cbind/libp2p_ffi/config.nim Outdated
Comment thread cbind/libp2p_ffi/config.nim Outdated
Comment thread .github/workflows/cbindings.yml Outdated
@github-project-automation github-project-automation Bot moved this from new to In Progress in nim-libp2p Jul 7, 2026
Comment thread .github/workflows/cbindings.yml Outdated
Comment thread cbind/libp2p_ffi.nim Outdated
return err("could not create libp2p node: " & e.msg)

proc libp2pDestroy*(lib: LibP2P) {.ffiDtor.} =
discard

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous destroy 'waited' for the thread to return before returning. Does this do the same or just gets rid of lib? because if so, maybe we should defensively run a libp2pStop here?

@gmelodie gmelodie Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! Implemented a dtor hook that we can use here to make destroy call stop (needs this because destroy and stop are on different threads). Now they both call shutdown.

Comment thread .github/workflows/cbindings.yml
Comment thread cbind/libp2p_ffi.nim Outdated
Comment thread cbind/libp2p_ffi.nim Outdated
Comment thread cbind/libp2p_ffi.nim Outdated
Comment thread cbind/libp2p_ffi/config.nim
Base automatically changed from chore/cbind/ffi/01-deps-pinning to master July 8, 2026 17:29
@gmelodie gmelodie requested review from a team and richard-ramos July 8, 2026 17:29
@gmelodie gmelodie force-pushed the chore/cbind/ffi/02-scaffold branch from 8df4348 to b5f5891 Compare July 8, 2026 18:15
gmelodie and others added 10 commits July 8, 2026 15:42
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@gmelodie gmelodie force-pushed the chore/cbind/ffi/02-scaffold branch from 21d09a0 to 502d422 Compare July 8, 2026 20:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

5 participants