Gate custom Detnix functionality behind feature flag#23
Open
Naxdy wants to merge 2 commits into
Open
Conversation
…r DetNix' thread-safety" This reverts commit 15b159b.
📝 WalkthroughWalkthroughThis PR introduces a ChangesDetNix feature and context persistence
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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
🧹 Nitpick comments (1)
nix-bindings-store/src/store.rs (1)
255-276: 💤 Low valueConsider a helper macro to reduce boilerplate for conditional context selection.
The same 8-line pattern for selecting between a fresh context (detnix) and the reusable context (non-detnix) is repeated across 14 methods. A declarative macro could consolidate this:
macro_rules! get_context { ($self:expr) => {{ #[cfg(feature = "detnix")] let mut context = Context::new(); #[cfg(feature = "detnix")] let ctx = &mut context; #[cfg(not(feature = "detnix"))] let ctx = &mut $self.context; ctx }}; }Then each method would simply use
let ctx = get_context!(self);. This reduces duplication and ensures consistency if the pattern needs to change.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nix-bindings-store/src/store.rs` around lines 255 - 276, The repetitive conditional context-selection block used in methods like get_uri (creating a local Context when feature "detnix" is enabled and otherwise using &mut self.context) should be replaced by a small declarative macro (e.g., get_context!) defined at module scope; implement macro_rules! get_context { ($self:expr) => {{ /* same cfg blocks returning ctx */ }} } that preserves the #[cfg(feature = "detnix")] / #[cfg(not(feature = "detnix"))] branches and mutability, then in get_uri and the other ~13 methods simply call let ctx = get_context!(self); to obtain the &mut Context and remove the duplicated 8-line pattern. Ensure the macro parameter is $self:expr so references to self.context compile and that borrows remain mutable where required.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@nix-bindings-expr/src/value.rs`:
- Around line 90-91: The unsafe impl Sync for Value is incorrectly gated by
#[cfg(feature = "detnix")], removing Value: Sync in non-detnix builds; remove
the cfg so write unsafe impl Sync for Value {} unconditionally (leave any unsafe
impl Send for Value still behind the detnix gate if present) and ensure there
are no duplicate Sync impls elsewhere (adjust or delete the existing
#[cfg(feature = "detnix")] unsafe impl Sync for Value {} so Sync is always
implemented).
In `@nix-bindings-store/src/path/mod.rs`:
- Around line 25-26: The Sync impl for StorePath is incorrectly feature-gated;
make StorePath unconditionally Sync by removing the #[cfg(feature = "detnix")]
guard from the unsafe impl Sync for StorePath so that the type retains
thread-sharing bounds outside the detnix feature (verify the unsafe impl Sync
for StorePath is present without cfg and that any Send impls remain as
intended).
---
Nitpick comments:
In `@nix-bindings-store/src/store.rs`:
- Around line 255-276: The repetitive conditional context-selection block used
in methods like get_uri (creating a local Context when feature "detnix" is
enabled and otherwise using &mut self.context) should be replaced by a small
declarative macro (e.g., get_context!) defined at module scope; implement
macro_rules! get_context { ($self:expr) => {{ /* same cfg blocks returning ctx
*/ }} } that preserves the #[cfg(feature = "detnix")] / #[cfg(not(feature =
"detnix"))] branches and mutability, then in get_uri and the other ~13 methods
simply call let ctx = get_context!(self); to obtain the &mut Context and remove
the duplicated 8-line pattern. Ensure the macro parameter is $self:expr so
references to self.context compile and that borrows remain mutable where
required.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6bc0ffcb-16dd-43de-a362-d45957041353
📒 Files selected for processing (14)
nix-bindings-expr/Cargo.tomlnix-bindings-expr/src/eval_state.rsnix-bindings-expr/src/eval_state_detnix.rsnix-bindings-expr/src/lib.rsnix-bindings-expr/src/primop.rsnix-bindings-expr/src/primop_detnix.rsnix-bindings-expr/src/value.rsnix-bindings-flake/Cargo.tomlnix-bindings-flake/src/lib.rsnix-bindings-store/Cargo.tomlnix-bindings-store/src/path/mod.rsnix-bindings-store/src/store.rsnix-bindings-util/Cargo.tomlnix-bindings-util/src/context.rs
Comment on lines
+90
to
91
| #[cfg(feature = "detnix")] | ||
| unsafe impl Sync for Value {} |
There was a problem hiding this comment.
Sync should not be behind the detnix gate here.
This drops Value: Sync in non-detnix builds and conflicts with the layer contract (gate Send, keep Sync available). Keep unsafe impl Sync for Value {} unconditional.
#!/bin/bash
# Read-only check: inspect Send/Sync gating across this PR layer's key types.
rg -nP --type=rust -C2 'unsafe impl (Send|Sync) for (Value|StorePath|Context|FlakeSettings|LockedFlake)'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@nix-bindings-expr/src/value.rs` around lines 90 - 91, The unsafe impl Sync
for Value is incorrectly gated by #[cfg(feature = "detnix")], removing Value:
Sync in non-detnix builds; remove the cfg so write unsafe impl Sync for Value {}
unconditionally (leave any unsafe impl Send for Value still behind the detnix
gate if present) and ensure there are no duplicate Sync impls elsewhere (adjust
or delete the existing #[cfg(feature = "detnix")] unsafe impl Sync for Value {}
so Sync is always implemented).
Comment on lines
+25
to
26
| #[cfg(feature = "detnix")] | ||
| unsafe impl Sync for StorePath {} |
There was a problem hiding this comment.
StorePath also loses Sync outside detnix.
Sync is currently feature-gated along with Send; this should stay unconditional to avoid regressing non-detnix thread-sharing bounds.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@nix-bindings-store/src/path/mod.rs` around lines 25 - 26, The Sync impl for
StorePath is incorrectly feature-gated; make StorePath unconditionally Sync by
removing the #[cfg(feature = "detnix")] guard from the unsafe impl Sync for
StorePath so that the type retains thread-sharing bounds outside the detnix
feature (verify the unsafe impl Sync for StorePath is present without cfg and
that any Send impls remain as intended).
Member
Author
|
coderabbit doesn't properly understand the PR, because it compares it against the target branch, when it really should be compared against the previous commit (the revert) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This gates all detnix parallel eval related changes behind a feature flag. For small changes this is done inline, for bigger changes like
EvalState, different modules are used to select between variants, to keep the diff between upstream's original files and ours minimal.Summary by CodeRabbit
Release Notes
New Features
detnixfeature flag enabling dynamic Nix primitive operations with custom callbacks.Chores