nng-sys: upgrade to NNG 2.0.0-alpha.7#40
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:
📝 WalkthroughWalkthroughUpdated CI to use vendored features for anng, removed a local path dependency from anng, bumped nng-sys version and advanced the nng submodule, added extensive FFI binding changes in nng-sys (TLS cert APIs, HTTP/address getters, const-correct string getters, option constants), and added a local .claude settings file. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
nng-sys/src/bindings.rs (2)
1-1:⚠️ Potential issue | 🔴 CriticalPipeline failure: pre-generated bindings are out of sync.
The CI reports that
src/bindings.rsdoesn't match the output of bindgen. This file needs to be regenerated (e.g., viacargo build --features source-update-bindings) and committed.
593-599:⚠️ Potential issue | 🔴 CriticalBreaking signature change:
get_stringfunctions now return*constinstead of*mut.
nng_dialer_get_string,nng_listener_get_string,nng_pipe_get_string,nng_stream_get_string, and related stream dialer/listener variants now havearg3: *mut *const c_charinstead of*mut *mut c_char. This reflects upstream's change to return const pointers (the caller should not free/modify the returned string).The trait definition in
nng/src/options/private.rs:52-56must be updated to match, as it still declares*mut *mut c_char. This trait definition mismatch will cause compilation errors for all implementations until corrected.
🤖 Fix all issues with AI agents
In `@nng-sys/nng`:
- Line 1: The NNG submodule commit hash referenced
(3089394da9014b8130c00da6e3dc4ddaca8f0ba1) does not exist; update the submodule
entry to point to a valid NNG v2.0.0-alpha.7 identifier by checking the upstream
repo for the correct commit or tag (e.g., the tag v2.0.0-alpha.7), replace the
invalid hash in the submodule reference in nng-sys/nng with the correct commit
or tag, then run git submodule sync && git submodule update --init --recursive
to verify and fetch the correct commit.
In `@nng-sys/src/bindings.rs`:
- Line 26: The codebase still references the removed constants
NNG_OPT_TCP_BOUND_PORT and NNG_OPT_UDP_BOUND_PORT; update any usages to the new
NNG_OPT_BOUND_PORT constant (replace occurrences of NNG_OPT_TCP_BOUND_PORT and
NNG_OPT_UDP_BOUND_PORT with NNG_OPT_BOUND_PORT) so calls in modules that read
bound port options (e.g., the option parsing in types.rs and pipe handling in
pipes.rs) compile against the new byte-string constant b"bound-port\0"; ensure
any logic that expected separate TCP/UDP keys still works with the single key
and adjust tests or matching branches accordingly.
|
|
||
| [dependencies.nng-sys] | ||
| version = "0.4.0-v2pre.1" | ||
| path = "../nng-sys" |
There was a problem hiding this comment.
Let's leave a TODO here as well
flxo
force-pushed
the
nng-sys/v2.0.0-alpha.7
branch
from
a23adbe to
f3a9339
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
nng-sys/src/bindings.rs (1)
593-599:⚠️ Potential issue | 🟠 MajorBreaking change:
get_stringfunctions now return*constpointers.
nng_dialer_get_string(line 597) andnng_listener_get_string(line 712) now write to*mut *const c_charinstead of*mut *mut c_char. This means callers can no longer free the returned string — it's now a borrowed pointer owned by NNG. Any existing code that callsnng_strfreeon these results will be incorrect (double-free or freeing a non-owned pointer).#!/bin/bash # Find callers that might be freeing strings from get_string calls rg -n 'get_string|nng_strfree' --type rust -C3Also applies to: 708-714
🤖 Fix all issues with AI agents
In `@nng-sys/src/bindings.rs`:
- Around line 954-956: The binding for nng_aio_finish now expects nng_err (a
#[repr(transparent)] wrapper around c_uint) instead of c_int; update all call
sites that pass raw integers to construct/convert into nng_err before calling
nng_aio_finish and update any wrapper functions that forward an error value
(e.g., in your anng/nng higher-level wrappers) to accept/return nng_err or
convert signed c_int to nng_err explicitly; search for usages of nng_aio_finish
and change the argument handling so the ABI type matches the new binding (ensure
any helper functions that previously took c_int are adjusted to create an
nng_err wrapper).
🧹 Nitpick comments (1)
nng-sys/src/bindings.rs (1)
185-190:Defaultderive on opaque FFI typenng_tls_cert_sis misleading.
nng_tls_cert_sis an opaque type (zero-sized_unused: [u8; 0]) that should only be created by NNG's C functions (nng_tls_cert_parse_pem, etc.). DerivingDefaultsuggests it can be meaningfully default-constructed in Rust, which it cannot — the result would be an invalid handle. This is auto-generated by bindgen so it's a cosmetic concern, but worth noting for safe wrapper authors.
flxo
force-pushed
the
nng-sys/v2.0.0-alpha.7
branch
from
7efbfbb to
106556e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
nng-sys/src/bindings.rs (1)
593-599:⚠️ Potential issue | 🔴 CriticalCritical:
get_stringfunctions now return non-owning*const c_charbut wrapper code was not updated.All
*_get_stringfunctions (nng_dialer_get_string,nng_listener_get_string,nng_pipe_get_string,nng_stream_get_string,nng_stream_dialer_get_string,nng_stream_listener_get_string) changed from*mut *mut c_charto*mut *const c_charin the bindings, reflecting a change from owned/strdup'd strings to borrowed references into NNG internals. However,nng/src/options/private.rshas not been updated—the trait definition still declares*mut *mut c_charand thegetopt_string()implementation still callsnng_strfree()on the returned pointer.Fix the trait definition to expect
*mut *const c_charand remove thenng_strfree()call fromgetopt_string(). The newnng_pipe_get_strdupprovides the old owned-string behavior when needed.
🤖 Fix all issues with AI agents
In `@nng-sys/src/bindings.rs`:
- Line 26: Downstream references to the removed NNG_OPT_TCP_BOUND_PORT and
NNG_OPT_UDP_BOUND_PORT must be replaced with the new NNG_OPT_BOUND_PORT constant
and its new option string; update any code that uses NNG_OPT_TCP_BOUND_PORT or
NNG_OPT_UDP_BOUND_PORT (for example in the option handling at types.rs:556 and
pipe code at pipes.rs:265) to use NNG_OPT_BOUND_PORT instead and remove any code
that assumed the former "tcp-bound-port\0"/"udp-bound-port\0" names so the
option lookup uses the new "bound-port\0" string exported by NNG_OPT_BOUND_PORT.
- Around line 60-83: The bundled tm definition in bindings.rs declares
non-portable fields (tm_gmtoff, tm_zone) causing ABI mismatches on Windows when
calling nng_tls_cert_not_before / nng_tls_cert_not_after; replace the local tm
struct by using the platform-aware type from the libc crate (e.g., ::libc::tm)
in all FFI signatures and remove the manual tm declaration, or if you prefer
per-platform control, wrap the extra fields and the tm definition behind
appropriate #[cfg(target_os = "...")] gates so Windows uses the libc::tm layout;
ensure all references to tm in bindings.rs and the FFI functions
nng_tls_cert_not_before / nng_tls_cert_not_after are updated to the chosen
approach.
63c5f2a to
9f27647
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
nng-sys/src/bindings.rs (2)
1-1:⚠️ Potential issue | 🔴 CriticalCI failure: pre-generated bindings are out of sync with bindgen output.
The
sys-bindingspipeline is failing because this checked-in file doesn't match whatbindgenproduces in CI. Regenerate the bindings with the same bindgen/clang version used in CI and commit the result.
593-599:⚠️ Potential issue | 🟠 MajorBreaking semantic change:
*_get_stringbindings now return*const c_char(borrowed) instead of*mut c_char(owned), but wrapper code still callsnng_strfree.The trait definition in
nng/src/options/private.rs(line 52-56) declaresGETOPT_STRINGwith the old signature*mut *mut c_char, while the bindings innng-sys/src/bindings.rsnow return*mut *const c_char. Thegetopt_stringimplementation (lines 128-132) unconditionally callsnng_sys::nng_strfree(ptr.as_ptr())on the returned pointer, which is now borrowed from nng and must not be freed. This will cause undefined behavior.Update the wrapper:
- Change
GETOPT_STRINGtrait signature inHasOptsto use*mut *const c_char- Remove the
nng_strfreecall ingetopt_string- Update all implementations in
socket.rs,pipe.rs,listener.rs,dialer.rs, andctx.rsto matchFor callers needing owned copies, use the new
nng_pipe_get_strdupvariant.
🤖 Fix all issues with AI agents
In @.claude/settings.local.json:
- Line 13: Remove the corrupted permission entry that contains the embedded git
commit message (the string beginning with "Bash(static library nng\"
errors.\n\nExplicitly set CMAKE_INSTALL_LIBDIR to lib...") from the settings
JSON, ensuring you delete the entire offending string/value and any trailing
comma so the JSON remains valid; if a legitimate permission rule is required,
replace it with the intended, well-formed permission pattern, then validate the
file with a JSON linter and rerun the config to confirm no syntax errors.
- Around line 1-25: Remove the developer-specific file
.claude/settings.local.json from the PR and stop tracking it in git: delete the
file from the commit, add an entry to .gitignore to exclude
.claude/settings.local.json (or the entire .claude/*.local pattern), and commit
the .gitignore change; if a shared .claude/settings.json is intended, ensure
that canonical non-local settings exist and are the only .claude file tracked.
- Around line 10-12: Remove the hardcoded absolute path strings found in the
Bash(...) entries (e.g., "Bash(git -C /Users/felix.obenhuber/palau/nng/nng-rs
...)") and replace them with portable alternatives: use relative paths (e.g.,
"Bash(git -C nng-rs ...)" or workspace variables like
"${workspaceFolder}/nng-rs"), or drop these user-specific commands from version
control and keep them in a local, gitignored config; update the entries so no
username or absolute filesystem path is committed.
Update bindings to reflect upstream API changes: - Consolidate bound port options into NNG_OPT_BOUND_PORT - Add TLS certificate type and inspection functions - Add dedicated pipe/stream address functions (peer_addr, self_addr) - Add WebSocket header iteration options - Change get_string functions to return const pointers - Add HTTP address and header iteration functions
flxo
force-pushed
the
nng-sys/v2.0.0-alpha.7
branch
from
9f27647 to
d9795a1
Compare
2 participants
Bump NNG to v2.0.0-alpha.7. Disable the
anngCI check with vendored NNG sinceanngis not yet upgraded toalpha.7.Summary by CodeRabbit
New Features
API Changes
Deprecations