libexpr-c: fix changing eval settings having no effect #310
Conversation
📝 WalkthroughWalkthroughIntroduces a global Changes
Sequence Diagram(s)sequenceDiagram
participant StaticInit as Static Init
participant GlobalConfig as GlobalConfig::Register
participant EvalSettings as nix::evalSettings
rect rgba(100,149,237,0.5)
StaticInit->>GlobalConfig: construct Register(&evalSettings, callback)
GlobalConfig->>EvalSettings: store pointer in registry
GlobalConfig->>EvalSettings: invoke callback()
EvalSettings->>EvalSettings: lookupPathHooks = { ... }
end
sequenceDiagram
participant Caller as C API caller
participant Builder as nix_eval_state_builder
participant GlobalEval as nix::evalSettings / nix::fetchSettings
participant EvalState as EvalState
rect rgba(60,179,113,0.5)
Caller->>Builder: nix_eval_state_builder_new()
Builder->>GlobalEval: settings = &nix::evalSettings\nfetchSettings = &nix::fetchSettings
Caller->>Builder: nix_eval_state_builder_load(builder)
Builder->>GlobalEval: loadConfFile(*builder->settings)\nloadConfFile(*builder->fetchSettings)
Caller->>Builder: nix_eval_state_build(builder)
Builder->>EvalState: construct using *builder->settings, *builder->fetchSettings
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![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
Fix all issues with AI Agents 🤖
In @src/libexpr/eval-settings.cc:
- Line 122: The global evalSettings currently binds to settings.readOnlyMode at
static init time, risking a dangling reference due to undefined init order
between evalSettings and settings; change evalSettings to be lazily initialized
(e.g., replace the global EvalSettings evalSettings with an accessor like
getEvalSettings() that creates a function-local static EvalSettings and reads
settings.readOnlyMode at first use) or store the readOnlyMode value (or a safe
pointer) inside EvalSettings instead of a reference; ensure the new code
references the existing EvalSettings type and the settings.readOnlyMode symbol
so callers use getEvalSettings() (or the new safe construct) everywhere
evalSettings was used.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/libcmd/common-eval-args.cc (1)
22-43: Callback pattern creates static initialization order dependency.The callback mechanism cleanly separates libcmd concerns from libexpr, but the implementation has a critical flaw: the callback executes immediately in the
GlobalConfig::Registerconstructor (not deferred), and it references the globalfetchSettingsvariable (lines 28, 31). SincefetchSettingsis defined in a separate translation unit (src/libfetchers/fetch-settings.cc), C++ provides no guarantee about initialization order—rEvalSettingscould execute beforefetchSettingsis initialized, causing undefined behavior.Either defer the callback execution until after all globals are initialized, or move the flake lookup hook registration to a location that guarantees both
evalSettingsandfetchSettingsare already initialized.
🧹 Nitpick comments (1)
src/libutil/config-global.cc (1)
70-74: Consider callback ordering dependencies.The callback is executed immediately after registration during static initialization. Ensure that callbacks don't depend on other configs being registered first, as static initialization order across translation units is undefined.
If callback ordering becomes an issue, consider deferring callback execution to a later initialization phase (e.g., after all static registrations complete). However, this is likely not necessary for the current use case.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/libcmd/common-eval-args.ccsrc/libexpr-c/nix_api_expr.ccsrc/libexpr-c/nix_api_expr_internal.hsrc/libexpr/eval-settings.ccsrc/libexpr/include/nix/expr/eval-settings.hhsrc/libflake-c/nix_api_flake.ccsrc/libutil/config-global.ccsrc/libutil/include/nix/util/config-global.hh
🧰 Additional context used
🧬 Code graph analysis (2)
src/libexpr/include/nix/expr/eval-settings.hh (1)
src/libexpr/include/nix/expr/parallel-eval.hh (1)
evalSettings(50-50)
src/libutil/config-global.cc (1)
src/libutil/include/nix/util/config-global.hh (2)
Register(28-28)Register(29-29)
🔇 Additional comments (11)
src/libexpr/include/nix/expr/eval-settings.hh (1)
415-418: LGTM! Clean global settings declaration.The extern declaration is well-documented and follows common patterns for global configuration objects. The placement after
evalStackSizeis logical.src/libexpr/eval-settings.cc (1)
124-124: LGTM! Static registration is appropriate.The static registration using
GlobalConfig::Registerensures thatevalSettingsis registered with the global configuration system during static initialization.src/libutil/include/nix/util/config-global.hh (1)
29-29: LGTM! Clean callback overload declaration.The new constructor overload with an rvalue reference callback parameter follows C++ best practices and enables post-registration hooks.
src/libutil/config-global.cc (1)
64-68: Duplicate-registration guard is not thread-safe but acceptable for static initialization.The duplicate check using
std::findprevents the same config from being registered multiple times. While not thread-safe (TOCTOU), this is acceptable since registrations occur during single-threaded static initialization.src/libflake-c/nix_api_flake.cc (1)
35-35: The pointer dereference is safe and correct.builder->settingsis initialized to point to the global static objectnix::evalSettingsand cannot be null. The usage pattern is consistent throughout the codebase.Likely an incorrect or invalid review comment.
src/libexpr-c/nix_api_expr_internal.h (2)
16-17: LGTM! Pointer semantics enable shared global settings.The change from value to pointer types allows the C API to reference the global
evalSettingsandfetchSettingsobjects, ensuring that modifications to these settings take effect. The lifetime dependency on globals is acceptable here.
25-26: LGTM! Consistent pointer semantics.The pointer types here match the
nix_eval_state_builderstructure, maintaining consistency in how settings are passed from builder to the constructedEvalState.src/libexpr-c/nix_api_expr.cc (4)
9-9: LGTM! Necessary include for fetchSettings global.This include is required to access the
nix::fetchSettingsglobal object referenced in the initialization code.
138-145: LGTM! Global settings initialization enables the fix.Initializing
settingsandfetchSettingsto point to the globalnix::evalSettingsandnix::fetchSettingsobjects (lines 141-142) is the key change that allows C API modifications to take effect. This ensures all eval state builders share the same configuration.Note that this introduces shared mutable state: modifications to these settings through one builder will affect all eval states using the global settings.
163-165: LGTM! Correct pointer dereferencing.The pointer dereferences correctly access the global settings objects to:
- Propagate
readOnlyModefrom the global settings- Load configuration into both eval and fetch settings
The code assumes the pointers are non-null, which is valid since they're initialized in
nix_eval_state_builder_newto point to globals.
190-196: LGTM! Proper pointer propagation to EvalState.The construction correctly:
- Passes the settings pointers from the builder (lines 192-193)
- Dereferences them for the
nix::EvalStateconstructor (line 194)This maintains the shared reference to global settings throughout the eval state's lifetime. Since the pointers reference globals with static storage duration, the lifetime dependencies are satisfied.
| return settings.useXDGBaseDirectories ? getStateDir() / "defexpr" : getHome() / ".nix-defexpr"; | ||
| } | ||
|
|
||
| EvalSettings evalSettings{settings.readOnlyMode}; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the definition of the global settings object
# Search for the definition of settings global object
rg -n -C5 '^Settings settings' --type=cpp
# Also search for extern declarations
rg -n 'extern.*Settings.*settings' --type=cpp -g '*.hh' -g '*.h'Repository: DeterminateSystems/nix-src
Length of output: 793
🏁 Script executed:
# Find EvalSettings class definition and constructor
rg -n 'class EvalSettings' --type=cpp -A 10
# Also search for the constructor implementation
rg -n 'EvalSettings::EvalSettings' --type=cpp -A 5Repository: DeterminateSystems/nix-src
Length of output: 536
🏁 Script executed:
# Find EvalSettings class header definition
rg -n 'class EvalSettings' --type=cpp -B 5 -A 20 src/libexpr/
# Also check the header file
fd 'eval-settings.hh' -x cat {}Repository: DeterminateSystems/nix-src
Length of output: 16493
🏁 Script executed:
# Check the Register call in eval-settings.cc around line 124
sed -n '120,130p' src/libexpr/eval-settings.cc
# Check what GlobalConfig::Register does and if it provides ordering guarantees
rg -n 'class.*Register|struct.*Register' src/libutil/ --type=cpp -A 5Repository: DeterminateSystems/nix-src
Length of output: 638
🏁 Script executed:
# Find the Register implementation
rg -n 'GlobalConfig::Register::Register' src/libutil/ --type=cpp -A 10
# Also check if there's any lazy initialization or factory pattern
rg -n 'evalSettings' src/libexpr/eval-settings.cc -B 5 -A 5Repository: DeterminateSystems/nix-src
Length of output: 1490
Address static initialization order dependency between evalSettings and settings.
The global evalSettings stores a reference to settings.readOnlyMode. Since settings is defined in src/libstore/globals.cc and evalSettings in src/libexpr/eval-settings.cc, the C++ standard leaves their initialization order undefined. If evalSettings initializes before settings, it will hold a dangling reference.
Consider using lazy initialization (e.g., a factory function or callback mechanism) to defer creation of evalSettings until settings is guaranteed to be initialized.
🤖 Prompt for AI Agents
In @src/libexpr/eval-settings.cc around line 122, The global evalSettings
currently binds to settings.readOnlyMode at static init time, risking a dangling
reference due to undefined init order between evalSettings and settings; change
evalSettings to be lazily initialized (e.g., replace the global EvalSettings
evalSettings with an accessor like getEvalSettings() that creates a
function-local static EvalSettings and reads settings.readOnlyMode at first use)
or store the readOnlyMode value (or a safe pointer) inside EvalSettings instead
of a reference; ensure the new code references the existing EvalSettings type
and the settings.readOnlyMode symbol so callers use getEvalSettings() (or the
new safe construct) everywhere evalSettings was used.
Using the Rust bindings (which call into the C bindings) to set eval-related settings was not working. Since EvalSettings were defined in libcmd, we need to move them to libexpr if libexpr-c wants to use them. Since libcmd enforces a dependency on libflake, we also need to modify config-global to absorb duplicate registrations and support callbacks for customizing behavior that we only need in libcmd. Note that we add back some of the global setting variables with this, but this does fix the C API.
numinit
force-pushed
the
fix-c-api-eval-settings
branch
from
6142e85 to
409c004
Compare
1 participant
Motivation
Using the Rust bindings (which call into the C bindings) to set eval-related settings was not working.
Since EvalSettings were defined in libcmd, we need to move them to libexpr if libexpr-c wants to use them. Since libcmd enforces a dependency on libflake, we also need to modify config-global to absorb duplicate registrations and support callbacks for customizing behavior that we only need in libcmd.
Note that we add back some of the global setting variables with this, but this does fix the C API.
Context
Implementation strategy, since this was slightly nontrivial:
Upstream PR: NixOS#14917
Note that this was tested using the Rust bindings and setting
eval-cores.Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.