Sync with upstream 2.31.2 #208
Conversation
(cherry picked from commit 6bdb5e8)
…/pr-13900 Fix downstream MinGW build by not looking for Boost Regex (backport NixOS#13900)
On macOS, poll() is fundamentally broken for HUP detection. It loses event subscriptions when EVFILT_READ fires without matching the requested events in the pollfd. This causes daemon processes to linger after client disconnect. This commit replaces poll() with kqueue on macOS, which is what poll() uses internally but without the bugs. The kqueue implementation uses EVFILT_READ which works for both sockets and pipes, avoiding EVFILT_SOCK which only works for sockets. On Linux and other platforms, we continue using poll() with the standard POSIX behavior where POLLHUP is always reported regardless of requested events. Based on work from the Lix project (https://git.lix.systems/lix-project/lix) commit 69ba3c92db3ecca468bcd5ff7849fa8e8e0fc6c0 Fixes: NixOS#13847 Related: https://git.lix.systems/lix-project/lix/issues/729 Apple bugs: rdar://37537852 (poll), FB17447257 (poll) Co-authored-by: Jade Lovelace <[email protected]> (cherry picked from commit 1286d5d)
…/pr-13901 Fix macOS HUP detection using kqueue instead of poll (backport NixOS#13901)
This is relied upon (specifically the `local` store) by existing tooling [1] and we broke this in 3e7879e (which was first released in 2.31). To lessen the scope of the breakage we should not normalize "auto" references and explicitly specified references like "local" or "daemon". It also makes sense to canonicalize local://,daemon:// to be more compatible with prior behavior. [1]: https://github.com/maralorn/nix-output-monitor/blob/05e1b3cba2fa328a1781390a4e4515e9c432229e/lib/NOM/Builds.hs#L60-L64 (cherry picked from commit 3513ab1)
…/pr-13911 libstore: Do not normalize daemon -> unix://, local -> local:// (backport NixOS#13911)
This implements a special back-compat shim to specifically allow unbracketed IPv6 addresses in store references. This is something that is relied upon in the wild and the old parsing logic accepted both ways (brackets were optional). This patch restores this behavior. As always, we didn't have any tests for this. Addresses NixOS#13937. (cherry picked from commit 7cc654a)
…/pr-13940 libstore: Reallow unbracketed IPv6 addresses in store references (backport NixOS#13940)
(cherry picked from commit 258d41b)
I couldn't come up with a test that failed before this, but my existing test still passes so 🤷 (cherry picked from commit 9c832a0)
…/pr-13934 Fix flake registry ignoring `dir` parameter (backport NixOS#13934)
Will not pass until the next commit. (cherry picked from commit ed6ef7c)
This is handled similarly in the handler for `--override-flake` in `MixEvalArgs`. (cherry picked from commit 38663fb)
…/pr-13939 Pass `dir` in extraAttrs when overriding the registry (backport NixOS#13939)
(cherry picked from commit bdbc739)
…/pr-13966 meson: add soversion to libraries (NixOS#13960) (backport NixOS#13966)
This reverts commit bdbc739. Such a change needs more thought put into it. By versioning shared libraries we'd make a false impression that libraries themselves are actually versioned and have some sort of stable ABI, which is not the case. This will be useful when C bindings become stable, but as long as they are experimental it does not make sense to set SONAME. Also this change should not have been backported, since it's severely breaking. (cherry picked from commit 0db2b8c)
This allows the weird network or DNS server fallback mechanism inside
glibc to work, and prevents a "Resolving timed out after 5000
milliseconds" error. Read on for details.
The DNS request stuff (dns-hosts) in glibc uses this fallback procedure
to minimize network RTT in the ideal case while dealing with
ill-behaving networks and DNS servers gracefully (see resolv.conf(5)):
- Use sendmmsg() to send UDP DNS requests for IPv4 and IPv6 in parallel
- If that times out (meaning that none or only one of the responses have
been received), send the requests one by one, waiting for the response
before sending the next request ("single-request")
- If that still times out, try to use a different socket (hence
different address) for each request ("single-request-reopen")
The default timeout inside glibc is 5 seconds. Therefore, setting
connect-timeout, and therefore CURLOPT_CONNECTTIMEOUT to 5 seconds
prevents the single-request fallback, and setting it to even 10 seconds
prevents the single-request-reopen fallback as well.
The fallback decision is saved by glibc, but only thread-locally, and
libcurl starts a new thread for getaddrinfo() for each connection.
Therefore for every connection the fallback starts from sendmmsg() all
over again. And since these are considered to have timed out by libcurl,
even though getaddrinfo() might return a successful result, it is not
cached in libcurl.
While a user could tweak these with resolv.conf(5) options (e.g. using
networking.resolvconf.extraOptions in NixOS), and indeed that is
probably needed to avoid annoying delays, it still means that the
default connect-timeout of 5 is too low. Raise it to give fallback a
chance.
(cherry picked from commit 7295034)
…/pr-13985 libstore: Raise default connect-timeout to 15 secs (backport NixOS#13985)
With the migration to /nix/var/nix/builds we now have failing builds when the derivation name is too long. This change removes the derivation name from the temporary build to have a predictable prefix length: Also see: NixOS/infra#764 for context. (cherry picked from commit 725a2f3)
…/pr-13839 don't include derivation name in temporary build directories (backport NixOS#13839)
…/pr-14009 Revert "tests/nixos: Fix daemon store reference in authorization test" (backport NixOS#14009)
Tagging release 2.31.2
WalkthroughIntroduces new StoreReference variants (Daemon, Local) and updates parsing/rendering to normalize certain URIs to “daemon” and “local”. Adjusts LocalStore/UDSRemoteStore to emit these variants. Refactors MonitorFdHup into OS-specific thread routines. Tweaks Boost modules in Meson. Raises file-transfer connectTimeout. Adds tests and data; bumps version. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant LSC as LocalStoreConfig
participant URC as UDSRemoteStoreConfig
participant SR as StoreReference
Note over SR: New variants: Daemon, Local
Caller->>LSC: getReference()
LSC->>LSC: getQueryParams()
alt params empty
LSC-->>Caller: SR::Local
else params present
LSC-->>Caller: SR::Specified(scheme, params)
end
Caller->>URC: getReference(path)
alt path == nixDaemonSocketFile
URC-->>Caller: SR::Daemon
else
URC-->>Caller: SR::Specified(scheme="unix", authority=path)
end
Caller->>SR: render()
alt Variant = Local
SR-->>Caller: "local"
else Variant = Daemon
SR-->>Caller: "daemon"
else Variant = Specified
SR-->>Caller: "scheme://authority[?params]"
else Variant = Auto
SR-->>Caller: "auto"
end
sequenceDiagram
autonumber
actor User
participant MFH as MonitorFdHup
participant Thr as Watch Thread
participant OS as OS I/O (kqueue|poll)
User->>MFH: new MonitorFdHup(fd)
MFH->>MFH: create notifyPipe()
MFH->>Thr: start(runThread(fd, notifyFd))
Thr->>OS: wait(watchFd, notifyFd)
alt watchFd HUP/EOF
OS-->>Thr: event(HUP/EOF)
Thr-->>MFH: exit thread
else notifyFd closed
OS-->>Thr: event(notify)
Thr-->>MFH: exit thread
end
User->>MFH: ~MonitorFdHup()
MFH->>MFH: close notify write end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.version (1)
1-2: Version bump to 2.31.2 — update leftover 2.31.1 referencesFound explicit 2.31.1 references; replace with 2.31.2 or confirm they intentionally reference upstream 2.31.1:
- doc/manual/source/release-notes-determinate/v3.11.2.md:3 — "* Based on upstream Nix 2.31.1."
- doc/manual/source/release-notes-determinate/v3.11.0.md:3 — "- Based on upstream Nix 2.31.1."
- doc/manual/source/release-notes-determinate/v3.10.1.md:3 — "* Based on upstream Nix 2.31.1."
- doc/manual/source/release-notes-determinate/v3.10.1.md:6 — "This release rebases Determinate Nix on upstream Nix 2.31.1."
- doc/manual/source/release-notes-determinate/v3.11.1.md:3 — "* Based on upstream Nix 2.31.1."
🧹 Nitpick comments (6)
tests/nixos/authorization.nix (1)
85-85: Updated expected error to 'daemon': LGTM; make grep a touch stricter.Limit to first match to reduce flakiness in noisy logs.
Apply:
- grep -F "cannot open connection to remote store 'daemon'" diag + grep -F -m1 "cannot open connection to remote store 'daemon'" diagtests/functional/store-info.sh (1)
16-21: Normalization rules look good; tiny pattern consistency nit.Optional: quote bare words for symmetry with other patterns.
- local | 'local://' ) + 'local' | 'local://' ) @@ - daemon | 'unix://' ) + 'daemon' | 'unix://' )src/libutil/unix/include/nix/util/monitor-fd.hh (2)
31-36: Join-guard for destructor (defensive).If construction ever evolves to conditional thread start, guard join.
~MonitorFdHup() { // Close the write side to signal termination via POLLHUP notifyPipe.writeSide.close(); - thread.join(); + if (thread.joinable()) thread.join(); }
55-95: macOS kqueue path: consider EV_ERROR handling.If kevent reports EV_ERROR on the watched fds, current loop ignores it and spins. Optionally handle EV_ERROR similarly to EOF (exit, maybe with a debug log).
src/libstore/uds-remote-store.cc (1)
60-71: "daemon" rendering is fine; watch live-reload semanticsLGTM. Minor: constructor eagerly copies settings.nixDaemonSocketFile into path; if that setting can change at runtime, getReference() won’t reflect it. Consider tracking “uses default daemon path” as a flag to keep behavior aligned with the comment above the delegating ctor.
src/libstore/store-reference.cc (1)
22-45: Render/parse updates are sensible; consider handling authority/path split in legacy IPv6 forms
- Rendering of Daemon/Local is correct.
- Fallback parser and IPv6 bracketization look good.
Minor enhancement: when handling legacy forms without "://", split authority from path before IPv6 detection so inputs like "ssh:user@fe80::1/some/path" can be normalized instead of rejected.
Example tweak (conceptual; adjust to local helpers):
- auto authorityString = schemeAndAuthority->authority; + auto authorityString = schemeAndAuthority->authority; + auto slashPos = authorityString.find('/'); + std::string_view authorityOnly = slashPos == std::string::npos + ? authorityString + : authorityString.substr(0, slashPos); + std::string_view pathRemainder = slashPos == std::string::npos + ? std::string_view{} + : authorityString.substr(slashPos); - auto maybeIpv6 = boost::urls::parse_ipv6_address(authorityString); + auto maybeIpv6 = boost::urls::parse_ipv6_address(authorityOnly); if (maybeIpv6) { std::string fixedAuthority; if (userinfo) { fixedAuthority += *userinfo; fixedAuthority += '@'; } fixedAuthority += '['; - fixedAuthority += authorityString; + fixedAuthority += authorityOnly; fixedAuthority += ']'; return { .variant = Specified{ .scheme = std::string(schemeAndAuthority->scheme), - .authority = fixedAuthority, + .authority = fixedAuthority + std::string(pathRemainder), }, .params = std::move(params), }; }Add a test exercising "ssh:user@fe80::1/path" legacy input and expected normalized output.
Also applies to: 47-69, 97-110, 119-145
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
.version(1 hunks)src/libexpr/meson.build(1 hunks)src/libstore-tests/data/store-reference/daemon_shorthand.txt(1 hunks)src/libstore-tests/data/store-reference/local_shorthand_3.txt(1 hunks)src/libstore-tests/data/store-reference/ssh_unbracketed_ipv6_1.txt(1 hunks)src/libstore-tests/data/store-reference/ssh_unbracketed_ipv6_2.txt(1 hunks)src/libstore-tests/data/store-reference/ssh_unbracketed_ipv6_3.txt(1 hunks)src/libstore-tests/local-store.cc(1 hunks)src/libstore-tests/store-reference.cc(2 hunks)src/libstore-tests/uds-remote-store.cc(1 hunks)src/libstore/include/nix/store/filetransfer.hh(1 hunks)src/libstore/include/nix/store/store-reference.hh(1 hunks)src/libstore/local-store.cc(1 hunks)src/libstore/meson.build(1 hunks)src/libstore/store-api.cc(1 hunks)src/libstore/store-reference.cc(5 hunks)src/libstore/uds-remote-store.cc(1 hunks)src/libutil/meson.build(1 hunks)src/libutil/unix/include/nix/util/monitor-fd.hh(2 hunks)tests/functional/store-info.sh(2 hunks)tests/nixos/authorization.nix(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/libstore/local-store.cc (3)
src/libstore/http-binary-cache-store.cc (2)
uriSchemes(12-19)uriSchemes(12-12)src/libstore/dummy-store.cc (1)
uriSchemes(36-44)src/libstore/local-binary-cache-store.cc (2)
uriSchemes(118-124)uriSchemes(118-118)
src/libstore/uds-remote-store.cc (3)
src/libstore/http-binary-cache-store.cc (2)
uriSchemes(12-19)uriSchemes(12-12)src/libstore/dummy-store.cc (1)
uriSchemes(36-44)src/libstore/local-binary-cache-store.cc (2)
uriSchemes(118-124)uriSchemes(118-118)
⏰ 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 / build
- GitHub Check: build_x86_64-linux / build
🔇 Additional comments (17)
src/libstore-tests/data/store-reference/ssh_unbracketed_ipv6_1.txt (1)
1-1: IPv6 unbracketed authority test input: LGTM.Pairs well with normalization to bracketed form in parser.
src/libstore-tests/data/store-reference/local_shorthand_3.txt (1)
1-1: Local shorthand fixture: LGTM.Matches new Local variant rendering.
src/libstore-tests/data/store-reference/ssh_unbracketed_ipv6_2.txt (1)
1-1: IPv6 with userinfo (unbracketed) fixture: LGTM.Complements coverage of authority parsing.
src/libstore-tests/data/store-reference/ssh_unbracketed_ipv6_3.txt (1)
1-1: IPv6 with query (unbracketed) fixture: LGTM.Good edge-case for preserving query during normalization.
src/libstore-tests/data/store-reference/daemon_shorthand.txt (1)
1-1: Fixture added: LGTM — trailing newline present.
All listed fixtures end with a newline; no changes needed.src/libutil/meson.build (1)
60-65: Formatting-only Boost modules change: LGTM — verify CI Boost >= 1.82 provides the 'url' modulerg returned no Boost dependency declarations in the repo; confirm CI images/install provide Boost >= 1.82 with the url component enabled and that all meson.build files declare the same modules list.
src/libexpr/meson.build (1)
43-47: LGTM: formatting-only Boost modules list.No functional changes; consistent with Meson style.
src/libstore/include/nix/store/filetransfer.hh (1)
33-49: Behavior change: connect-timeout default raised to 15s; note release/test impact.This affects network behavior and potentially test timings. Please:
- Call out in release notes and update any docs/examples that mention defaults.
- Verify CI flakes/timeouts relying on the prior 5s don’t regress.
tests/functional/store-info.sh (2)
26-31: Canonicalization for query forms is correct.local? → local://?... and daemon? → unix://?... match the new normalization behavior.
47-55: Tests align with new local/daemon normalization.Good coverage of both shorthand and scheme forms.
src/libstore/meson.build (1)
111-116: Verify removal of Boost.Regex and addition of Boost.URL.Automated search produced no output — manual verification required. Confirm no sources or targets still include/use Boost.Regex and that Boost.URL is present/supported on every CI builder.
Run locally:
- rg -n --hidden -S '(^\s*#\sinclude\s<boost/regex.hpp>|\bboost::regex\b|\bboost::smatch\b|\bboost::regex_match\b|\bboost::regex_search\b)'
- rg -n --hidden -S '(^\s*#\sinclude\s<boost/(url|urls).hpp>|\bboost::urls?::)'
Check: src/libstore/meson.build (lines 111–116).
src/libstore-tests/uds-remote-store.cc (1)
19-24: Test covers daemon shorthand; consider adding explicit-path caseLGTM. Please also add a case where authority equals settings.nixDaemonSocketFile to ensure getReference() renders "daemon" when the daemon path is explicitly provided.
src/libstore-tests/local-store.cc (1)
36-41: Good coverage of local shorthand; add a params caseLGTM. Add a test with a non-empty query param (e.g., trusted=true) to assert rendering becomes "local://?trusted=true".
src/libstore/local-store.cc (1)
460-471: Return Local variant on empty params — looks goodChange is correct and reduces churn in rendered URIs for legacy tooling.
src/libstore-tests/store-reference.cc (1)
110-116: Solid new cases; add write-path tests for IPv6 normalizationTests for Local/Daemon shorthands and IPv6 parsing look good. Please add URI_TEST_WRITE for the IPv6 cases to assert we render bracketed hosts consistently.
Also applies to: 144-185
src/libstore/include/nix/store/store-reference.hh (1)
67-90: New Daemon/Local variants are clean and minimalDeriving from Specified keeps compatibility. No issues.
src/libstore/store-api.cc (1)
826-833: Incorrect — Specified overload already covers Daemon/LocalDaemon and Local publicly derive from Specified (src/libstore/include/nix/store/store-reference.hh), so the lambda taking const StoreReference::Specified& will bind to Daemon/Local values; the std::visit here is correct and no change is required.
Likely an incorrect or invalid review comment.
| #ifdef __APPLE__ | ||
| # include <sys/types.h> | ||
| # include <sys/event.h> | ||
| #endif | ||
|
|
||
| #include "nix/util/signals.hh" | ||
| #include "nix/util/file-descriptor.hh" | ||
|
|
There was a problem hiding this comment.
Missing header for std::array (compile break on macOS path).
runThread(…) uses std::array but isn’t included.
Apply:
#include <thread>
#include <cassert>
+#include <array>
#include <poll.h>
#include <errno.h>
@@
#include "nix/util/signals.hh"
+#include "nix/util/error.hh" // for SysError in inline definitions
#include "nix/util/file-descriptor.hh"📝 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.
| #ifdef __APPLE__ | |
| # include <sys/types.h> | |
| # include <sys/event.h> | |
| #endif | |
| #include "nix/util/signals.hh" | |
| #include "nix/util/file-descriptor.hh" | |
| #include <thread> | |
| #include <cassert> | |
| #include <array> | |
| #include <poll.h> | |
| #include <errno.h> | |
| #ifdef __APPLE__ | |
| # include <sys/types.h> | |
| # include <sys/event.h> | |
| #endif | |
| #include "nix/util/signals.hh" | |
| #include "nix/util/error.hh" // for SysError in inline definitions | |
| #include "nix/util/file-descriptor.hh" |
🤖 Prompt for AI Agents
In src/libutil/unix/include/nix/util/monitor-fd.hh around lines 10 to 17, the
code uses std::array in runThread(...) but never includes the <array> header
which breaks compilation on macOS; add a single line including <array> (e.g.,
#include <array>) alongside the other system/standard includes (place it before
the file's declarations or before any use of std::array) so the type is
available on all platforms.
| inline MonitorFdHup::MonitorFdHup(int fd) | ||
| { | ||
| notifyPipe.create(); | ||
| int notifyFd = notifyPipe.readSide.get(); | ||
| thread = std::thread([this, fd, notifyFd]() { this->runThread(fd, notifyFd); }); | ||
| }; |
There was a problem hiding this comment.
Unhandled exceptions in thread will call std::terminate.
runThread(…) throws on syscall errors; exceptions escaping a std::thread end function crash the process. Catch inside the thread.
- thread = std::thread([this, fd, notifyFd]() { this->runThread(fd, notifyFd); });
+ thread = std::thread([this, fd, notifyFd]() noexcept {
+ try {
+ this->runThread(fd, notifyFd);
+ } catch (...) {
+ // Cannot propagate from a background thread; exit quietly.
+ }
+ });📝 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.
| inline MonitorFdHup::MonitorFdHup(int fd) | |
| { | |
| notifyPipe.create(); | |
| int notifyFd = notifyPipe.readSide.get(); | |
| thread = std::thread([this, fd, notifyFd]() { this->runThread(fd, notifyFd); }); | |
| }; | |
| inline MonitorFdHup::MonitorFdHup(int fd) | |
| { | |
| notifyPipe.create(); | |
| int notifyFd = notifyPipe.readSide.get(); | |
| thread = std::thread([this, fd, notifyFd]() noexcept { | |
| try { | |
| this->runThread(fd, notifyFd); | |
| } catch (...) { | |
| // Cannot propagate from a background thread; exit quietly. | |
| } | |
| }); | |
| }; |
🤖 Prompt for AI Agents
In src/libutil/unix/include/nix/util/monitor-fd.hh around lines 128 to 133, the
thread lambda launches runThread(fd, notifyFd) without catching exceptions so
any exception escaping the thread will call std::terminate; modify the lambda to
wrap the call to this->runThread(fd, notifyFd) in a try/catch(...) block that
catches all exceptions, logs the error (or propagate an error condition) and
ensures resources are cleaned up (close notifyFd/pipe if needed and signal any
waiter) before returning from the thread so exceptions never escape the thread
function.
Motivation
Context
Summary by CodeRabbit