libstore-c: add nix_store_get_fs_closure #209
Conversation
WalkthroughAdds a new C API function Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant C_API as nix_store_get_fs_closure
participant Store as Store
participant Core as computeFSClosure
participant CB as callback (optional)
Caller->>C_API: nix_store_get_fs_closure(context, store, store_path, flags, userdata, callback)
C_API->>Store: resolve underlying store
C_API->>Core: computeFSClosure(store_path, flip_direction, include_outputs, include_derivers)
Core-->>C_API: StorePathSet (closure)
alt callback provided
loop for each StorePath in closure
C_API->>CB: callback(userdata, StorePath*)
end
else no callback
Note over C_API: closure computed (no per-path callback)
end
C_API-->>Caller: return nix_err
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
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 |
src/libstore-c/nix_api_store.cc
Outdated
| NIXC_CATCH_ERRS | ||
| } | ||
|
|
||
| nix_err nix_store_compute_fs_closure( |
There was a problem hiding this comment.
I wouldn't mind calling this nix_store_get_fs_closure in the C API since the "compute" thing in the C++ API is a bit weird.
src/libstore-c/nix_api_store.cc
Outdated
|
|
||
| if (callback) { | ||
| for (const auto & path : set) { | ||
| callback(userdata, new StorePath(path)); |
There was a problem hiding this comment.
Where does this StorePath get freed? If it's the callee's responsibility we should probably mention that in the docs.
There was a problem hiding this comment.
CodeRabbit wants it to be a temp so we could just make it a temp value if that's fine, then nobody has to worry about it.
There was a problem hiding this comment.
For the use case I have, I ended up making the callee own it.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/libstore-c/nix_api_store.h (1)
220-240: Documentation improvements needed for clarity and completeness.The documentation for the new function needs several improvements:
- Missing
@returntag to document the return value- Parameter documentation lacks detail about what the boolean flags control
- Parameter order in documentation doesn't match function signature
Apply this diff to improve the documentation:
/** * @brief Computes the closure of a specific store path * * @param[out] context Optional, stores error information * @param[in] store nix store reference * @param[in] store_path The path to compute from - * @param[in] flip_direction - * @param[in] include_outputs - * @param[in] include_derivers - * @param[in] callback The function to call for every store path - * @param[in] userdata The userdata to pass to the callback + * @param[in] flip_direction If true, compute referrers instead of references + * @param[in] include_outputs If true, include the outputs of derivations in the closure + * @param[in] include_derivers If true, include derivers of store paths in the closure + * @param[in] userdata The userdata to pass to the callback + * @param[in] callback The function to call for every store path in the computed closure + * @return NIX_OK on success, or an error code if the operation fails */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/libstore-c/nix_api_store.cc(1 hunks)src/libstore-c/nix_api_store.h(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/libstore-c/nix_api_store.h (1)
src/libstore-c/nix_api_store.cc (4)
nix_store_compute_fs_closure(184-209)nix_store_compute_fs_closure(184-192)callback(132-132)callback(192-192)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build_x86_64-linux / manual
- GitHub Check: build_x86_64-linux / vm_tests_smoke
- GitHub Check: build_x86_64-linux / test
- GitHub Check: build_aarch64-darwin / build
🔇 Additional comments (1)
src/libstore-c/nix_api_store.cc (1)
200-200: computeFSClosure parameter order is correctThe API declares the flags as (bool flipDirection, bool includeOutputs, bool includeDerivers) (src/libstore/include/nix/store/store-api.hh) and provides a single-StorePath overload (src/libstore/misc.cc); the call in src/libstore-c/nix_api_store.cc passes (store_path->path, set, flip_direction, include_outputs, include_derivers) which matches that order.
RossComputerGuy
force-pushed
the
RossComputerGuy/feat/expose-computefsclosure
branch
2 times, most recently
from
3bfd2e3 to
9cb8de8
Compare
RossComputerGuy
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/libstore-c/nix_api_store.h (3)
220-233: Tighten/align Doxygen: correct param order and describe flagsDocs list callback before userdata and don’t describe the boolean flags. Align with the signature and clarify semantics.
/** - * @brief Gets the closure of a specific store path - * - * @note The callback receives ownership of each StorePath and must call nix_store_path_free() to avoid memory leaks + * @brief Compute the filesystem closure of a specific store path. + * + * The closure is enumerated via a callback; no particular iteration order is guaranteed. + * + * @note The callback receives ownership of each StorePath and must call nix_store_path_free() to avoid memory leaks. * * @param[out] context Optional, stores error information * @param[in] store nix store reference - * @param[in] store_path The path to compute from - * @param[in] flip_direction - * @param[in] include_outputs - * @param[in] include_derivers - * @param[in] callback The function to call for every store path - * @param[in] userdata The userdata to pass to the callback + * @param[in] store_path The root path to compute the closure from + * @param[in] flip_direction If true, traverse reverse references (referrers) instead of forward references + * @param[in] include_outputs If true and a derivation is encountered, include its outputs in the closure + * @param[in] include_derivers If true and an output path is encountered, include its deriver (.drv) in the closure + * @param[in] userdata Arbitrary user data passed to each callback invocation + * @param[in] callback The function called for every store path; receives ownership of the StorePath */
234-234: API name consistency: “compute” vs “get”PR objective/title calls this nix_store_compute_fs_closure and the underlying C++ API is computeFSClosure. Consider renaming for consistency or providing a stable alias to avoid confusion for downstreams (e.g., nixops4).
-nix_err nix_store_get_fs_closure( +nix_err nix_store_compute_fs_closure(If you keep the current name, please confirm the public docs and bindings use “get” consistently, or add an alias exported symbol.
234-242: Optional: introduce a named callback typeFor readability and reuse across APIs returning StorePath items, consider a typedef.
/* near the other typedefs */ typedef void (*nix_store_path_callback)(void * userdata, StorePath * store_path);Then use
nix_store_path_callbackhere instead of an inline function pointer type.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/libstore-c/nix_api_store.cc(1 hunks)src/libstore-c/nix_api_store.h(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/libstore-c/nix_api_store.cc
🧰 Additional context used
🧬 Code graph analysis (1)
src/libstore-c/nix_api_store.h (1)
src/libstore-c/nix_api_store.cc (4)
nix_store_get_fs_closure(184-209)nix_store_get_fs_closure(184-192)callback(132-132)callback(192-192)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build_aarch64-darwin / test
- GitHub Check: build_x86_64-linux / test
| void * userdata, | ||
| void (*callback)(void * userdata, const StorePath * store_path)); |
There was a problem hiding this comment.
🧩 Analysis chain
Fix ownership/const mismatch in callback parameter
Callback receives ownership but is declared with const StorePath *, which prevents calling nix_store_path_free(StorePath *) without a cast. Make the parameter non-const.
- void * userdata,
- void (*callback)(void * userdata, const StorePath * store_path));
+ void * userdata,
+ void (*callback)(void * userdata, StorePath * store_path));Please update the implementation signature in src/libstore-c/nix_api_store.cc to match.
🏁 Script executed:
#!/bin/bash
# Verify header/impl callback constness and function name usage.
rg -n -C2 $'nix_store_get_fs_closure\\s*\\('
rg -n $'\\(\\*callback\\)\\(void \\* userdata, const StorePath \\* store_path\\)' src/libstore-c/nix_api_store.* || trueLength of output: 798
Fix ownership/const mismatch in callback parameter
Callback receives ownership but is declared const StorePath *, which prevents calling nix_store_path_free(StorePath *) without a cast — make the parameter non-const in both header and implementation.
Locations: src/libstore-c/nix_api_store.h:242, src/libstore-c/nix_api_store.cc:192
- void * userdata,
- void (*callback)(void * userdata, const StorePath * store_path));
+ void * userdata,
+ void (*callback)(void * userdata, StorePath * store_path));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void * userdata, | |
| void (*callback)(void * userdata, const StorePath * store_path)); | |
| void * userdata, | |
| void (*callback)(void * userdata, StorePath * store_path)); |
🤖 Prompt for AI Agents
In src/libstore-c/nix_api_store.h around line 242 and
src/libstore-c/nix_api_store.cc around line 192 the callback parameter is
declared as const StorePath * but the callback takes ownership and must call
nix_store_path_free(StorePath *), so change the callback signature in the header
from const StorePath * to StorePath * and update the matching implementation
signature and any forward declarations to use StorePath *; remove any const
casts inside the implementation, ensure callers and tests treat the StorePath as
owned (call nix_store_path_free when done), and update any related comments or
docs to reflect ownership semantics, then rebuild to verify no const-related
warnings remain.
9cb8de8 to
76afe59
Compare
RossComputerGuy
force-pushed
the
RossComputerGuy/feat/expose-computefsclosure
branch
from
76afe59 to
a6eb590
Compare
Motivation
Exposes the nix store API's
computeFSClosurefunction inside the C API asnix_store_compute_fs_closure.Context
This is useful for expanding the nixops4 Rust bindings so we're able to look into a store path's closure.
Summary by CodeRabbit