Add binding for nix_store_build_paths#13
Conversation
📝 WalkthroughWalkthroughAdds a new public method Changes
Sequence DiagramsequenceDiagram
participant Caller
participant Store as Store::build_paths
participant Callback as "extern \"C\" callback"
participant FFI as raw::store_build_paths
participant Native as NativeStore
Caller->>Store: build_paths(&paths)
activate Store
Store->>Store: create BTreeMap outputs
Store->>Callback: register extern "C" callback (user_data -> map)
Store->>Store: convert &StorePath -> raw pointers
Store->>FFI: call raw::store_build_paths(paths, callback, user_data)
activate FFI
FFI->>Native: perform builds
activate Native
Native->>Callback: invoke callback with (path, result, user_data)
activate Callback
Callback->>Store: append path->result into map via user_data
deactivate Callback
deactivate Native
deactivate FFI
Store->>Caller: return BTreeMap<String, String>
deactivate Store
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@nix-bindings-store/src/store.rs`:
- Around line 392-393: The build_paths method must be gated with the same
feature flag as the BTreeMap import to avoid compile errors on Nix 2.26–2.30;
add #[cfg(nix_at_least = "2.31")] above the pub fn build_paths(&mut self, paths:
&[&StorePath]) -> Result<BTreeMap<String, String>> definition so the method
(like realise) is only compiled when nix_at_least = "2.31" is enabled, ensuring
the BTreeMap type and related code are present when build_paths is compiled.
60ee025 to
cd1ddad
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
nix-bindings-store/src/store.rs (2)
401-418: Inline callback is inconsistent with the rest of the file — consider extracting.Every other callback in this file (
callback_get_result_store_path_set,callback_get_result_derivation,callback_make_drv_outputs) is defined at module level with a companion*_datahelper. The inline definition here works but breaks the established pattern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nix-bindings-store/src/store.rs` around lines 401 - 418, Extract the inline unsafe extern "C" fn callback into a module-level function following the existing pattern (e.g., name it callback_get_result_map) and add a companion helper (e.g., callback_get_result_map_data) to create/pack the userdata pointer from &mut BTreeMap<String, String>; move the current body (converting path/result CStrs and inserting into the map) verbatim into that new function and replace the inline closure call site to pass callback_get_result_map as the function pointer and callback_get_result_map_data(...) as userdata so it matches callback_get_result_store_path_set / callback_get_result_derivation / callback_make_drv_outputs conventions.
392-437: No test coverage forbuild_paths.All other gated store methods (
realise,derivation_from_json,add_derivation,get_fs_closure) have corresponding tests in this file.build_pathshas none, making it difficult to verify correctness of the callback wiring and argument passing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nix-bindings-store/src/store.rs` around lines 392 - 437, Add a test for Store::build_paths that mirrors the existing tests for realise/derivation_from_json/add_derivation/get_fs_closure: create a Store and one or more StorePath fixtures used elsewhere in the test file, call build_paths(&[&path1, &path2...]) under the same #[cfg(nix_at_least = "2.31")] gate, and assert the returned BTreeMap contains expected keys and values (verify the C callback wiring by checking that the inserted map entries match the supplied StorePath strings and expected result strings). Ensure the test uses the same setup/teardown helpers in this module (context/Store construction) and exercises the path pointer conversion (StorePath::as_ptr()) path so the unsafe callback path and check_call!(raw::store_build_paths(...)) invocation are covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@nix-bindings-store/src/store.rs`:
- Around line 401-418: Extract the inline unsafe extern "C" fn callback into a
module-level function following the existing pattern (e.g., name it
callback_get_result_map) and add a companion helper (e.g.,
callback_get_result_map_data) to create/pack the userdata pointer from &mut
BTreeMap<String, String>; move the current body (converting path/result CStrs
and inserting into the map) verbatim into that new function and replace the
inline closure call site to pass callback_get_result_map as the function pointer
and callback_get_result_map_data(...) as userdata so it matches
callback_get_result_store_path_set / callback_get_result_derivation /
callback_make_drv_outputs conventions.
- Around line 392-437: Add a test for Store::build_paths that mirrors the
existing tests for realise/derivation_from_json/add_derivation/get_fs_closure:
create a Store and one or more StorePath fixtures used elsewhere in the test
file, call build_paths(&[&path1, &path2...]) under the same #[cfg(nix_at_least =
"2.31")] gate, and assert the returned BTreeMap contains expected keys and
values (verify the C callback wiring by checking that the inserted map entries
match the supplied StorePath strings and expected result strings). Ensure the
test uses the same setup/teardown helpers in this module (context/Store
construction) and exercises the path pointer conversion (StorePath::as_ptr())
path so the unsafe callback path and check_call!(raw::store_build_paths(...))
invocation are covered.
Summary by CodeRabbit