Conversation
📝 WalkthroughWalkthroughAdds centralized, compiler-aware Meson Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant Meson as Meson (configure)
participant Build as Compiler/Linker
participant Wasmtime as Wasmtime lib
participant Runtime as Nix runtime
Dev->>Meson: set option `wasm` / package `enableWasm`
Meson->>Meson: compute `prelink`, `wasmtime_required`, `link_args`, set NIX_USE_WASMTIME
Meson->>Build: emit compile defs and `link_args` (may include -lwasmtime)
Build->>Wasmtime: link if `link_args` contains wasmtime
Runtime->>Runtime: instantiate WasiDerivationBuilder if NIX_USE_WASMTIME
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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 |
529dd02 to
dcb75d7
Compare
7053c04 to
769f599
Compare
878d282 to
37b33a6
Compare
![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 @.github/workflows/build.yml:
- Line 55: The step running "nix build .#packages.${{ inputs.system
}}.nix-cli-static --no-link -L" must be gated to Linux because nix-cli-static
relies on pkgsStatic which only works on Linux; add an if condition to the step
such as if: contains(inputs.system, 'linux') (or if: runner.os == 'Linux') so
the nix-cli-static build is only executed for Linux systems and skipped on macOS
(e.g., aarch64-darwin).
255efce to
1fee9a2
Compare
f8bfb37 to
512bac7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packaging/dependencies.nix`:
- Around line 86-90: The curl override uses pkgs.stdenv to detect static builds
which can differ from the scoped stdenv in this file; change the checks in the
override to use the scoped stdenv available in the current scope (replace
pkgs.stdenv.hostPlatform.isStatic with stdenv.hostPlatform.isStatic) so
pslSupport and idnSupport are computed correctly for static/cross builds (update
references in the curl override block for pslSupport and idnSupport).
678239b to
a75a0c5
Compare
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Fix all issues with AI agents
In `@src/libfetchers-c/nix_api_fetchers.h`:
- Around line 24-35: Fix the doc comment for nix_libfetchers_init: correct the
grammar "it idempotent" to "it is idempotent" and change the `@ingroup` tag from
libfetcher_init to the existing group name libfetchers (or alternatively define
a libfetcher_init subgroup if that was intended) so the group reference matches
the file's `@defgroup`; update only the comment block above the
nix_libfetchers_init declaration.
In `@src/libfetchers/fetchers.cc`:
- Around line 546-557: initFetchers() currently registers every input scheme by
calling registerTarballInputScheme(), registerGitHubInputScheme(),
registerPathInputScheme(), etc., but each of those is also registered via
OnStartup wrappers in their individual modules (e.g., tarball.cc, github.cc,
path.cc), causing duplicate-registration errors from registerInputScheme() when
initFetchers() runs later; remove the OnStartup registration wrappers from the
individual source files (e.g., the OnStartup calls in tarball.cc, github.cc,
path.cc and any other register*InputScheme() OnStartup usages) so that
initFetchers() is the single place that calls all register*InputScheme()
functions and no duplicate registerInputScheme() calls occur.
In `@src/libfetchers/git.cc`:
- Around line 1238-1243: The duplicate registration happens because
rGitInputScheme (the OnStartup lambda) calls registerGitInputScheme() and
initFetchers() also calls the same function, causing registerInputScheme() to
throw on second registration; fix by making the registration idempotent or
removing the automatic startup registration: either delete the rGitInputScheme
OnStartup declaration so registerGitInputScheme() is only invoked from
initFetchers(), or change registerGitInputScheme() to guard against double
registration (for example with a static bool/atomic "registered" flag or by
checking if the scheme is already registered before calling
registerInputScheme()) so repeated calls are safe; update references to
rGitInputScheme, registerGitInputScheme, and registerInputScheme accordingly.
In `@src/libfetchers/github.cc`:
- Around line 662-664: These static variables use OnStartup wrappers which cause
duplicate registration when initFetchers() also registers schemes; remove the
OnStartup(...) wrapper and initialize the variables by directly calling the
registration functions instead (replace the lines for rGitHubInputScheme,
rGitLabInputScheme, and rSourceHutInputScheme so they call
registerGitHubInputScheme(), registerGitLabInputScheme(), and
registerSourceHutInputScheme() directly or remove the variables entirely and
rely on initFetchers() to perform registration).
In `@src/libfetchers/include/nix/fetchers/fetchers.hh`:
- Around line 281-282: The header declares nix::fetchers::initFetchers() but the
only implementation is nix::initFetchers() — causing a linker mismatch; either
remove the nix::fetchers::initFetchers() declaration from the header or add a
delegating implementation (nix::fetchers::initFetchers() { return
::nix::initFetchers(); }) so both symbols exist and link correctly; locate the
declaration in the nix::fetchers namespace and either delete it or implement the
wrapper to call the existing nix::initFetchers().
In `@src/libfetchers/indirect.cc`:
- Around line 142-147: The OnStartup static rIndirectInputScheme currently calls
registerIndirectInputScheme() but initFetchers() also calls
registerIndirectInputScheme(), causing double registration and a thrown error
from registerInputScheme(); fix by making registration idempotent: modify
registerIndirectInputScheme() to check for prior registration (e.g., a static
bool/guard or query existing schemes) and return early if already registered, or
alternatively remove the OnStartup rIndirectInputScheme registration so
initFetchers() is the sole initializer; update references to
rIndirectInputScheme, OnStartup, registerIndirectInputScheme(),
registerInputScheme(), and initFetchers() accordingly.
In `@src/libfetchers/mercurial.cc`:
- Around line 378-383: The Mercurial fetcher is being registered twice due to
the OnStartup static rMercurialInputScheme plus the initFetchers() path; remove
the duplicate registration by deleting the OnStartup usage and relying on
initFetchers() (or vice versa) so only one registration occurs. Specifically,
eliminate the rMercurialInputScheme = OnStartup(...) static or stop calling
registerMercurialInputScheme() from initFetchers(), ensuring only one of the
symbols (rMercurialInputScheme, registerMercurialInputScheme, or the
initFetchers registration entry) performs the
registerInputScheme(std::make_unique<MercurialInputScheme()) call.
In `@src/libfetchers/path.cc`:
- Around line 174-179: Static initialization via rPathInputScheme (OnStartup)
plus the explicit call to registerPathInputScheme() from initFetchers() causes
double registration and a thrown error from registerInputScheme; fix by making
registration idempotent or removing the automatic startup. Either remove
rPathInputScheme/OnStartup so registerPathInputScheme() is only invoked from
initFetchers(), or guard registerPathInputScheme() with a static bool (or check
registerInputScheme for existence) so repeated calls to
registerPathInputScheme() simply return without re-registering; reference
registerPathInputScheme, rPathInputScheme, OnStartup, registerInputScheme, and
initFetchers when applying the change.
In `@src/libfetchers/tarball.cc`:
- Around line 521-522: Remove the OnStartup wrappers to prevent duplicate
registration: replace the static initializers using OnStartup
(rTarballInputScheme and rFileInputScheme) with direct calls to
registerTarballInputScheme() and registerFileInputScheme() so the schemes are
registered exactly once (matching how initFetchers() performs registration);
update the lines referencing OnStartup to static auto rTarballInputScheme =
registerTarballInputScheme(); and static auto rFileInputScheme =
registerFileInputScheme().
| /** | ||
| * @brief Initialize the Nix fetchers. | ||
| * @ingroup libfetcher_init | ||
| * | ||
| * This function must be called at least once, | ||
| * at some point before using a fetcher for the first time. | ||
| * This function can be called multiple times, and it idempotent. | ||
| * | ||
| * @param[out] context Optional, stores error information | ||
| * @return NIX_OK if the initialization was successful, an error code otherwise. | ||
| */ | ||
| nix_err nix_libfetchers_init(nix_c_context * context); |
There was a problem hiding this comment.
Minor documentation issues.
- Line 30: Grammar issue - "it idempotent" should be "it is idempotent"
- Line 26: The
@ingroup libfetcher_initreferences a group that doesn't appear to be defined. The file's@defgroupon line 3 useslibfetchers. Consider using@ingroup libfetchersfor consistency, or define alibfetcher_initsubgroup.
Suggested fix
/**
* `@brief` Initialize the Nix fetchers.
- * `@ingroup` libfetcher_init
+ * `@ingroup` libfetchers
*
* This function must be called at least once,
* at some point before using a fetcher for the first time.
- * This function can be called multiple times, and it idempotent.
+ * This function can be called multiple times, and it is idempotent.
*
* `@param`[out] context Optional, stores error information
* `@return` NIX_OK if the initialization was successful, an error code otherwise.
*/📝 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.
| /** | |
| * @brief Initialize the Nix fetchers. | |
| * @ingroup libfetcher_init | |
| * | |
| * This function must be called at least once, | |
| * at some point before using a fetcher for the first time. | |
| * This function can be called multiple times, and it idempotent. | |
| * | |
| * @param[out] context Optional, stores error information | |
| * @return NIX_OK if the initialization was successful, an error code otherwise. | |
| */ | |
| nix_err nix_libfetchers_init(nix_c_context * context); | |
| /** | |
| * `@brief` Initialize the Nix fetchers. | |
| * `@ingroup` libfetchers | |
| * | |
| * This function must be called at least once, | |
| * at some point before using a fetcher for the first time. | |
| * This function can be called multiple times, and it is idempotent. | |
| * | |
| * `@param`[out] context Optional, stores error information | |
| * `@return` NIX_OK if the initialization was successful, an error code otherwise. | |
| */ | |
| nix_err nix_libfetchers_init(nix_c_context * context); |
🤖 Prompt for AI Agents
In `@src/libfetchers-c/nix_api_fetchers.h` around lines 24 - 35, Fix the doc
comment for nix_libfetchers_init: correct the grammar "it idempotent" to "it is
idempotent" and change the `@ingroup` tag from libfetcher_init to the existing
group name libfetchers (or alternatively define a libfetcher_init subgroup if
that was intended) so the group reference matches the file's `@defgroup`; update
only the comment block above the nix_libfetchers_init declaration.
| void initFetchers() | ||
| { | ||
| registerTarballInputScheme(); | ||
| registerFileInputScheme(); | ||
| registerGitInputScheme(); | ||
| registerGitHubInputScheme(); | ||
| registerGitLabInputScheme(); | ||
| registerSourceHutInputScheme(); | ||
| registerPathInputScheme(); | ||
| registerIndirectInputScheme(); | ||
| registerMercurialInputScheme(); | ||
| } |
There was a problem hiding this comment.
Double registration issue affects all input schemes.
This initFetchers() function calls all register*InputScheme() functions. However, each of those functions is also called via OnStartup in their respective source files (e.g., tarball.cc lines 521-522, github.cc lines 662-664, path.cc line 174).
Since registerInputScheme() throws on duplicate registration (line 29-30), calling initFetchers() after the OnStartup callbacks have already run will crash.
The OnStartup wrappers in all the individual files should be removed to let initFetchers() be the sole registration mechanism.
🤖 Prompt for AI Agents
In `@src/libfetchers/fetchers.cc` around lines 546 - 557, initFetchers() currently
registers every input scheme by calling registerTarballInputScheme(),
registerGitHubInputScheme(), registerPathInputScheme(), etc., but each of those
is also registered via OnStartup wrappers in their individual modules (e.g.,
tarball.cc, github.cc, path.cc), causing duplicate-registration errors from
registerInputScheme() when initFetchers() runs later; remove the OnStartup
registration wrappers from the individual source files (e.g., the OnStartup
calls in tarball.cc, github.cc, path.cc and any other register*InputScheme()
OnStartup usages) so that initFetchers() is the single place that calls all
register*InputScheme() functions and no duplicate registerInputScheme() calls
occur.
| static auto rGitHubInputScheme = OnStartup([] { registerGitHubInputScheme(); }); | ||
| static auto rGitLabInputScheme = OnStartup([] { registerGitLabInputScheme(); }); | ||
| static auto rSourceHutInputScheme = OnStartup([] { registerSourceHutInputScheme(); }); |
There was a problem hiding this comment.
Remove OnStartup wrappers to avoid double registration.
Same issue as in other fetcher files - these OnStartup wrappers will conflict with initFetchers() calls.
Proposed fix
-static auto rGitHubInputScheme = OnStartup([] { registerGitHubInputScheme(); });
-static auto rGitLabInputScheme = OnStartup([] { registerGitLabInputScheme(); });
-static auto rSourceHutInputScheme = OnStartup([] { registerSourceHutInputScheme(); });
-
void registerGitHubInputScheme()📝 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.
| static auto rGitHubInputScheme = OnStartup([] { registerGitHubInputScheme(); }); | |
| static auto rGitLabInputScheme = OnStartup([] { registerGitLabInputScheme(); }); | |
| static auto rSourceHutInputScheme = OnStartup([] { registerSourceHutInputScheme(); }); | |
| void registerGitHubInputScheme() |
🤖 Prompt for AI Agents
In `@src/libfetchers/github.cc` around lines 662 - 664, These static variables use
OnStartup wrappers which cause duplicate registration when initFetchers() also
registers schemes; remove the OnStartup(...) wrapper and initialize the
variables by directly calling the registration functions instead (replace the
lines for rGitHubInputScheme, rGitLabInputScheme, and rSourceHutInputScheme so
they call registerGitHubInputScheme(), registerGitLabInputScheme(), and
registerSourceHutInputScheme() directly or remove the variables entirely and
rely on initFetchers() to perform registration).
| void initFetchers(); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all occurrences of initFetchers in the header file
echo "=== Declarations in fetchers.hh ==="
rg -n "void initFetchers" src/libfetchers/include/nix/fetchers/fetchers.hh -B 2 -A 2
echo -e "\n=== Implementations in fetchers.cc ==="
rg -n "void initFetchers\|void.*fetchers.*initFetchers" src/libfetchers/fetchers.cc -B 2 -A 10Repository: DeterminateSystems/nix-src
Length of output: 341
🏁 Script executed:
#!/bin/bash
# Check namespace context for declarations
echo "=== Namespace context around line 281-282 in fetchers.hh ==="
sed -n '270,290p' src/libfetchers/include/nix/fetchers/fetchers.hh
echo -e "\n=== Namespace context around line 20-22 in fetchers.hh ==="
sed -n '15,30p' src/libfetchers/include/nix/fetchers/fetchers.hh
echo -e "\n=== Namespace context around implementations in fetchers.cc ==="
sed -n '540,560p' src/libfetchers/fetchers.ccRepository: DeterminateSystems/nix-src
Length of output: 1367
🏁 Script executed:
#!/bin/bash
# Search the entire codebase for all initFetchers implementations
echo "=== All initFetchers function definitions in the codebase ==="
rg -n "^\s*(void\s+)?initFetchers\s*\(" src/libfetchers/ --type cpp --type h -A 3Repository: DeterminateSystems/nix-src
Length of output: 904
Remove or implement the duplicate nix::fetchers::initFetchers() declaration.
The declaration at line 281 in the nix::fetchers namespace lacks a corresponding implementation. The only implementation (in fetchers.cc:546) is in the nix namespace. This will cause linker errors if the nix::fetchers::initFetchers() function is called. Either remove this declaration or provide an implementation. If both namespace versions are needed, have one delegate to the other.
🤖 Prompt for AI Agents
In `@src/libfetchers/include/nix/fetchers/fetchers.hh` around lines 281 - 282, The
header declares nix::fetchers::initFetchers() but the only implementation is
nix::initFetchers() — causing a linker mismatch; either remove the
nix::fetchers::initFetchers() declaration from the header or add a delegating
implementation (nix::fetchers::initFetchers() { return ::nix::initFetchers(); })
so both symbols exist and link correctly; locate the declaration in the
nix::fetchers namespace and either delete it or implement the wrapper to call
the existing nix::initFetchers().
| static auto rTarballInputScheme = OnStartup([] { registerTarballInputScheme(); }); | ||
| static auto rFileInputScheme = OnStartup([] { registerFileInputScheme(); }); |
There was a problem hiding this comment.
Remove OnStartup wrappers to avoid double registration.
Same issue as flagged in path.cc and fetchers.cc - these OnStartup wrappers will cause double registration when initFetchers() is called.
Proposed fix
-static auto rTarballInputScheme = OnStartup([] { registerTarballInputScheme(); });
-static auto rFileInputScheme = OnStartup([] { registerFileInputScheme(); });
-
void registerTarballInputScheme()🤖 Prompt for AI Agents
In `@src/libfetchers/tarball.cc` around lines 521 - 522, Remove the OnStartup
wrappers to prevent duplicate registration: replace the static initializers
using OnStartup (rTarballInputScheme and rFileInputScheme) with direct calls to
registerTarballInputScheme() and registerFileInputScheme() so the schemes are
registered exactly once (matching how initFetchers() performs registration);
update the lines referencing OnStartup to static auto rTarballInputScheme =
registerTarballInputScheme(); and static auto rFileInputScheme =
registerFileInputScheme().
a661669 to
b7fef3b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/libfetchers/fetchers.cc`:
- Around line 546-567: The current initFetchers() uses a non-atomic static bool
guard which can race; replace that pattern with a thread-safe one-time
initialization using a static std::once_flag and std::call_once. Inside
initFetchers(), remove the static bool init and its check/assignment, declare a
static std::once_flag (e.g. fetchers_init_flag) and wrap the registration calls
(registerTarballInputScheme, registerFileInputScheme, registerGitInputScheme,
registerGitHubInputScheme, registerGitLabInputScheme,
registerSourceHutInputScheme, registerPathInputScheme,
registerIndirectInputScheme, registerMercurialInputScheme) in a
std::call_once(fetchers_init_flag, [] { ... }); to ensure registrations run
exactly once across threads.
♻️ Duplicate comments (1)
src/libfetchers-c/nix_api_fetchers.h (1)
24-35: Documentation issues persist from previous review.The past review flagged two issues that remain unaddressed:
- Line 26:
@ingroup libfetcher_initshould be@ingroup libfetchersto match the@defgroupon line 3- Line 30: Grammar error - "and it idempotent" should be "and it is idempotent"
Suggested fix
/** * `@brief` Initialize the Nix fetchers. - * `@ingroup` libfetcher_init + * `@ingroup` libfetchers * * This function must be called at least once, * at some point before using a fetcher for the first time. - * This function can be called multiple times, and it idempotent. + * This function can be called multiple times, and it is idempotent. * * `@param`[out] context Optional, stores error information * `@return` NIX_OK if the initialization was successful, an error code otherwise. */
| namespace nix { | ||
|
|
||
| using namespace nix::fetchers; | ||
|
|
||
| void initFetchers() | ||
| { | ||
| static bool init; | ||
| if (init) | ||
| return; | ||
|
|
||
| init = true; | ||
|
|
||
| registerTarballInputScheme(); | ||
| registerFileInputScheme(); | ||
| registerGitInputScheme(); | ||
| registerGitHubInputScheme(); | ||
| registerGitLabInputScheme(); | ||
| registerSourceHutInputScheme(); | ||
| registerPathInputScheme(); | ||
| registerIndirectInputScheme(); | ||
| registerMercurialInputScheme(); | ||
| } |
There was a problem hiding this comment.
Make initFetchers() thread-safe (static bool causes a data race).
Line 552 uses a non-atomic static bool guard. If initFetchers() is reached concurrently (now possible via Input::fromURL / fromAttrs), two threads can pass the guard, causing double-registration or UB. Prefer std::once_flag + std::call_once.
🔧 Suggested fix (thread-safe one-time init)
+#include <mutex>
...
void initFetchers()
{
- static bool init;
- if (init)
- return;
-
- init = true;
-
- registerTarballInputScheme();
- registerFileInputScheme();
- registerGitInputScheme();
- registerGitHubInputScheme();
- registerGitLabInputScheme();
- registerSourceHutInputScheme();
- registerPathInputScheme();
- registerIndirectInputScheme();
- registerMercurialInputScheme();
+ static std::once_flag once;
+ std::call_once(once, [] {
+ registerTarballInputScheme();
+ registerFileInputScheme();
+ registerGitInputScheme();
+ registerGitHubInputScheme();
+ registerGitLabInputScheme();
+ registerSourceHutInputScheme();
+ registerPathInputScheme();
+ registerIndirectInputScheme();
+ registerMercurialInputScheme();
+ });
}📝 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.
| namespace nix { | |
| using namespace nix::fetchers; | |
| void initFetchers() | |
| { | |
| static bool init; | |
| if (init) | |
| return; | |
| init = true; | |
| registerTarballInputScheme(); | |
| registerFileInputScheme(); | |
| registerGitInputScheme(); | |
| registerGitHubInputScheme(); | |
| registerGitLabInputScheme(); | |
| registerSourceHutInputScheme(); | |
| registerPathInputScheme(); | |
| registerIndirectInputScheme(); | |
| registerMercurialInputScheme(); | |
| } | |
| namespace nix { | |
| using namespace nix::fetchers; | |
| void initFetchers() | |
| { | |
| static std::once_flag once; | |
| std::call_once(once, [] { | |
| registerTarballInputScheme(); | |
| registerFileInputScheme(); | |
| registerGitInputScheme(); | |
| registerGitHubInputScheme(); | |
| registerGitLabInputScheme(); | |
| registerSourceHutInputScheme(); | |
| registerPathInputScheme(); | |
| registerIndirectInputScheme(); | |
| registerMercurialInputScheme(); | |
| }); | |
| } |
🤖 Prompt for AI Agents
In `@src/libfetchers/fetchers.cc` around lines 546 - 567, The current
initFetchers() uses a non-atomic static bool guard which can race; replace that
pattern with a thread-safe one-time initialization using a static std::once_flag
and std::call_once. Inside initFetchers(), remove the static bool init and its
check/assignment, declare a static std::once_flag (e.g. fetchers_init_flag) and
wrap the registration calls (registerTarballInputScheme,
registerFileInputScheme, registerGitInputScheme, registerGitHubInputScheme,
registerGitLabInputScheme, registerSourceHutInputScheme,
registerPathInputScheme, registerIndirectInputScheme,
registerMercurialInputScheme) in a std::call_once(fetchers_init_flag, [] { ...
}); to ensure registrations run exactly once across threads.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/libexpr/include/nix/expr/primops.hh`:
- Around line 24-27: The member primOp is currently declared as an rvalue
reference (PrimOp && primOp) which can dangle; change the member to store PrimOp
by value (PrimOp primOp) and update the constructor initializer to move the
parameter into the member (primOp(std::move(primOp))) so that ensure() and
primOps().push_back(primOp) own a valid object; keep the existing registered
bool and ensure() usage unchanged.
In `@src/libexpr/primops.cc`:
- Around line 5205-5219: The ensure() method in RegisterPrimOp mutates
registered and primOps() without synchronization; wrap the body of
RegisterPrimOp::ensure() with a mutex to make the check-then-act atomic: add a
(likely static) std::mutex (or std::recursive_mutex if needed) used by
RegisterPrimOp::ensure(), acquire a std::lock_guard<std::mutex> at the start of
ensure(), then re-check registered and, if false, push primOp into primOps() and
set registered = true; include <mutex> and keep the lock scope minimal to cover
the check and mutation only.
| void ensure(); | ||
| private: | ||
| bool registered; | ||
| PrimOp && primOp; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find RegisterPrimOp class definition and constructor
rg -n "class RegisterPrimOp|struct RegisterPrimOp" -A 20 --type cpp --type hRepository: DeterminateSystems/nix-src
Length of output: 1537
🏁 Script executed:
#!/bin/bash
# Find RegisterPrimOp::ensure() implementation
rg -n "RegisterPrimOp::ensure|void ensure" -B 2 -A 10 --type cpp --type hRepository: DeterminateSystems/nix-src
Length of output: 6538
🏁 Script executed:
#!/bin/bash
# Search for RegisterPrimOp instantiation patterns to see if temporaries are used
rg -n "RegisterPrimOp\s*[({]|new RegisterPrimOp" -B 2 -A 2 --type cppRepository: DeterminateSystems/nix-src
Length of output: 1103
🏁 Script executed:
#!/bin/bash
# Check if registered member is initialized anywhere
rg -n "registered\s*=" -C 2 --type cpp --type h | head -30Repository: DeterminateSystems/nix-src
Length of output: 269
🌐 Web query:
C++ rvalue reference data members lifetime semantics constructor temporary binding
💡 Result:
In C++, binding a temporary to a reference data member (including an T&& rvalue-reference member) generally does not extend the temporary’s lifetime to the lifetime of the containing object. The temporary typically dies at the end of the full-expression that performed the construction, leaving the member dangling. [1]
The rule (what does and doesn’t get lifetime extension)
A temporary’s lifetime is extended when it is directly bound to a reference in certain contexts (e.g., a local const T& r = T{...}; or T&& r = T{...};). But there are important exceptions, including:
- A temporary bound to a reference in a member initializer (
mem-initializer) (e.g.S(T&& x) : r(std::move(x)) {}or: r(T{}) {}) lives only until the end of the full-expression containing the construction (i.e., through the;ofS s(...);), not as long asslives. [1]
Consequence for rvalue-reference data members
struct S {
std::string&& r;
S(std::string&& x) : r(std::move(x)) {}
};
S make() { return S(std::string("hi")); } // r will dangle after the full-expressionstd::string("hi") is a temporary; S::r ends up referring to it, but that temporary is destroyed at the end of the statement that creates the S. [1]
A subtle special case: aggregate initialization in C++20
For aggregates, C++20 introduced a distinction:
struct A { int&& r; };
A a1{7}; // lifetime *is* extended (braces) [1]
A a2(7); // well-formed, but r dangles (parens) [1]So brace-init aggregate can extend the lifetime in this specific scenario, but constructor-style init does not. [1]
Why expression-template code can “work”
Expression-template libraries avoid dangling by ensuring that:
- they only store references to operands that outlive the expression object, and/or
- they capture rvalues by value (owning storage) while referencing lvalues, often via perfect-forwarding wrappers.
If you store an rvalue-reference member to a temporary operand, you’ll reproduce the dangling behavior discussed in practice. [2]
Practical guidance
- Don’t use
T&&(orconst T&) data members to “hold” temporaries unless you prove the referred object outlives the containing object. - Prefer storing by value (possibly with move) for ownership:
struct S { std::string v; S(std::string x) : v(std::move(x)) {} };
Sources: cppreference on temporary lifetime extension and the “reference member in new-expression / construction” exception (and aggregate brace/parens difference) [1], plus a representative discussion showing the dangling behavior with rvalue-reference members in practice [2].
[1] Reference initialization & temporary lifetime notes (cppreference)
[2] “Data member and rvalue life-time” discussion (Stack Overflow)
Store PrimOp by value to prevent dangling references.
The member PrimOp && primOp binds to an rvalue reference parameter that may itself be bound to a temporary. Per C++ semantics, temporaries bound to reference data members in the initializer list do not have their lifetime extended—they are destroyed at the end of the constructor's full-expression. This leaves the member reference dangling when ensure() later accesses it via primOps().push_back(primOp).
The constructor correctly initializes registered to false, but storing primOp by value is necessary to own the object safely.
Suggested fix
- bool registered;
- PrimOp && primOp;
+ bool registered = false;
+ PrimOp primOp;Update the constructor initializer to primOp(std::move(primOp)), which will move-assign the parameter into the by-value member.
🤖 Prompt for AI Agents
In `@src/libexpr/include/nix/expr/primops.hh` around lines 24 - 27, The member
primOp is currently declared as an rvalue reference (PrimOp && primOp) which can
dangle; change the member to store PrimOp by value (PrimOp primOp) and update
the constructor initializer to move the parameter into the member
(primOp(std::move(primOp))) so that ensure() and primOps().push_back(primOp) own
a valid object; keep the existing registered bool and ensure() usage unchanged.
| RegisterPrimOp::RegisterPrimOp(PrimOp && primOp) | ||
| : registered(false) | ||
| , primOp(std::move(primOp)) | ||
| { | ||
| primOps().push_back(std::move(primOp)); | ||
| ensure(); | ||
| } | ||
|
|
||
| void RegisterPrimOp::ensure() | ||
| { | ||
| if (registered) | ||
| return; | ||
|
|
||
| primOps().push_back(primOp); | ||
| registered = true; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate the file and check the RegisterPrimOp class definition
find . -name "primops.cc" -o -name "primops.h" | head -20Repository: DeterminateSystems/nix-src
Length of output: 125
🏁 Script executed:
# Search for RegisterPrimOp class definition
rg "class RegisterPrimOp" -A 10 -B 2Repository: DeterminateSystems/nix-src
Length of output: 52
🏁 Script executed:
# Search for where initPrimOps is called
rg "initPrimOps\(" -nRepository: DeterminateSystems/nix-src
Length of output: 221
🏁 Script executed:
# Search for primOps function definition
rg "primOps\(\)" -B 3 -A 5 --max-count=10Repository: DeterminateSystems/nix-src
Length of output: 1983
🏁 Script executed:
# Check the context where initPrimOps() is called
sed -n '335,350p' src/libexpr/eval.ccRepository: DeterminateSystems/nix-src
Length of output: 595
🏁 Script executed:
# Find RegisterPrimOp class definition
rg "struct RegisterPrimOp|class RegisterPrimOp" -A 15Repository: DeterminateSystems/nix-src
Length of output: 1110
🏁 Script executed:
# Search for where RegisterPrimOp objects are instantiated
rg "RegisterPrimOp\{" -B 3 -A 3Repository: DeterminateSystems/nix-src
Length of output: 52
🏁 Script executed:
# Check the EvalState constructor to see if it's called from multiple threads
rg "EvalState::" src/libexpr/eval.cc | head -20Repository: DeterminateSystems/nix-src
Length of output: 1220
🏁 Script executed:
# Find initPrimOps() implementation
sed -n '5535,5600p' src/libexpr/primops.ccRepository: DeterminateSystems/nix-src
Length of output: 2001
🏁 Script executed:
# Search for RegisterPrimOp constructor calls more thoroughly
rg "RegisterPrimOp" src/libexpr/primops.cc | head -30Repository: DeterminateSystems/nix-src
Length of output: 1262
🏁 Script executed:
# Look for global RegisterPrimOp instances
rg "^.*RegisterPrimOp" src/libexpr/primops.cc | grep -v "//" | head -20Repository: DeterminateSystems/nix-src
Length of output: 866
🏁 Script executed:
# Check for any threading/concurrency related comments or patterns in eval.hh
rg "thread|concurrent|lock|mutex|atomic" src/libexpr/include/nix/expr/eval.hhRepository: DeterminateSystems/nix-src
Length of output: 592
🏁 Script executed:
# Look for any synchronization in the codebase for primop initialization
rg "primOps\|RegisterPrimOp" src/libexpr/primops.cc | grep -i "lock\|mutex\|atomic"Repository: DeterminateSystems/nix-src
Length of output: 52
Add synchronization to RegisterPrimOp::ensure() for thread-safe concurrent initialization.
The registered field and primOps() vector are mutated without synchronization. Since initPrimOps() is called in the EvalState constructor and EvalState can be constructed concurrently from multiple threads (as evidenced by concurrent data structures elsewhere in the codebase), this creates a data race. The check-then-act pattern (if (registered) return; ... registered = true;) without locking is unsafe under concurrent access.
🔒 Suggested fix (mutex-guarded registration)
+#include <mutex>
...
void RegisterPrimOp::ensure()
{
+ static std::mutex primOpsMutex;
+ std::lock_guard<std::mutex> lock(primOpsMutex);
if (registered)
return;
primOps().push_back(primOp);
registered = true;
}🤖 Prompt for AI Agents
In `@src/libexpr/primops.cc` around lines 5205 - 5219, The ensure() method in
RegisterPrimOp mutates registered and primOps() without synchronization; wrap
the body of RegisterPrimOp::ensure() with a mutex to make the check-then-act
atomic: add a (likely static) std::mutex (or std::recursive_mutex if needed)
used by RegisterPrimOp::ensure(), acquire a std::lock_guard<std::mutex> at the
start of ensure(), then re-check registered and, if false, push primOp into
primOps() and set registered = true; include <mutex> and keep the lock scope
minimal to cover the check and mutation only.
7c6e6ba to
5600f5f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
nix-meson-build-support/default-system-cpu/meson.build (1)
13-18: Change'arm64'to'aarch64'in the cpu_family condition.Meson's canonical
cpu_familyvalue for Apple Silicon (and 64-bit ARM generally) is'aarch64', not'arm64'. The current condition checks for'arm64'which will never match, causing Apple Silicon machines to fall through to other branches and produce incorrect system strings. Update the condition tohost_machine.cpu_family() in [ 'arm', 'aarch64' ].
🤖 Fix all issues with AI agents
In `@src/nix/main.cc`:
- Around line 44-53: The current preprocessor blocks using __arm__ and __arm64__
unconditionally invoke `#error` (with macros STRINGIFY/TOSTRING) which breaks all
ARM/aarch64 builds; modify the blocks around the symbols __arm__ and __arm64__
to be conditional on an explicit opt-in debug macro (e.g., NIX_ARM_DIAGNOSTIC or
ENABLE_DEBUG_DIAGNOSTICS) or remove the `#error` entirely, and replace the
informal error text with a professional diagnostic via `#pragma` message (or log)
if the debug macro is set; ensure STRINGIFY and TOSTRING are still used for
printing NIX_LOCAL_SYSTEM, and keep the diagnostic gated so normal builds on ARM
succeed.
77850bc to
83c2e4f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/libexpr/meson.build`:
- Around line 82-91: The problem is that link_args is a list but the += is
adding a string when wasmtime is enabled; modify the wasmtime_required.enabled()
branch to add the flag as a one-element list or use the list append method
instead of concatenating a raw string — update the block that references
link_args and wasmtime_required (the link_args variable and
wasmtime_required.enabled() conditional) to use either link_args +=
['-lwasmtime'] or link_args.append('-lwasmtime') so Meson receives a list.
39f7fd3 to
a7f926c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@nix-meson-build-support/common/meson.build`:
- Around line 68-75: The variable cxx (set via meson.get_compiler('cpp')) is
declared too late; move the statement cxx = meson.get_compiler('cpp') to the top
of the meson.build file before any use (before the first occurrence where cxx is
referenced) so all subsequent checks (the clang/static-library prelink logic
using cxx.get_id(), and other uses currently at the earlier occurrences) see a
defined compiler instance; remove or avoid the later duplicate assignment at its
current location and ensure all references use that single top-level cxx
binding.
a7f926c to
7f7fef6
Compare
6f015ee to
65030cd
Compare
65030cd to
1f2a242
Compare
|
This all looks roughly fine to me. But I'll leave it to Eelco (as the more knowledgeable about this codebase ;p) for final approval. |
a06c8c2 to
afca2cf
Compare
This branch was previously deployed
3 participants
Motivation
Enables static builds so static binaries can be made.
Context
Summary by CodeRabbit
New Features
Improvements
Bug Fixes