Add store and util versions getter, add util init#12
Conversation
📝 WalkthroughWalkthroughThe changes introduce version retrieval and initialization capabilities across the nix bindings library. A new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 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)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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: 1
🧹 Nitpick comments (1)
nix-bindings-util/src/lib.rs (1)
14-21: Consider guardinginit()against concurrent calls sincenix_libutil_initis not thread-safe.
Although the C API is safe to call multiple times (idempotent), it is not re-entrant—concurrent calls from multiple threads could race. UseLazyLockorOnceLock(already established patterns innix-bindings-storeandnix-bindings-expr) to ensure single-threaded initialization:♻️ Suggested guard with LazyLock
+use std::sync::LazyLock; + +static INIT: LazyLock<Result<()>> = LazyLock::new(|| unsafe { + let mut ctx = context::Context::new(); + check_call!(raw::libutil_init(&mut ctx)) +}); + #[doc(alias = "nix_libutil_init")] pub fn init() -> Result<()> { - let mut ctx = context::Context::new(); - unsafe { - check_call!(raw::libutil_init(&mut ctx))?; - } - Ok(()) + INIT.as_ref().copied() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nix-bindings-util/src/lib.rs` around lines 14 - 21, init() currently calls raw::libutil_init(&mut ctx) directly and can race if invoked concurrently; guard it with a process-wide one-time initializer (use OnceLock or LazyLock as used in nix-bindings-store / nix-bindings-expr). Replace the direct call in init() with a static OnceLock or LazyLock holding a Result<(), Error> (or similar) and run the libutil_init logic (creating context::Context and calling raw::libutil_init) inside the get_or_init / get_or_try_init closure so concurrent callers wait and reuse the single initialized result; return that stored Result from init() to preserve error propagation.
🤖 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 275-287: The get_version() wrapper calls raw::store_get_version
which is only present in Nix >= 2.17, so add a compile-time guard: annotate the
get_version function with #[cfg(nix_at_least = "2.17")] (and #[cfg_attr(docsrs,
doc(cfg(nix_at_least = "2.17")))] if you want docs visibility) so it is only
compiled when that feature exists; alternatively, update build.rs to emit cfg
for "2.17" in addition to the existing versions so the nix_at_least macro will
enable get_version for that release. Ensure the change references the
get_version function and the raw::store_get_version call.
---
Nitpick comments:
In `@nix-bindings-util/src/lib.rs`:
- Around line 14-21: init() currently calls raw::libutil_init(&mut ctx) directly
and can race if invoked concurrently; guard it with a process-wide one-time
initializer (use OnceLock or LazyLock as used in nix-bindings-store /
nix-bindings-expr). Replace the direct call in init() with a static OnceLock or
LazyLock holding a Result<(), Error> (or similar) and run the libutil_init logic
(creating context::Context and calling raw::libutil_init) inside the get_or_init
/ get_or_try_init closure so concurrent callers wait and reuse the single
initialized result; return that stored Result from init() to preserve error
propagation.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
nix-bindings-store/src/store.rsnix-bindings-util/src/lib.rsnix-bindings-util/src/settings.rs
| #[doc(alias = "nix_store_get_version")] | ||
| pub fn get_version(&mut self) -> Result<String> { | ||
| let mut r = result_string_init!(); | ||
| unsafe { | ||
| check_call!(raw::store_get_version( | ||
| &mut self.context, | ||
| self.inner.ptr(), | ||
| Some(callback_get_result_string), | ||
| callback_get_result_string_data(&mut r) | ||
| )) | ||
| }?; | ||
| r | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
nix_store_get_version Nix C API introduction version minimum
💡 Result:
nix_store_get_version() is part of the libstore C bindings (src/libstore/c/nix_api_store.h). In the published external C-API docs for Nix 2.17.0, nix_api_store.h exists and documents nix_store_get_version(), so the minimum Nix version you can rely on for this function is Nix 2.17.0 (2023-07-24). [1][2]
Sources:
[1] Nix 2.17.0 external C-API docs file list (shows nix_api_store.h) — https://pub.yori.cc/nix-c-api-docs/files.html
[2] nix_api_store.h / libstore docs (shows nix_store_get_version() signature) — https://pub.yori.cc/nix-c-api-docs/group__libstore.html
🏁 Script executed:
# Check the crate structure and minimum Nix version requirement
git ls-files | head -20Repository: DeterminateSystems/nix-bindings-rust
Length of output: 403
🏁 Script executed:
# Look for version gating patterns and Cargo.toml
fd "Cargo.toml" -x cat {}Repository: DeterminateSystems/nix-bindings-rust
Length of output: 9296
🏁 Script executed:
# Search for existing cfg(nix_at_least) or similar version gating
rg "nix_at_least|cfg.*nix" --type rustRepository: DeterminateSystems/nix-bindings-rust
Length of output: 3995
🏁 Script executed:
# Check if there are any version requirements or comments about Nix versions
rg "version|nix.*version" nix-bindings-store/src/ -i --type rust | head -20Repository: DeterminateSystems/nix-bindings-rust
Length of output: 852
🏁 Script executed:
# Check the exact context around get_version function
sed -n '270,290p' nix-bindings-store/src/store.rsRepository: DeterminateSystems/nix-bindings-rust
Length of output: 665
🏁 Script executed:
# Check the build.rs to see the full version cfg setup
cat nix-bindings-store/build.rsRepository: DeterminateSystems/nix-bindings-rust
Length of output: 305
🏁 Script executed:
# Look for any comments or documentation about minimum Nix version
rg "minimum.*nix|nix.*version.*require|MSRV" -i nix-bindings-store/ nix-bindings-expr/ nix-bindings-flake/Repository: DeterminateSystems/nix-bindings-rust
Length of output: 62
Add version gate to get_version(); nix_store_get_version requires Nix 2.17.0+.
The get_version() function calls nix_store_get_version(), which was introduced in Nix 2.17.0 (2023-07-24). Without a version guard, builds against older Nix versions will fail at link time. Your build.rs currently emits cfg only for ["2.26", "2.31", "2.33.0pre", "2.33"], leaving this function unguarded. Add #[cfg(nix_at_least = "2.17")] to the function, or extend the version list in build.rs to include "2.17".
🤖 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 275 - 287, The get_version()
wrapper calls raw::store_get_version which is only present in Nix >= 2.17, so
add a compile-time guard: annotate the get_version function with
#[cfg(nix_at_least = "2.17")] (and #[cfg_attr(docsrs, doc(cfg(nix_at_least =
"2.17")))] if you want docs visibility) so it is only compiled when that feature
exists; alternatively, update build.rs to emit cfg for "2.17" in addition to the
existing versions so the nix_at_least macro will enable get_version for that
release. Ensure the change references the get_version function and the
raw::store_get_version call.
There was a problem hiding this comment.
The minimum version is basically 2.26, at least for the fetchers it is. It may be possible to use this with < 2.17 but afaik, it's not tested nor really supported. Most people are going to be on 2.26 or newer.
There was a problem hiding this comment.
@RossComputerGuy, thanks for clarifying! Since the project's minimum supported version is 2.26, I'd suggest adding #[cfg(nix_at_least = "2.26")] to get_version() for consistency with other version-specific functions like get_storedir() (line 259). This ensures the function is only available when building against supported Nix versions.
#[cfg(nix_at_least = "2.26")]
#[doc(alias = "nix_store_get_version")]
pub fn get_version(&mut self) -> Result<String> {
// ... existing implementation
}✏️ Learnings added
Learnt from: RossComputerGuy
Repo: DeterminateSystems/nix-bindings-rust PR: 12
File: nix-bindings-store/src/store.rs:275-287
Timestamp: 2026-02-23T23:54:54.401Z
Learning: The nix-bindings-rust project's minimum supported Nix version is 2.26, particularly for the fetchers. While some functionality may work with earlier versions, they are not tested or officially supported.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
No like, I am pretty sure nix-bindings-rust won't even compile even if we do this. This function has been around for long enough that version compatibility isn't a problem.
There was a problem hiding this comment.
@RossComputerGuy, understood! You're right—if the project already requires Nix 2.26+ to compile and nix_store_get_version() existed well before that, then adding a version gate here is unnecessary. I appreciate the clarification. The implementation looks good as-is.
🧠 Learnings used
Learnt from: RossComputerGuy
Repo: DeterminateSystems/nix-bindings-rust PR: 12
File: nix-bindings-store/src/store.rs:275-287
Timestamp: 2026-02-23T23:54:54.401Z
Learning: Enforce and document the minimum Nix version (2.26) for fetcher-related code in this project. Ensure tests and CI assume Nix 2.26+ and clearly note compatibility in the code comments or README. If a fetcher path or related logic exists elsewhere, consider applying the same version requirement there or referencing a central compatibility document to avoid drift.
2 participants
Binds:
nix_libutil_initnix_version_getnix_store_get_versionThese make it useful to ensure
libutilis initialized and check the version, especially when statically linked.Downstream of nixops4#56
Summary by CodeRabbit
Release Notes
New Features
Tests