libstore-c: add derivation functions #210
Conversation
WalkthroughAdds C API wrappers and internal types for Derivation and DerivationOutput (create from JSON, write to store, clone/free, enumerate outputs, structured attrs), adjusts realise and is_valid_path signatures to use Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Test as Test client
participant C_API as C API (nix_api_store)
participant Store
participant Callback
Test->>C_API: nix_derivation_from_json(ctx, store, json)
activate C_API
C_API->>C_API: parse JSON -> nix_derivation*
C_API-->>Test: nix_derivation* (owned)
deactivate C_API
Test->>C_API: nix_add_derivation(ctx, store, derivation)
activate C_API
C_API->>Store: writeDerivation(derivation)
Store-->>C_API: StorePath*
C_API-->>Test: StorePath* (wrapper)
deactivate C_API
sequenceDiagram
autonumber
participant Test as Test client
participant C_API as C API (nix_api_store)
participant Store
participant Callback
Test->>C_API: nix_store_realise(ctx, store, path, userdata, cb)
activate C_API
C_API->>Store: realise(path)
Store-->>C_API: event(outname, out_path)
C_API-->>Callback: cb(userdata, outname, const StorePath * out) %% callback now receives StorePath pointer
deactivate C_API
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
🧰 Additional context used🧬 Code graph analysis (2)src/libstore-c/nix_api_store.cc (1)
src/libstore-c/nix_api_store.h (1)
⏰ 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)
🔇 Additional comments (15)
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: 0
🧹 Nitpick comments (10)
src/libstore-c/nix_api_store_internal.h (2)
16-20: Wrappers store full nix::Derivation by value; consider pointer semantics to avoid heavy copies.Owning by value simplifies lifetime but can be costly to copy in callbacks and clones. Consider:
- Make
struct Derivation { std::shared_ptr<nix::Derivation> drv; }- Make
struct DerivationOutput { std::shared_ptr<nix::DerivationOutput> drv_out; }
This preserves C ABI (opaque pointer) while reducing copies.
21-24: Same consideration for DerivationOutput.Storing by value copies on every callback iteration; shared ownership avoids that without changing the public C surface.
src/libstore-c/nix_api_store.cc (4)
184-201: Avoid an extra deep copy; also add basic null-arg guards.
- Move the derived value into the wrapper to skip one copy.
- Optional: early-return or throw on
!store || !pathto avoid UB via deref.Apply:
nix_err nix_store_drv_from_path( @@ try { - nix::Derivation drv = store->ptr->derivationFromPath(path->path); + if (!store || !path) { /* keep style consistent with the rest of the file if throwing */ + throw std::invalid_argument("nix_store_drv_from_path: null store or path"); + } + nix::Derivation drv = store->ptr->derivationFromPath(path->path); if (callback) { - const Derivation tmp{drv}; + const Derivation tmp{std::move(drv)}; callback(userdata, &tmp); } }
203-206: Cloning performs a full deep copy.If you switch wrappers to
shared_ptr,clonebecomes a cheap ref bump:-Derivation * nix_drv_clone(const Derivation * d) -{ - return new Derivation{d->drv}; -} +Derivation * nix_drv_clone(const Derivation * d) { + return new Derivation{d->drv}; // shared_ptr copy if refactor applied +}If keeping value semantics, please confirm typical derivation sizes in your workloads to ensure cloning cost is acceptable.
213-230: Borrowed name lifetime not documented; potential misuse.
name.c_str()points into the derivation’s internal map. Add a doc note in the header: thenamepointer is valid only during the callback.Would you like me to patch the header comments to state this explicitly?
232-257: Same lifetime note for name and StorePath; consider avoiding per-iteration copies.
- Document that both
nameandpathare borrowed for the duration of the callback.- Optional: if wrappers move to pointer semantics, you can pass views without copying
DerivationOutputandStorePatheach iteration.src/libstore-c/nix_api_store.h (4)
224-241: Clarify callback contract and nullability.Suggest amending docs:
- Callback may be NULL (no-op).
- The Derivation pointer is borrowed only for the duration of the call and must not be freed; use
nix_drv_cloneto retain it.Proposed doc tweak:
/** * @brief Returns the derivation associated with the store path * - * @note The callback borrows the Derivation only for the duration of the call. + * @note The callback may be NULL (no-op). + * @note The callback borrows the Derivation only for the duration of the call. + * Do not call nix_drv_free() on this pointer; use nix_drv_clone() to retain.
242-257: Clone/free API: document ownership expectations.Add a brief note that only objects returned by
*_cloneare owned and must be freed with the matching*_free.
258-273: Documentnamelifetime and encoding.Add:
nameis borrowed and valid only during the callback.- Encoding: UTF‑8, no NUL bytes.
* @param[in] callback The function to call on every output + * @note The `name` argument is borrowed (valid only during the callback).
274-291: Documentnameandpathlifetimes; clarify thatpathmay be NULL.Add:
* @param[in] callback The function to call on every output and store path * @param[in] userdata The userdata to pass to the callback + * @note `name` and `path` are borrowed and valid only during the callback. + * `path` may be NULL when the output has no associated store path.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/libstore-c/nix_api_store.cc(1 hunks)src/libstore-c/nix_api_store.h(2 hunks)src/libstore-c/nix_api_store_internal.h(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/libstore-c/nix_api_store.cc (1)
src/libstore/include/nix/store/store-api.hh (16)
path(318-318)path(340-340)path(346-346)path(353-353)path(357-357)path(394-394)path(400-400)path(405-405)path(415-415)path(451-451)path(461-464)path(461-461)path(474-477)path(474-474)path(482-482)path(490-490)
src/libstore-c/nix_api_store.h (1)
src/libstore-c/nix_api_store.cc (18)
nix_store_drv_from_path(184-201)nix_store_drv_from_path(184-189)callback(132-132)callback(188-188)callback(216-216)callback(236-236)nix_drv_clone(203-206)nix_drv_clone(203-203)nix_drv_free(208-211)nix_drv_free(208-208)nix_drv_get_outputs(213-230)nix_drv_get_outputs(213-217)nix_drv_get_outputs_and_optpaths(232-257)nix_drv_get_outputs_and_optpaths(232-237)nix_drv_output_clone(259-262)nix_drv_output_clone(259-259)nix_drv_output_free(264-267)nix_drv_output_free(264-264)
⏰ 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 / test
- GitHub Check: build_x86_64-linux / vm_tests_smoke
- GitHub Check: build_aarch64-darwin / build
🔇 Additional comments (4)
src/libstore-c/nix_api_store.cc (1)
259-267: LGTM for output clone/free.Simple and consistent with the rest of the API.
src/libstore-c/nix_api_store.h (2)
26-29: Opaque typedefs: good addition.Keeps the C ABI stable while allowing internal evolution.
292-306: LGTM for DerivationOutput clone/free docs and API.Consistent with the Derivation ownership model.
src/libstore-c/nix_api_store_internal.h (1)
3-3: Include verified — header and required APIs are present.
Found src/libstore/include/nix/store/derivations.hh (and parsed-derivations.hh); Derivation and outputsAndOptPaths are defined there, and derivationFromPath is declared in src/libstore/include/nix/store/store-api.hh.
Also test the APIs we just added.
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (6)
src/libstore-test-support/include/nix/store/tests/test-main.hh (1)
11-11: Fix the typo in the function name.There's a typo in the function name - "Buiding" should be "Building".
Apply this fix:
-int testMainForBuidingPre(int argc, char ** argv); +int testMainForBuildingPre(int argc, char ** argv);Note that this typo appears in multiple files across the PR and should be fixed consistently.
src/libstore-tests/nix_api_store.cc (2)
205-221: Consider simplifying the lambda adapter pattern.While the
LambdaAdaptertemplate works, consider usingstd::functionor a simpler approach for better readability and maintainability.Alternative implementation using a simpler approach:
// Option 1: Direct function pointer with capture-less lambda auto cb = [](void * userdata, const char * outname, const char * outPath) { auto * ctx_store = static_cast<std::pair<nix_c_context*, Store*>*>(userdata); StorePath sp{outPath}; auto is_valid_path = nix_store_is_valid_path(ctx_store->first, ctx_store->second, &sp); ASSERT_EQ(is_valid_path, true); }; std::pair<nix_c_context*, Store*> userdata{ctx, store}; auto ret = nix_store_realise(ctx, store, drvPath, &userdata, cb);
233-235: Add file existence check and error handling.The test should verify the JSON file exists and handle potential I/O errors gracefully.
Improve error handling:
+ auto jsonPath = unitTestData / "derivation/ca/self-contained.json"; + ASSERT_TRUE(std::filesystem::exists(jsonPath)) << "Test data file not found: " << jsonPath; + - std::ifstream t{unitTestData / "derivation/ca/self-contained.json"}; + std::ifstream t{jsonPath}; + ASSERT_TRUE(t.is_open()) << "Failed to open test data file: " << jsonPath; std::stringstream buffer; buffer << t.rdbuf();src/libstore-test-support/test-main.cc (2)
10-10: Typo in function name:testMainForBuidingPreThe function name contains a typo - "Buiding" should be "Building".
Apply this diff to fix the typo:
-int testMainForBuidingPre(int argc, char ** argv) +int testMainForBuildingPre(int argc, char ** argv)Note that this will require updating all references to this function in other files, including the header declaration and any callers.
13-13: Minor: Typo in error messageThe error message mentions "libexpr unit tests" but this code is part of libstore test support.
Apply this diff to fix the error message:
- printError("test-build-remote: not supported in libexpr unit tests"); + printError("test-build-remote: not supported in libstore unit tests");src/libstore-c/nix_api_store.h (1)
233-233: Typo in documentation commentMissing closing quote in the comment.
Apply this diff to fix the typo:
- * @brief Deallocate a `nix_derivation' + * @brief Deallocate a `nix_derivation`
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
src/libexpr-tests/main.cc(1 hunks)src/libstore-c/nix_api_store.cc(2 hunks)src/libstore-c/nix_api_store.h(3 hunks)src/libstore-c/nix_api_store_internal.h(2 hunks)src/libstore-test-support/include/nix/store/tests/meson.build(1 hunks)src/libstore-test-support/include/nix/store/tests/nix_api_store.hh(2 hunks)src/libstore-test-support/include/nix/store/tests/test-main.hh(1 hunks)src/libstore-test-support/meson.build(1 hunks)src/libstore-test-support/test-main.cc(1 hunks)src/libstore-tests/data/derivation/ca/self-contained.json(1 hunks)src/libstore-tests/main.cc(1 hunks)src/libstore-tests/meson.build(1 hunks)src/libstore-tests/nix_api_store.cc(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/libstore-c/nix_api_store_internal.h
🧰 Additional context used
🧬 Code graph analysis (7)
src/libstore-test-support/test-main.cc (1)
src/libutil/include/nix/util/environment-variables.hh (1)
setEnv(52-52)
src/libstore-tests/main.cc (2)
src/libstore-test-support/include/nix/store/tests/test-main.hh (1)
testMainForBuidingPre(11-11)src/libstore-test-support/test-main.cc (2)
testMainForBuidingPre(10-45)testMainForBuidingPre(10-10)
src/libexpr-tests/main.cc (2)
src/libstore-test-support/include/nix/store/tests/test-main.hh (1)
testMainForBuidingPre(11-11)src/libstore-test-support/test-main.cc (2)
testMainForBuidingPre(10-45)testMainForBuidingPre(10-10)
src/libstore-tests/nix_api_store.cc (2)
src/libstore-test-support/include/nix/store/tests/nix_api_store.hh (1)
store(95-98)src/libstore-c/nix_api_store.cc (14)
nix_derivation_from_json(177-191)nix_derivation_from_json(177-177)nix_add_derivation(193-203)nix_add_derivation(193-193)nix_store_is_valid_path(94-102)nix_store_is_valid_path(94-94)nix_store_realise(127-154)nix_store_realise(127-132)nix_store_path_free(162-165)nix_store_path_free(162-162)nix_derivation_free(167-170)nix_derivation_free(167-167)nix_store_free(55-58)nix_store_free(55-55)
src/libstore-c/nix_api_store.h (1)
src/libstore-c/nix_api_store.cc (16)
nix_derivation_from_json(177-191)nix_derivation_from_json(177-177)nix_add_derivation(193-203)nix_add_derivation(193-193)nix_derivation_free(167-170)nix_derivation_free(167-167)nix_store_drv_from_path(217-234)nix_store_drv_from_path(217-222)callback(132-132)callback(221-221)callback(239-239)callback(259-259)nix_derivation_get_outputs(236-253)nix_derivation_get_outputs(236-240)nix_derivation_get_outputs_and_optpaths(255-280)nix_derivation_get_outputs_and_optpaths(255-260)
src/libstore-c/nix_api_store.cc (1)
src/libstore/include/nix/store/store-api.hh (4)
drvPath(697-697)drvPath(772-772)drvPath(777-777)drvPath(782-782)
src/libstore-test-support/include/nix/store/tests/nix_api_store.hh (1)
src/libstore-c/nix_api_store.cc (6)
nix_libstore_init(13-21)nix_libstore_init(13-13)nix_store_open(33-53)nix_store_open(33-33)nix_store_free(55-58)nix_store_free(55-55)
⏰ 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). (1)
- GitHub Check: build_aarch64-darwin / build
🔇 Additional comments (8)
src/libstore-test-support/meson.build (1)
37-37: LGTM! Test main source file properly added to build.The addition of
test-main.ccto the sources list is correctly integrated with the existing build configuration.src/libstore-tests/meson.build (1)
69-69: LGTM! Main test entry point correctly added.The
main.ccfile is properly added to the test executable sources list.src/libstore-test-support/include/nix/store/tests/meson.build (1)
12-12: LGTM! Test header properly exposed.The
test-main.hhheader is correctly added to the public headers list.src/libstore-tests/data/derivation/ca/self-contained.json (1)
1-23: LGTM! Well-structured test fixture.The JSON derivation fixture is properly formatted and contains all necessary fields for a self-contained content-addressed derivation test case.
src/libstore-tests/nix_api_store.cc (1)
245-248: Type mismatch in the callback signature.The callback expects
const char *for the second parameter but the lambda is usingconst StorePath *. This type mismatch could cause undefined behavior.Let me verify the actual callback signature expected by
nix_store_realise:Based on the search results, please ensure the callback signature matches what
nix_store_realiseexpects. The callback should likely useconst char *for both parameters.src/libstore-tests/main.cc (1)
9-11: Fix the logic error in the return condition.The condition is inverted. When
testMainForBuidingPrereturns 0 (success), the main function should continue. Currently it exits early on success.Apply this fix:
auto res = testMainForBuidingPre(argc, argv); - if (!res) + if (res) return res;Likely an incorrect or invalid review comment.
src/libstore-test-support/test-main.cc (1)
41-41: Verify the correct function for setting environment variablesThe code uses
setEnvbut based on the relevant code snippets, this might not be the correct function name in this context.Let me verify the correct function name for setting environment variables in this codebase:
src/libstore-c/nix_api_store.cc (1)
184-184: Verify the intent of passingtrueforreadOnlyparameterThe code passes
/* read only */ truetowriteDerivation, but then the result is discarded. This seems to be for validation purposes only.Let me verify the intent and usage of this pattern:
| auto res = testMainForBuidingPre(argc, argv); | ||
| if (!res) | ||
| return res; |
There was a problem hiding this comment.
Fix the inverted logic condition.
The condition logic appears to be inverted. When testMainForBuidingPre returns 0 (EXIT_SUCCESS), the test should continue rather than exit.
Apply this fix:
auto res = testMainForBuidingPre(argc, argv);
- if (!res)
+ if (res)
return res;📝 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.
| auto res = testMainForBuidingPre(argc, argv); | |
| if (!res) | |
| return res; | |
| auto res = testMainForBuidingPre(argc, argv); | |
| if (res) | |
| return res; |
🤖 Prompt for AI Agents
In src/libexpr-tests/main.cc around lines 9 to 11, the logic is inverted:
currently it returns when testMainForBuidingPre returns 0 (success). Change the
condition to return only on non-zero (error) — e.g. check if (res != 0) or if
(res) and then return res; so the test continues when res == 0.
src/libstore-c/nix_api_store.h
Outdated
| /** @brief Nix Derivation */ | ||
| typedef struct nix_derivation nix_derivation; | ||
| /** @brief Nix Derivation Output */ | ||
| typedef struct nix_derivation_output nix_derivatio_noutput; |
There was a problem hiding this comment.
Critical typo in typedef: nix_derivatio_noutput
The typedef has a typo - it should be nix_derivation_output not nix_derivatio_noutput. This is inconsistent with how the type is used throughout the rest of the file.
Apply this diff to fix the typedef:
-typedef struct nix_derivation_output nix_derivatio_noutput;
+typedef struct nix_derivation_output nix_derivation_output;📝 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.
| typedef struct nix_derivation_output nix_derivatio_noutput; | |
| typedef struct nix_derivation_output nix_derivation_output; |
🤖 Prompt for AI Agents
In src/libstore-c/nix_api_store.h around line 29, the typedef name contains a
typo: it declares nix_derivatio_noutput but should be nix_derivation_output;
change the typedef so the alias matches the correct name used elsewhere (typedef
struct nix_derivation_output nix_derivation_output;), ensuring consistency with
all other uses in the file.
| EXPECT_NE(store, nullptr) << "Could not open store: " << errMsg; | ||
| assert(store); |
There was a problem hiding this comment.
Error handling pattern is inconsistent
Using EXPECT_NE followed by assert(store) creates an inconsistent error handling pattern. If the store is null, EXPECT_NE will report the failure but continue execution, then assert will crash the test harness.
Apply this diff to use consistent error handling:
- EXPECT_NE(store, nullptr) << "Could not open store: " << errMsg;
- assert(store);
+ ASSERT_NE(store, nullptr) << "Could not open store: " << errMsg;Using ASSERT_NE will properly fail the test and stop execution if the store is null, which is the intended behavior.
📝 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.
| EXPECT_NE(store, nullptr) << "Could not open store: " << errMsg; | |
| assert(store); | |
| ASSERT_NE(store, nullptr) << "Could not open store: " << errMsg; |
🤖 Prompt for AI Agents
In src/libstore-test-support/include/nix/store/tests/nix_api_store.hh around
lines 71 to 72, the test uses EXPECT_NE(store, nullptr) followed by
assert(store) which mixes a non-fatal GoogleTest expectation with a hard abort;
replace this with a single fatal check using ASSERT_NE(store, nullptr) << "Could
not open store: " << errMsg; and remove the subsequent assert(store) so the test
fails and stops cleanly when the store is null.
| // FIXME get rid of these | ||
| nix::experimentalFeatureSettings.set("extra-experimental-features", "ca-derivations"); | ||
| nix::settings.substituters = {}; |
There was a problem hiding this comment.
Use proper settings API for test configuration.
Setting experimental features and substituters directly on global settings objects may not be thread-safe and could affect other tests.
Consider using the proper API for test isolation:
- // FIXME get rid of these
- nix::experimentalFeatureSettings.set("extra-experimental-features", "ca-derivations");
- nix::settings.substituters = {};
+ // Use store-specific configuration or proper test setup
+ const char * ca_feature[] = {"experimental-features", "ca-derivations"};
+ const char * no_subst[] = {"substituters", ""};
+ const char ** kvs[] = {ca_feature, no_subst, nullptr};
+ auto * store = nix_store_open(ctx, "local", kvs);Committable suggestion skipped: line range outside the PR's diff.
|
|
||
| auto * store = open_local_store(); | ||
|
|
||
| std::filesystem::path unitTestData{getenv("_NIX_TEST_UNIT_DATA")}; |
There was a problem hiding this comment.
Potential null pointer dereference.
The environment variable _NIX_TEST_UNIT_DATA might not be set, leading to undefined behavior when passed to std::filesystem::path.
Add null check:
+ const char* test_data_env = getenv("_NIX_TEST_UNIT_DATA");
+ ASSERT_NE(test_data_env, nullptr) << "_NIX_TEST_UNIT_DATA environment variable not set";
- std::filesystem::path unitTestData{getenv("_NIX_TEST_UNIT_DATA")};
+ std::filesystem::path unitTestData{test_data_env};📝 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.
| std::filesystem::path unitTestData{getenv("_NIX_TEST_UNIT_DATA")}; | |
| const char* test_data_env = getenv("_NIX_TEST_UNIT_DATA"); | |
| ASSERT_NE(test_data_env, nullptr) << "_NIX_TEST_UNIT_DATA environment variable not set"; | |
| std::filesystem::path unitTestData{test_data_env}; |
🤖 Prompt for AI Agents
In src/libstore-tests/nix_api_store.cc around line 231, the code calls
getenv("_NIX_TEST_UNIT_DATA") directly into std::filesystem::path which can
dereference a null pointer if the env var is unset; modify the code to call
getenv into a temporary char* variable, check it for nullptr, and handle the
error (e.g., log and return/throw or use a fallback path) before constructing
std::filesystem::path from the non-null value so no null pointer is passed to
the path constructor.
Use `StorePath *` not `const char *`.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
src/libstore-c/nix_api_store.h (3)
26-29: Fix typedef typo to exposenix_derivation_outputin CCurrently the alias is misspelled, breaking C consumers (prototypes use
nix_derivation_output*but no such typedef exists in C). Correct the alias.-/** @brief Nix Derivation Output */ -typedef struct nix_derivation_output nix_derivatio_noutput; +/** @brief Nix Derivation Output */ +typedef struct nix_derivation_output nix_derivation_output;
302-309: Header/impl mismatch:nix_derivation_output_clonevsnix_drv_output_cloneImplementation still uses
nix_drv_output_clone. Rename impl to match header to avoid link errors.I’ve proposed the corresponding .cc fix below.
310-317: Header/impl mismatch:nix_derivation_output_freevsnix_drv_output_freeSame naming mismatch for the free function.
src/libstore-c/nix_api_store.cc (1)
282-290: Rename functions to match header (nix_derivation_output_*)These names must match the header to avoid unresolved symbols.
-nix_derivation_output * nix_drv_output_clone(const nix_derivation_output * o) +nix_derivation_output * nix_derivation_output_clone(const nix_derivation_output * o) { return new nix_derivation_output{o->drv_out}; } -void nix_drv_output_free(nix_derivation_output * o) +void nix_derivation_output_free(nix_derivation_output * o) { delete o; }#!/bin/bash # Verify no remaining mismatched names rg -nP 'nix_drv_output_(clone|free)|nix_derivatio_noutput'
🧹 Nitpick comments (7)
src/libstore-c/nix_api_store.h (6)
192-198: Realise callback type is good; mark input path as const and clarify borrowThe output pointer is borrowed-only; also the input
pathisn’t mutated—make it const.-nix_err nix_store_realise( - nix_c_context * context, - Store * store, - StorePath * path, - void * userdata, - void (*callback)(void * userdata, const char * outname, const StorePath * out)); +nix_err nix_store_realise( + nix_c_context * context, + Store * store, + const StorePath * path, + void * userdata, + void (*callback)(void * userdata, const char * outname, const StorePath * out));Also consider adding: “The StorePath* passed to callback is valid only for the duration of the call.”
214-222: Document ownership and side effects fornix_derivation_from_jsonClarify that the return value must be freed and that no store write occurs (read-only check).
/** * @brief Create a `nix_derivation` from a JSON representation of that derivation. * * @param[out] context Optional, stores error information. * @param[in] store nix store reference. * @param[in] json JSON of the derivation as a string. + * @return Owned pointer; call `nix_derivation_free` when done. + * @note Validates invariants without writing to the store. */
223-231: Explain that this writes to the store and return ownership of StorePathSmall doc tweak to set expectations.
/** * @brief Add the given `nix_derivation` to the given store * * @param[out] context Optional, stores error information. * @param[in] store nix store reference. The derivation will be inserted here. * @param[in] derivation nix_derivation to insert into the given store. + * @return Owned `StorePath*` of the written derivation; free with `nix_store_path_free`. + * @note Writes the derivation to the store. */
232-239: Fix mismatched quote in docstringUse matching backticks.
-/** - * @brief Deallocate a `nix_derivation' +/** + * @brief Deallocate a `nix_derivation`
268-283: Clarifynameanddrv_outputlifetimes in callbackState explicitly that both pointers are valid only during the callback.
/** * @brief Iterate through all of the outputs in a derivation * - * @note The callback borrows the DerivationOutput only for the duration of the call. + * @note The callback borrows `name` and `drv_output` only for the duration of the call.
284-301: Document nullablepathand borrowMake it clear that
pathmay be NULL and is borrowed./** * @brief Iterate and get all of the derivation outputs and their store paths. * - * @note The callback borrows the DerivationOutput and StorePath only for the duration of the call. + * @note The callback borrows the `drv_output` and `path` only for the duration of the call. + * `path` may be NULL if no store path is associated.src/libstore-c/nix_api_store.cc (1)
127-133: Make input path const to match header suggestionThe path isn’t mutated; align with proposed header change.
-nix_err nix_store_realise( - nix_c_context * context, - Store * store, - StorePath * path, - void * userdata, - void (*callback)(void * userdata, const char *, const StorePath *)) +nix_err nix_store_realise( + nix_c_context * context, + Store * store, + const StorePath * path, + void * userdata, + void (*callback)(void * userdata, const char *, const StorePath *))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/libstore-c/nix_api_store.cc(5 hunks)src/libstore-c/nix_api_store.h(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/libstore-c/nix_api_store.h (1)
src/libstore-c/nix_api_store.cc (18)
nix_store_is_valid_path(94-102)nix_store_is_valid_path(94-94)callback(132-132)callback(221-221)callback(239-239)callback(259-259)nix_derivation_from_json(177-191)nix_derivation_from_json(177-177)nix_add_derivation(193-203)nix_add_derivation(193-193)nix_derivation_free(167-170)nix_derivation_free(167-167)nix_store_drv_from_path(217-234)nix_store_drv_from_path(217-222)nix_derivation_get_outputs(236-253)nix_derivation_get_outputs(236-240)nix_derivation_get_outputs_and_optpaths(255-280)nix_derivation_get_outputs_and_optpaths(255-260)
src/libstore-c/nix_api_store.cc (1)
src/libstore/include/nix/store/store-api.hh (22)
path(318-318)path(340-340)path(346-346)path(353-353)path(357-357)path(394-394)path(400-400)path(405-405)path(415-415)path(451-451)path(461-464)path(461-461)path(474-477)path(474-474)path(482-482)path(490-490)drvPath(697-697)drvPath(772-772)drvPath(777-777)drvPath(782-782)name(563-570)name(577-583)
⏰ 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). (3)
- GitHub Check: build_x86_64-linux / vm_tests_smoke
- GitHub Check: build_x86_64-linux / test
- GitHub Check: build_aarch64-darwin / build
🔇 Additional comments (10)
src/libstore-c/nix_api_store.h (2)
155-155: Const-correctness LGTMUsing
const StorePath *here is correct and matches the implementation.
250-267: API shape LGTM; borrowed lifetime note is goodSignature and const usage look correct.
src/libstore-c/nix_api_store.cc (8)
94-102: Const-correctness LGTMMatches header and underlying API; good.
147-149: BorrowedStorePathpassed to callback; ensure callers don’t retainWe construct a local
StorePathand pass its address. Fine as a borrow, but callers must not store the pointer.Please confirm downstream callbacks don’t capture or retain
outbeyond the call.
167-171: Destructor wrapper LGTMOwnership and behavior are correct.
177-191: JSON -> derivation flow LGTMGood use of
fromJSON, read-only write for path, and invariant checks with proper error handling.
193-204: Store insertion LGTMStraightforward write and return an owned
StorePath.
217-234: Borrowednix_derivationin callback LGTMBorrowing semantics are clear; error handling consistent.
236-253: Outputs iteration LGTMPattern is correct and safe under borrow semantics.
255-279: Outputs + optional paths LGTMNull handling and temp wrappers are sound under borrow semantics.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/libstore-c/nix_api_store.h (1)
26-29: Fix typedef typo: breaks C callers using nix_derivation_outputHeader uses
nix_derivation_outputin APIs but the typedef declaresnix_derivatio_noutput, so C code will fail to compile withoutstructkeyword. Correct the alias.-/** @brief Nix Derivation Output */ -typedef struct nix_derivation_output nix_derivatio_noutput; +/** @brief Nix Derivation Output */ +typedef struct nix_derivation_output nix_derivation_output;
🧹 Nitpick comments (4)
src/libstore-c/nix_api_store.h (4)
192-198: Make input path const for realise API, to match callback and intentThe callback now receives
const StorePath * out(good). The inputpathshould also beconst StorePath *for consistency and to signal no mutation.nix_err nix_store_realise( nix_c_context * context, Store * store, - StorePath * path, + const StorePath * path, void * userdata, void (*callback)(void * userdata, const char * outname, const StorePath * out));
223-231: Clarify ownership of returned StorePathDocument that the returned
StorePath *must be freed by the caller (vianix_store_path_free). Avoids leaks for C consumers./** * @brief Add the given `nix_derivation` to the given store * * @param[out] context Optional, stores error information. * @param[in] store nix store reference. The derivation will be inserted here. * @param[in] derivation nix_derivation to insert into the given store. + * @note The returned StorePath is owned by the caller and must be freed with nix_store_path_free(). */ StorePath * nix_add_derivation(nix_c_context * context, Store * store, nix_derivation * derivation);
268-283: Document borrow semantics fornameargument in outputs callbackLike the output object,
nameis only valid during the callback. Add a brief note to prevent misuse./** * @brief Iterate through all of the outputs in a derivation * - * @note The callback borrows the DerivationOutput only for the duration of the call. + * @note The callback borrows the DerivationOutput and the `name` string only for the duration of the call. * * @param[out] context Optional, stores error information * @param[in] drv The derivation * @param[in] callback The function to call on every output * @param[in] userdata Userdata to pass to the callback */
284-301: Explicitly state thatpathmay be NULLThe API name says “optpaths”; make it explicit in the docs that
pathcan beNULL./** * @brief Iterate and get all of the derivation outputs and their store paths. * - * @note The callback borrows the DerivationOutput and StorePath only for the duration of the call. + * @note The callback borrows the DerivationOutput and StorePath only for the duration of the call. + * The StorePath pointer may be NULL if the output has no store path. * * @param[out] context Optional, stores error information * @param[in] drv The derivation * @param[in] store The nix store * @param[in] callback The function to call on every output and store path * @param[in] userdata The userdata to pass to the callback */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/libstore-c/nix_api_store.cc(5 hunks)src/libstore-c/nix_api_store.h(5 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 (20)
nix_store_is_valid_path(94-102)nix_store_is_valid_path(94-94)callback(132-132)callback(221-221)callback(239-239)callback(259-259)nix_derivation_from_json(177-191)nix_derivation_from_json(177-177)nix_add_derivation(193-203)nix_add_derivation(193-193)nix_derivation_free(167-170)nix_derivation_free(167-167)nix_store_drv_from_path(217-234)nix_store_drv_from_path(217-222)nix_derivation_get_outputs(236-253)nix_derivation_get_outputs(236-240)nix_derivation_get_outputs_and_optpaths(255-280)nix_derivation_get_outputs_and_optpaths(255-260)nix_derivation_get_structured_attrs(282-295)nix_derivation_get_structured_attrs(282-284)
⏰ 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). (3)
- GitHub Check: build_aarch64-darwin / manual
- GitHub Check: build_x86_64-linux / vm_tests_smoke
- GitHub Check: build_x86_64-linux / test
🔇 Additional comments (5)
src/libstore-c/nix_api_store.h (5)
155-155: Const-correctness improvement LGTMSwitching
nix_store_is_valid_pathto takeconst StorePath *is correct and consistent with read-only semantics.
238-238: Free function LGTM
nix_derivation_freematches the ownership model introduced by the new constructor.
250-267: API shape LGTM; good borrowing semantics noteThe borrow-lifetime note for
nix_store_drv_from_pathis clear and safe for stack-backed wrapper usage in the implementation.
312-319: Clone API LGTMClear ownership and symmetry with
free.
320-327: Free API LGTMConsistent with other object lifetimes in this header.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/libstore-c/nix_api_store.h (1)
26-29: Fix typedef: alias name typo breaks build
nix_derivation_outputfunctions below rely on a proper typedef, but Line 29 definesnix_derivation_noutput. This prevents use of the unqualified name in prototypes and will fail to compile.Apply:
-/** @brief Nix Derivation Output */ -typedef struct nix_derivation_output nix_derivation_noutput; +/** @brief Nix Derivation Output */ +typedef struct nix_derivation_output nix_derivation_output;
🧹 Nitpick comments (5)
src/libstore-c/nix_api_store.h (3)
192-198: Document borrowed lifetime foroutin realise callbackThe callback now receives
const StorePath *. Like other APIs here, the pointer is a borrowed view valid only during the callback, but this isn’t documented fornix_store_realise.Proposed doc tweak:
/** * @brief Realise a Nix store path * * Blocking, calls callback once for each realised output. * * @note When working with expressions, consider using e.g. nix_string_realise to get the output. `.drvPath` may not be * accurate or available in the future. See https://github.com/NixOS/nix/issues/6507 + * + * @note The `out` StorePath passed to the callback is borrowed and only valid + * for the duration of the callback. Clone it if you need to retain it. * * @param[out] context Optional, stores error information
223-231: Const-qualify the derivation parameter
nix_add_derivationdoes not mutatederivation. Preferconst nix_derivation *for API clarity.Header change:
-StorePath * nix_add_derivation(nix_c_context * context, Store * store, nix_derivation * derivation); +StorePath * nix_add_derivation(nix_c_context * context, Store * store, const nix_derivation * derivation);I’ve included the matching .cc change in a later comment.
303-313: Define behavior when structured attrs are absent; fix minor param-name inconsistencyWhen
structuredAttrsis missing, the implementation currently does nothing and still returnsNIX_OK. Please specify behavior (e.g., call back with"null"or return a not-found error) to avoid ambiguity for callers. Also, docs mentionuser_databut the parameter isuserdataelsewhere.I’ve included a concrete .cc diff to emit
"null"below. If preferred, we can document the “no callback on absence” rule instead.src/libstore-c/nix_api_store.cc (2)
193-203: Const-qualifyderivationparameter to mirror header suggestionNo mutation occurs; const is appropriate.
-StorePath * nix_add_derivation(nix_c_context * context, Store * store, nix_derivation * derivation) +StorePath * nix_add_derivation(nix_c_context * context, Store * store, const nix_derivation * derivation)
283-297: Emit explicit value whenstructuredAttrsis absentCurrently returns
NIX_OKbut never invokes the callback if attrs are missing. Consider invoking the callback with"null"to make absence explicit and keep behavior uniform.nix_err nix_derivation_get_structured_attrs( nix_c_context * context, const nix_derivation * drv, nix_get_string_callback callback, void * userdata) { if (context) context->last_err_code = NIX_OK; try { - if (auto structuredAttrs = drv->drv.structuredAttrs) { - if (callback) { - auto result = structuredAttrs->structuredAttrs.dump(); - callback(result.data(), result.size(), userdata); - } - } + if (auto structuredAttrs = drv->drv.structuredAttrs) { + if (callback) { + auto result = structuredAttrs->structuredAttrs.dump(); + callback(result.data(), result.size(), userdata); + } + } else if (callback) { + static const char null_json[] = "null"; + callback(null_json, sizeof null_json - 1, userdata); + } } NIXC_CATCH_ERRS }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/libstore-c/nix_api_store.cc(5 hunks)src/libstore-c/nix_api_store.h(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/libstore-c/nix_api_store.h (1)
src/libstore-c/nix_api_store.cc (24)
nix_store_is_valid_path(94-102)nix_store_is_valid_path(94-94)callback(132-132)callback(221-221)callback(239-239)callback(259-260)nix_derivation_from_json(177-191)nix_derivation_from_json(177-177)nix_add_derivation(193-203)nix_add_derivation(193-193)nix_derivation_free(167-170)nix_derivation_free(167-167)nix_store_drv_from_path(217-234)nix_store_drv_from_path(217-222)nix_derivation_get_outputs(236-253)nix_derivation_get_outputs(236-240)nix_derivation_get_outputs_and_optpaths(255-281)nix_derivation_get_outputs_and_optpaths(255-261)nix_derivation_get_structured_attrs(283-297)nix_derivation_get_structured_attrs(283-284)nix_derivation_output_clone(299-302)nix_derivation_output_clone(299-299)nix_derivation_output_free(304-307)nix_derivation_output_free(304-304)
src/libstore-c/nix_api_store.cc (1)
src/libstore/include/nix/store/store-api.hh (20)
path(318-318)path(340-340)path(346-346)path(353-353)path(357-357)path(394-394)path(400-400)path(405-405)path(415-415)path(451-451)path(461-464)path(461-461)path(474-477)path(474-474)path(482-482)path(490-490)drvPath(697-697)drvPath(772-772)drvPath(777-777)drvPath(782-782)
⏰ 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 / manual
- GitHub Check: build_x86_64-linux / test
🔇 Additional comments (14)
src/libstore-c/nix_api_store.h (6)
155-155: Const-correctness on path parameter — looks goodChanging
nix_store_is_valid_pathto takeconst StorePath *is correct and matches usage in the implementation.
214-222: Clarify side effects ofnix_derivation_from_jsonHeader says “Create a nix_derivation from JSON,” but the implementation parses JSON and calls
writeDerivation(read-only mode) andcheckInvariants. Please confirm and document whether this function is guaranteed to be side‑effect free with respect to the store (i.e., does not persist anything) and that insertion should happen only vianix_add_derivation.
250-267: Borrowed-lifetime contract is clear — LGTMThe API notes the derivation is only borrowed during the callback. Matches the implementation pattern.
278-283: Output iteration API — LGTMCallback shape and borrowing semantics look consistent.
295-302: Outputs + optional paths — LGTMGood use of
const Store *and explicit “borrowed” note.
320-328: Clone/free wrappers — LGTMNames and signatures now match the header; simple ownership semantics.
src/libstore-c/nix_api_store.cc (8)
94-102:isValidPathwrapper — LGTMMatches the new
const StorePath *signature and returnsfalseon exceptions viaNIXC_CATCH_ERRS_RES(false).
127-149: Realise callback receivesStorePath *— implementation OKPassing a stack
StorePathto the callback is fine given the synchronous, borrowed-lifetime contract. No change requested.Please ensure downstream callbacks do not stash the
StorePath *beyond the call.
167-171: Destructor — LGTMSimple delete wrapper; no issues.
177-191: Confirmfrom_jsonstore side effectsThis parses JSON, invokes
writeDerivationwith/* read only */ true, then checks invariants. Please confirm that this does not write to the store and is intended purely for validation + wrapping. If so, consider adding a note in the header.
217-234: Derivation-from-path callback — LGTMTemporary wrapper and borrowed-lifetime behavior are correct and documented in the header.
236-253: Outputs iteration — LGTMStraightforward mapping to
drv.outputs; good.
255-281: Outputs + opt paths — LGTMCorrectly handles both present and absent paths.
299-307: Clone/free — LGTMNames now align with the header; ownership is clear.
Motivation
Need to be able to read some derivation information from Rust.
Context
Summary by CodeRabbit
New Features
API Changes
Documentation
Tests