Sparse lock files WIP - #579
Open
edolstra wants to merge 26 commits into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe flake lock system now supports version 7 and version 8 through an abstract ChangesFlake locking architecture
Formatting workflow
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant FlakeCommand
participant lockFlake
participant LockedFlakeV7
participant LockedFlakeV8
participant callFlake
participant EvalState
FlakeCommand->>lockFlake: request locked flake
lockFlake->>LockedFlakeV7: parse or generate format 7
lockFlake->>LockedFlakeV8: parse or generate format 8
lockFlake->>callFlake: evaluate shared locked flake
callFlake->>EvalState: fetch input source
EvalState-->>callFlake: return source metadata
callFlake-->>FlakeCommand: return constructed flake result
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
edolstra
added
the
flake-regression-test
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (3)
src/libflake/include/nix/flake/settings.hh (1)
45-55: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument that version 8 is incomplete.
LockedFlakeV8throws forgetInputTargets,findInput, andgetSourcePath. A user who setslock-file-format = 8can therefore create a lock file that cannot be evaluated. Add a note to the setting description while version 8 support is in progress.📝 Proposed documentation change
R"( The lock file format version to use when creating a new lock file (7 or 8). An existing lock file keeps its version unless `--recreate-lock-file` is passed. + + Version 8 is experimental and incomplete: evaluation of + version 8 lock files is not supported yet. )",🤖 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 `@src/libflake/include/nix/flake/settings.hh` around lines 45 - 55, Update the lock-file-format setting description in Setting<unsigned int> to note that version 8 support is incomplete and may produce lock files that cannot be evaluated because LockedFlakeV8 lacks required operations; preserve the existing version and recreate behavior documentation.src/libflake/lockfile-v7.cc (1)
700-745: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClarify the reuse condition for the "update all" case.
lockFlags.inputUpdates == std::nulloptnow means "update all inputs". At Line 707 the guard!(lockFlags.inputUpdates && ...->count(...))evaluates to true in that case, which reads as "reuse the old lock". The behavior is still correct only because Line 887 passesnullptrasoldNodewheninputUpdatesis unset. Add a comment or test the flag directly, so a later change to Line 887 does not silently re-enable reuse.🤖 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 `@src/libflake/lockfile-v7.cc` around lines 700 - 745, Clarify the old-lock lookup guard around oldNode and get(oldNode->inputs, id) so lockFlags.inputUpdates == std::nullopt explicitly represents update-all and cannot permit reuse. Test the flag directly or add a focused comment documenting that reuse is allowed only when inputUpdates is present and the current input is not selected for update, preserving the existing behavior.src/libflake/call-flake.nix (1)
87-93: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the bare assertions with explicit errors.
assert builtins.isFunction flake.outputs;andassert !info.buildTime;produce assertion messages that do not name the input. A build-time input that is also marked as a flake therefore fails with an opaque message. Usethrowwith the input attribute path to help users diagnose the cause.🤖 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 `@src/libflake/call-flake.nix` around lines 87 - 93, In the flake branch of the result construction, replace the bare assertions on flake.outputs and info.buildTime with explicit throw errors that include the input attribute path. Preserve the existing validation conditions and result behavior for valid inputs, while ensuring both invalid cases identify the offending input.
🤖 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 `@CLAUDE.md`:
- Line 3: Update the formatter command guidance in CLAUDE.md to use executable
commands through the Nix development shell: document `nix develop -c
./maintainers/format.sh` generally and `nix develop -c ./maintainers/format.sh
clang-format` for C++-only changes, removing the extra `run` token.
In `@maintainers/format.sh`:
- Line 19: Update the retry loop around the pre-commit invocation in the format
script to record the worktree state before each run and retry only if the failed
run modifies files. When the run fails without changing the worktree, exit with
its failure status instead of looping indefinitely; preserve the existing
until-stable behavior for failures that do change files.
In `@src/libflake/call-flake.nix`:
- Around line 56-59: Update callFlake and the lock-file parsing flow around
parseLockFileV7 to validate follows relationships for cycles before evaluating
the LockedFlake. Apply the same validation to version 8 follows entries when
getInputTargets is implemented, and reject cyclic inputs before listFlakeInputs
or callFlake processes them.
In `@src/libflake/flake.cc`:
- Around line 911-928: Update LockedFlake::visit to track visited version 7 node
identities while traversing inputs, and skip callbacks and recursion for nodes
already visited. Deduplicate by the lock-file node identity rather than
lockedRef, preserving traversal of distinct nodes that share a reference while
preventing repeated processing of shared nodes across input paths.
In `@src/libflake/lockfile-v7.cc`:
- Around line 122-145: Validate parentInputAttrPath after the lockfile has been
fully parsed, ensuring every relative input has a parent and that following each
parent chain terminates without revisiting a node; reject missing,
self-referential, or cyclic parent references with a Nix Error before
getSourcePath or prim_fetchFlakeInput can recurse.
In `@src/libflake/lockfile-v8.cc`:
- Around line 257-296: Prevent users from selecting unsupported lock-file format
8 until LockedFlakeV8 supports evaluation: update the lockFileFormat setting
validation to reject version 8, and update
src/libflake/include/nix/flake/settings.hh lines 45-55 to state that version 8
is experimental and its evaluation is not yet supported. No direct change is
required in src/libflake/lockfile-v8.cc lines 257-296; its existing
notImplemented handlers document the unsupported operations.
- Around line 342-352: Update the version 8 override handling around the
input-override loop and its recursion in lockFlake so each path component is
validated against declared inputs before creating or traversing an override
entry. For nonexistent transitive inputs, skip locking and do not add the path
to overridesUsed, allowing the existing unmatched-override warning to be
emitted; only record and process overrides whose full path matches declared
inputs.
In `@src/nix/flake.cc`:
- Around line 268-276: Restore cycle protection for lock-graph traversal used by
LockedFlake::visit, ensuring cyclic version 7 graphs cannot recurse indefinitely
or create unbounded TreeNode instances. Confirm that lock parsing rejects cycles
before visit, or add a bounded depth/visited-node guard within visit itself so
both metadata and archive traversals remain protected.
- Around line 281-303: Guard access to child.second.input in the tree-printing
lambda before dereferencing it with std::get_if. Skip nodes whose optional input
is disengaged, while preserving the existing LockedFlake::InputInfo and
InputAttrPath printing behavior for populated entries.
---
Nitpick comments:
In `@src/libflake/call-flake.nix`:
- Around line 87-93: In the flake branch of the result construction, replace the
bare assertions on flake.outputs and info.buildTime with explicit throw errors
that include the input attribute path. Preserve the existing validation
conditions and result behavior for valid inputs, while ensuring both invalid
cases identify the offending input.
In `@src/libflake/include/nix/flake/settings.hh`:
- Around line 45-55: Update the lock-file-format setting description in
Setting<unsigned int> to note that version 8 support is incomplete and may
produce lock files that cannot be evaluated because LockedFlakeV8 lacks required
operations; preserve the existing version and recreate behavior documentation.
In `@src/libflake/lockfile-v7.cc`:
- Around line 700-745: Clarify the old-lock lookup guard around oldNode and
get(oldNode->inputs, id) so lockFlags.inputUpdates == std::nullopt explicitly
represents update-all and cannot permit reuse. Test the flag directly or add a
focused comment documenting that reuse is allowed only when inputUpdates is
present and the current input is not selected for update, preserving the
existing behavior.
🪄 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: 62667b9a-510e-4b83-9121-7ddd8e3806f1
📒 Files selected for processing (29)
CLAUDE.mdmaintainers/format.shsrc/libcmd/flake-schemas.ccsrc/libcmd/include/nix/cmd/command.hhsrc/libcmd/installable-flake.ccsrc/libcmd/installables.ccsrc/libcmd/repl.ccsrc/libflake-c/nix_api_flake.ccsrc/libflake/call-flake.nixsrc/libflake/diff.ccsrc/libflake/flake-impl.hhsrc/libflake/flake.ccsrc/libflake/include/nix/flake/flake.hhsrc/libflake/include/nix/flake/input-attr-path.hhsrc/libflake/include/nix/flake/lockfile.hhsrc/libflake/include/nix/flake/meson.buildsrc/libflake/include/nix/flake/settings.hhsrc/libflake/input-attr-path.ccsrc/libflake/lockfile-v7.ccsrc/libflake/lockfile-v8.ccsrc/libflake/lockfile.ccsrc/libflake/meson.buildsrc/nix/develop.ccsrc/nix/flake-command.hhsrc/nix/flake-diff-locks.ccsrc/nix/flake-diff-locks.mdsrc/nix/flake-prefetch-inputs.ccsrc/nix/flake.ccsrc/nix/meson.build
💤 Files with no reviewable changes (4)
- src/libcmd/repl.cc
- src/libcmd/include/nix/cmd/command.hh
- src/libflake/include/nix/flake/lockfile.hh
- src/libflake/lockfile.cc
| Observe the following rules when contributing to this repository: | ||
|
|
||
| * Before committing, run ./maintainers/format.sh to detect/fix any formatting issues. | ||
| * Before committing, run `./maintainers/format.sh` to detect/fix any formatting issues. If you've only touched C++ files, run `run ./maintainers/format.sh clang-format` since it's a lot faster. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- CLAUDE.md ---'
cat -n CLAUDE.md | sed -n '1,12p'
printf '%s\n' '--- maintainer files ---'
fd -t f 'format\.sh$|flake\.nix$|shell\.nix$|pre-commit' . | sort
printf '%s\n' '--- format.sh ---'
format_script="$(fd -t f '^format\.sh$' . | head -n 1)"
cat -n "$format_script"
printf '%s\n' '--- related references ---'
rg -n --hidden -g '!node_modules' '_NIX_PRE_COMMIT_HOOKS_CONFIG|nix develop|format\.sh|clang-format' .Repository: DeterminateSystems/nix-src
Length of output: 13601
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- CI invocation ---'
cat -n ci/gha/tests/pre-commit-checks | sed -n '1,28p'
printf '%s\n' '--- development-shell environment ---'
cat -n packaging/dev-shell.nix | sed -n '250,275p'
printf '%s\n' '--- nix develop command documentation ---'
cat -n src/nix/develop.md | sed -n '68,82p'
printf '%s\n' '--- command-token verifier ---'
python3 - <<'PY'
import shlex
commands = [
"./maintainers/format.sh",
"run ./maintainers/format.sh clang-format",
"nix develop -c ./maintainers/format.sh",
"nix develop -c ./maintainers/format.sh clang-format",
]
for command in commands:
print(f"{command!r} -> {shlex.split(command)!r}")
PYRepository: DeterminateSystems/nix-src
Length of output: 2831
Use executable formatter commands.
From a normal shell, document nix develop -c ./maintainers/format.sh and nix develop -c ./maintainers/format.sh clang-format. Remove the extra run token.
🤖 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 `@CLAUDE.md` at line 3, Update the formatter command guidance in CLAUDE.md to
use executable commands through the Nix development shell: document `nix develop
-c ./maintainers/format.sh` generally and `nix develop -c
./maintainers/format.sh clang-format` for C++-only changes, removing the extra
`run` token.
Source: Learnings
| hook="${1:-}" | ||
| fi | ||
|
|
||
| while ! pre-commit run --config "$_NIX_PRE_COMMIT_HOOKS_CONFIG" --all-files ${hook:+"$hook"}; do |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- maintainers/format.sh ---'
cat -n maintainers/format.sh
printf '%s\n' '--- related references ---'
rg -n --hidden --glob '!node_modules' --glob '!dist' --glob '!build' 'until-stable|pre-commit run|format\.sh' .Repository: DeterminateSystems/nix-src
Length of output: 2403
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
p = Path("maintainers/format.sh")
text = p.read_text()
print("has_until_stable=", "--until-stable" in text)
for i, line in enumerate(text.splitlines(), 1):
if 15 <= i <= 25:
print(f"{i}: {line}")
PYRepository: DeterminateSystems/nix-src
Length of output: 457
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
lines = Path("maintainers/format.sh").read_text().splitlines()
for n, line in enumerate(lines, 1):
if 1 <= n <= 40:
print(f"{n}: {line}")
PYRepository: DeterminateSystems/nix-src
Length of output: 923
Stop retrying persistent pre-commit failures forever.
With --until-stable, every non-zero pre-commit result triggers another run. An unchanged worktree or configuration error causes an infinite loop. Retry only when the failed run changes files; otherwise exit with the failure status.
🤖 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 `@maintainers/format.sh` at line 19, Update the retry loop around the
pre-commit invocation in the format script to record the worktree state before
each run and retry only if the failed run modifies files. When the run fails
without changing the worktree, exit with its failure status instead of looping
indefinitely; preserve the existing until-stable behavior for failures that do
change files.
Comment on lines
+122
to
+145
| LockedNode(const fetchers::Settings & fetchSettings, const nlohmann::json & json) | ||
| : lockedRef(getFlakeRef(fetchSettings, json, "locked", "info")) // FIXME: remove "info" | ||
| , originalRef(getFlakeRef(fetchSettings, json, "original", nullptr)) | ||
| , isFlake(json.find("flake") != json.end() ? (bool) json["flake"] : true) | ||
| , buildTime(json.find("buildTime") != json.end() ? (bool) json["buildTime"] : false) | ||
| , parentInputAttrPath( | ||
| json.find("parent") != json.end() ? (std::optional<InputAttrPath>) json["parent"] : std::nullopt) | ||
| { | ||
| if (!lockedRef.input.isLocked(fetchSettings) && !lockedRef.input.isRelative()) { | ||
| if (lockedRef.input.getNarHash()) | ||
| warn( | ||
| "Lock file entry '%s' is unlocked (e.g. lacks a Git revision) but is checked by NAR hash. " | ||
| "This is not reproducible and will break after garbage collection or when shared.", | ||
| lockedRef.to_string()); | ||
| else | ||
| throw Error( | ||
| "Lock file contains unlocked input '%s'. Use '--allow-dirty-locks' to accept this lock file.", | ||
| fetchers::attrsToJSON(lockedRef.input.toAttrs())); | ||
| } | ||
|
|
||
| // For backward compatibility, lock file entries are implicitly final. | ||
| assert(!lockedRef.input.attrs.contains("__final")); | ||
| lockedRef.input.attrs.insert_or_assign("__final", Explicit<bool>(true)); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Validate the parent field when parsing a node.
parentInputAttrPath is taken from the lock file without any check. Two downstream paths trust it:
LockedFlakeV7::getSourcePath(Line 445) callslockedNode->parentInputAttrPath.value()for a relative input. A lock file that omitsparentfor a relative input causesstd::bad_optional_access, which is not reported as a Nix error.- The same call recurses into the parent path.
prim_fetchFlakeInputinsrc/libflake/flake.cc(Lines 800-807) loops over the same field. A lock file whoseparentpoints at the node itself, or at another relative input that points back, causes unbounded recursion or an infinite loop.
A flake.lock comes from a fetched flake, so it is untrusted input. Reject a missing or self-referential parent during parsing.
🛡️ Suggested validation direction
// For backward compatibility, lock file entries are implicitly final.
assert(!lockedRef.input.attrs.contains("__final"));
lockedRef.input.attrs.insert_or_assign("__final", Explicit<bool>(true));
+
+ if (lockedRef.input.isRelative() && !parentInputAttrPath)
+ throw Error(
+ "lock file entry for relative input '%s' lacks a 'parent' attribute", lockedRef.to_string());Add a separate check after parsing the whole file that the parent chain of every relative input terminates.
🤖 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 `@src/libflake/lockfile-v7.cc` around lines 122 - 145, Validate
parentInputAttrPath after the lockfile has been fully parsed, ensuring every
relative input has a parent and that following each parent chain terminates
without revisiting a node; reject missing, self-referential, or cyclic parent
references with a Nix Error before getSourcePath or prim_fetchFlakeInput can
recurse.
Comment on lines
+342
to
+352
| for (auto & [path, ref] : lockFlags.inputOverrides) { | ||
| auto input = get(flake.inputs, path.get().front()); | ||
| if (!input) | ||
| continue; | ||
| for (auto & elem : std::views::drop(path.get(), 1)) | ||
| input = &input->overrides[elem]; | ||
| input->ref = ref; | ||
| input->follows = std::nullopt; | ||
| overridesUsed.insert(path); | ||
| explicitCliOverrides.insert(path); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Version 8 does not validate overrides against declared inputs.
Two problems follow from this block and from the override recursion at Lines 540-558:
input = &input->overrides[elem]creates the override entry for every path element after the first. ThereforeoverridesUsed.insert(path)marks the override as used even when the flake declares no such transitive input. The "does not match any input" warning inlockFlake()is then never printed for those paths.createLockis later called for that override (Line 554), so Nix fetches the flakeref and writes a lock entry for an input that noflake.nixdeclares. Version 7 instead warns about an override for a non-existent input and never locks it.
Validate each override path against the declared inputs of the containing flake before locking it, and only insert into overridesUsed when the path matches.
🤖 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 `@src/libflake/lockfile-v8.cc` around lines 342 - 352, Update the version 8
override handling around the input-override loop and its recursion in lockFlake
so each path component is validated against declared inputs before creating or
traversing an override entry. For nonexistent transitive inputs, skip locking
and do not add the path to overridesUsed, allowing the existing
unmatched-override warning to be emitted; only record and process overrides
whose full path matches declared inputs.
Comment on lines
+281
to
+303
| [&](this const auto & recurse, const TreeNode & node, const std::string & prefix) -> void { | ||
| for (const auto & [last, child] : markLast(node.children)) { | ||
| if (auto inputInfo = std::get_if<flake::LockedFlake::InputInfo>(&*child.second.input)) { | ||
| std::string lastModifiedStr = ""; | ||
| if (auto lastModified = (*lockedNode)->lockedRef.input.getLastModified()) | ||
| if (auto lastModified = inputInfo->lockedRef.input.getLastModified()) | ||
| lastModifiedStr = fmt(" (%s)", std::put_time(std::gmtime(&*lastModified), "%F %T")); | ||
| logger->cout( | ||
| "%s" ANSI_BOLD "%s" ANSI_NORMAL ": %s%s", | ||
| prefix + (last ? treeLast : treeConn), | ||
| input.first, | ||
| (*lockedNode)->lockedRef.to_string(true), | ||
| child.first, | ||
| inputInfo->lockedRef.to_string(true), | ||
| lastModifiedStr); | ||
|
|
||
| bool firstVisit = visited.insert(*lockedNode).second; | ||
|
|
||
| if (firstVisit) | ||
| recurse(**lockedNode, prefix + (last ? treeNull : treeLine)); | ||
| } else if (auto follows = std::get_if<1>(&input.second)) { | ||
| recurse(child.second, prefix + (last ? treeNull : treeLine)); | ||
| } else if (auto follows = std::get_if<flake::InputAttrPath>(&*child.second.input)) { | ||
| logger->cout( | ||
| "%s" ANSI_BOLD "%s" ANSI_NORMAL " follows input '%s'", | ||
| prefix + (last ? treeLast : treeConn), | ||
| input.first, | ||
| child.first, | ||
| flake::printInputAttrPath(*follows)); | ||
| } | ||
| } | ||
| }(*lockedFlake.lockFile.root, ""); | ||
| }(root, ""); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard the optional dereference before printing a node.
TreeNode::input stays disengaged for any node that the traversal creates as an intermediate but never reports through the callback. &*child.second.input then dereferences a disengaged std::optional, which is undefined behavior. The current visit order reports a parent before its children, so this path is not reachable today. The version 8 sparse model nests inputs that have no lock file of their own, so keep the print loop robust to a missing entry.
🛡️ Proposed guard
[&](this const auto & recurse, const TreeNode & node, const std::string & prefix) -> void {
for (const auto & [last, child] : markLast(node.children)) {
- if (auto inputInfo = std::get_if<flake::LockedFlake::InputInfo>(&*child.second.input)) {
+ if (!child.second.input) {
+ recurse(child.second, prefix + (last ? treeNull : treeLine));
+ continue;
+ }
+ if (auto inputInfo = std::get_if<flake::LockedFlake::InputInfo>(&*child.second.input)) {📝 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.
Suggested change
| [&](this const auto & recurse, const TreeNode & node, const std::string & prefix) -> void { | |
| for (const auto & [last, child] : markLast(node.children)) { | |
| if (auto inputInfo = std::get_if<flake::LockedFlake::InputInfo>(&*child.second.input)) { | |
| std::string lastModifiedStr = ""; | |
| if (auto lastModified = (*lockedNode)->lockedRef.input.getLastModified()) | |
| if (auto lastModified = inputInfo->lockedRef.input.getLastModified()) | |
| lastModifiedStr = fmt(" (%s)", std::put_time(std::gmtime(&*lastModified), "%F %T")); | |
| logger->cout( | |
| "%s" ANSI_BOLD "%s" ANSI_NORMAL ": %s%s", | |
| prefix + (last ? treeLast : treeConn), | |
| input.first, | |
| (*lockedNode)->lockedRef.to_string(true), | |
| child.first, | |
| inputInfo->lockedRef.to_string(true), | |
| lastModifiedStr); | |
| bool firstVisit = visited.insert(*lockedNode).second; | |
| if (firstVisit) | |
| recurse(**lockedNode, prefix + (last ? treeNull : treeLine)); | |
| } else if (auto follows = std::get_if<1>(&input.second)) { | |
| recurse(child.second, prefix + (last ? treeNull : treeLine)); | |
| } else if (auto follows = std::get_if<flake::InputAttrPath>(&*child.second.input)) { | |
| logger->cout( | |
| "%s" ANSI_BOLD "%s" ANSI_NORMAL " follows input '%s'", | |
| prefix + (last ? treeLast : treeConn), | |
| input.first, | |
| child.first, | |
| flake::printInputAttrPath(*follows)); | |
| } | |
| } | |
| }(*lockedFlake.lockFile.root, ""); | |
| }(root, ""); | |
| [&](this const auto & recurse, const TreeNode & node, const std::string & prefix) -> void { | |
| for (const auto & [last, child] : markLast(node.children)) { | |
| if (!child.second.input) { | |
| recurse(child.second, prefix + (last ? treeNull : treeLine)); | |
| continue; | |
| } | |
| if (auto inputInfo = std::get_if<flake::LockedFlake::InputInfo>(&*child.second.input)) { | |
| std::string lastModifiedStr = ""; | |
| if (auto lastModified = inputInfo->lockedRef.input.getLastModified()) | |
| lastModifiedStr = fmt(" (%s)", std::put_time(std::gmtime(&*lastModified), "%F %T")); | |
| logger->cout( | |
| "%s" ANSI_BOLD "%s" ANSI_NORMAL ": %s%s", | |
| prefix + (last ? treeLast : treeConn), | |
| child.first, | |
| inputInfo->lockedRef.to_string(true), | |
| lastModifiedStr); | |
| recurse(child.second, prefix + (last ? treeNull : treeLine)); | |
| } else if (auto follows = std::get_if<flake::InputAttrPath>(&*child.second.input)) { | |
| logger->cout( | |
| "%s" ANSI_BOLD "%s" ANSI_NORMAL " follows input '%s'", | |
| prefix + (last ? treeLast : treeConn), | |
| child.first, | |
| flake::printInputAttrPath(*follows)); | |
| } | |
| } | |
| }(root, ""); |
🤖 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 `@src/nix/flake.cc` around lines 281 - 303, Guard access to child.second.input
in the tree-printing lambda before dereferencing it with std::get_if. Skip nodes
whose optional input is disengaged, while preserving the existing
LockedFlake::InputInfo and InputAttrPath printing behavior for populated
entries.
In preparation for the version 8 sparse lock file format, rename LockFile to LockFileV7 and move it, together with the Node and LockedNode data types, to a new header lockfile-v7.hh (implementation in lockfile-v7.cc). lockfile.hh continues to contain the common types like InputAttrPath and NonEmptyInputAttrPath. No behaviour change. Assisted-by: Claude Fable 5 <noreply@anthropic.com>
In preparation for the version 8 sparse lock file format, turn `LockedFlake` into an abstract base class that doesn't expose the version 7 lock file representation, and move that representation into a new subclass `LockedFlakeV7`. The `lockFlake()` functions now return a `std::unique_ptr<LockedFlake>`. Code that needs to query a locked flake now goes through virtual methods instead of inspecting the version 7 node graph directly: * `isUnlocked()`: check for unlocked or non-final inputs. This allows `getFingerprint()` to be implemented entirely in terms of the abstract interface. * `toJSON()`, with non-virtual `to_string()` and `operator <<` on top of it. * `getInputNames()`: return the names of the inputs of the input denoted by an attribute path prefix. * `findInput()`: look up an input by attribute path, resolving "follows" indirections, and return an `InputInfo` struct containing its locked reference. Used by `--inputs-from` and `InstallableFlake::nixpkgsFlakeRef()`. * `visit()`: walk all transitive inputs in depth-first order, calling a callback with either an `InputInfo` or the target path of a "follows" input. The callback controls recursion, e.g. to skip build-time inputs. Used by `nix flake metadata`, `nix flake archive` and `nix flake prefetch-inputs`. The only remaining uses of `LockedFlakeV7` are in `lockFlake()` and `callFlake()` in libflake itself, which will become the dispatch points between the version 7 and version 8 implementations. Assisted-by: Claude Fable 5 <noreply@anthropic.com>
The `computeLocks` machinery is moved from the free function `lockFlake()` in flake.cc into a new static member function `LockedFlakeV7::lockFlake()` in lockfile-v7.cc. The free function now only contains version-independent work, expressed in terms of the abstract `LockedFlake` interface: * Reading the old lock file into a `nlohmann::json`, and constructing a `LockedFlake` from it (via a new `LockedFlakeV7` constructor that takes the JSON). This is where we will dispatch on the lock file version in the future. * Change detection, by comparing the virtual `toJSON()` serializations of the old and new locked flakes. This also does the right thing across future version migrations (a version change is always a change). * Printing the diff, via a new abstract `LockedFlake::diff()` method that shows the differences relative to an older `LockedFlake`. The version 7 implementation diffs against an empty lock file if the old lock file is not version 7, so all inputs show up as added. * Writing/committing the new lock file, using `to_string()` and the virtual `isUnlocked()`. `LockedFlakeV7::lockFlake()` takes the old lock file as a `const LockedFlake &` and downcasts it internally. The `LockFileV7` parsing constructor now takes a `nlohmann::json` instead of the file contents, and `getFlake()` with a lock root attribute path is exported from flake.cc since the lock algorithm needs it from its new home. Assisted-by: Claude Fable 5 <noreply@anthropic.com>
This abstract method returns the source path of the input denoted by an input attribute path (or of the top-level flake if the path is empty), fetching the input if necessary. The returned path is backed by `EvalState::rootFS`, i.e. it's a store path - possibly a virtual one with the input's accessor mounted on it if lazy trees are enabled. `nix flake prefetch-inputs` now uses this method instead of accessing the version 7 node graph directly, so when lazy trees are disabled, inputs are copied to the store via `EvalState::mountInput()` (which also verifies their NAR hashes). In `LockedFlakeV7`, the `nodePaths` map is replaced by a `mutable Sync<std::optional<SourcePath>>` field in `LockedNode`, i.e. the source path of a fetched node is now stored in the node itself. It is set by `LockedFlakeV7::lockFlake()` for nodes fetched during locking, and cached by `getSourcePath()` for nodes fetched on demand (where relative path inputs are resolved against the source path of their parent flake, and the `subdir` of the locked flakeref is taken into account). `callFlake()` reconstructs the node -> source path map by walking the node graph. Assisted-by: Claude Fable 5 <noreply@anthropic.com>
Instead of the version 7 lock file JSON and an attrset of pre-fetched
source trees ("overrides"), call-flake.nix now receives an external
value wrapping the C++ `LockedFlake` object, along with two internal
primops (registered with `internal = true`, so they're not exposed to
the user):
* `listFlakeInputs lockedFlake inputAttrPath`: returns the inputs of
the input denoted by `inputAttrPath` as an attrset mapping input
names to either null (for a regular input) or the input attribute
path of the target of a "follows" input. This only consults the
lock data, so it doesn't fetch anything.
* `fetchFlakeInput lockedFlake inputAttrPath`: fetches an input (via
`LockedFlake::getSourcePath()`) and returns its `sourceInfo`
attributes and flake subdirectory; or, for build-time inputs, the
locked input attributes (without fetching).
As a result, fetching, override handling, relative path handling and
the lock file node graph all disappear from call-flake.nix - it just
lazily constructs a tree of inputs keyed by input attribute path,
resolving "follows" by walking the edges of that tree from the
top-level flake. This preserves evaluation sharing (every distinct
input is constructed only once) and prepares for the version 8 lock
file format, whose `LockedFlake` implementation will only need to
provide the same abstract interface.
Also:
* `callFlake()` now takes a `std::shared_ptr<const LockedFlake>`
since the thunks created by call-flake.nix reference the
`LockedFlake` object, which therefore must be kept alive for the
lifetime of the evaluator.
* `LockedFlake::getInputTargets()` is a new abstract method backing
`listFlakeInputs`; `getInputNames()` is now a non-virtual wrapper
around it.
* `LockedFlakeV7::findInput()` now returns the `buildTime` and
`parentInputAttrPath` fields (the former was dropped before, but
wasn't used).
* The `KeyMap` returned by `LockFileV7::toJSON()` is no longer
needed, since lock file node keys were only used by callFlake().
Assisted-by: Claude Fable 5 <noreply@anthropic.com>
These types are no longer used outside of lockfile-v7.cc, so they can
be implementation details. The header now only exposes two free
functions returning `std::unique_ptr<LockedFlake>`:
* `parseLockFileV7()`: construct a `LockedFlake` from the JSON
contents of a version 5-7 lock file.
* `lockFlakeV7()` (previously `LockedFlakeV7::lockFlake()`): compute a
new lock file.
This also keeps the version dispatch in the free function `lockFlake()`
symmetrical for the future version 8 implementation: it will just pick
between `parseLockFileV{7,8}()` and `lockFlakeV{7,8}()` based on the
`version` field in the lock file JSON.
Also remove the unused `LockedNode::computeStorePath()`.
Assisted-by: Claude Fable 5 <noreply@anthropic.com>
Now that these classes are local to this file, there is no need for separate declarations and definitions. Assisted-by: Claude Fable 5 <noreply@anthropic.com>
It's the input attribute path, relative to the top-level flake, of the flake that *declares* the relative path (i.e. the flake that declares the override, in the case of overridden inputs), which is not necessarily the input's parent in the lock file graph. Assisted-by: Claude Fable 5 <noreply@anthropic.com>
`std::nullopt` now means "update all inputs". Bare `nix flake update` uses this instead of setting `recreateLockFile`, so recreating the lock file is now only triggered by the explicit `--recreate-lock-file` flag (which is hereby un-deprecated). This distinction matters for the upcoming lock file version dispatch: `nix flake update` will keep the lock file's existing format, while `--recreate-lock-file` will switch it to the configured format. Note that "update all inputs" is implemented by ignoring the *top-level* old lock file (as `recreateLockFile` did), not by refusing reuse at every path: entries seeded from a dependency's own lock file (e.g. a vendored subflake) must still be copied from it. Assisted-by: Claude Fable 5 <noreply@anthropic.com>
Selects the lock file format version (currently 7, the default, or 8) used when creating a new lock file. An existing lock file keeps its version unless `--recreate-lock-file` is passed. Settable via nix.conf, `--option lock-file-format N` or the auto-generated `--lock-file-format` flag. Assisted-by: Claude Fable 5 <noreply@anthropic.com>
lockFlakeV7() now returns a LockFlakeResult struct containing the locked flake and the overridesUsed/updatesUsed sets, so the warnings about unmatched '--override-input' / update flags can be emitted by lockFlake(). This allows the upcoming lockFlakeV8() to share that code. Assisted-by: Claude Fable 5 <noreply@anthropic.com>
…ockedFlakes() The new free function diffLockedFlakes() (in diff.cc) can diff two LockedFlakes of any version. It relies on two new LockedFlake virtual methods: version(), used to report lock file version changes (which previously happened in lockFlake()), and getAllLockEntries(), which flattens the contents of a lock file into a map from input attribute paths to locked flakerefs or 'follows' targets. A nice side effect is that migrating an unchanged lock file between versions 7 and 8 now shows just the version change, rather than every input as "added". getAllLockEntries() takes a `fetchTransitive` flag (currently unimplemented for version 8) that will allow a future `nix flake diff-locks` command to include the transitive locks of inputs that have a lock file of their own. Assisted-by: Claude Fable 5 <noreply@anthropic.com>
Shows the differences between the lock files of two flakes, e.g.
# nix flake diff-locks github:NixOS/nix/2.28.0 github:NixOS/nix/2.29.0
• Updated input 'nixpkgs':
'github:NixOS/nixpkgs/48d12d5' (2024-12-16)
→ 'github:NixOS/nixpkgs/adaa24f' (2025-05-13)
The second argument defaults to the flake in the current directory.
The flakes do not need to use the same lock file format version. The
--transitive flag (not yet supported for version 8 lock files)
includes the transitive locks of inputs that have a lock file of their
own.
Also factors the version dispatch in lockFlake() into a new generic
parseLockFile() function that the command uses to read lock files of
any version.
Assisted-by: Claude Fable 5 <noreply@anthropic.com>
This function will be identitical between lockFlakeV7() and lockFlakeV8(), so move it into a utility function in the new internal header flake-impl.hh (it's not part of the public libflake API). Assisted-by: Claude Fable 5 <noreply@anthropic.com>
It can be implemented generically on top of getInputTargets() (which provides the input names and 'follows' targets of each level) and findInput() (which provides the InputInfo of non-follows inputs), so the per-version implementations are unnecessary. Note that we now only recurse into inputs with `isFlake = true`; in a well-formed version 7 lock file, non-flake nodes never have children anyway. Assisted-by: Claude Fable 5 <noreply@anthropic.com>
getInputTargets(), findInput() and getSourcePath() now require an input attribute path that doesn't pass through any 'follows' input, and throw an error otherwise. The new non-virtual resolveFollows() method resolves a path into that form, implemented generically on top of getInputTargets() (which returns the immediate 'follows' targets). This simplifies the per-version implementations: LockFileV7's node lookup no longer resolves 'follows' edges (and the check that 'follows' targets exist, previously the other user of that resolution, now uses resolveFollows() in lockFlakeV7()). The only external callers that need resolveFollows() are '--inputs-from' and InstallableFlake::nixpkgsFlakeRef(), where the named input may be a 'follows'; call-flake.nix and visit() only ever recurse into resolved paths. Assisted-by: Claude Fable 5 <noreply@anthropic.com>
getInputTargets(), findInput(), visit() and the non-virtual getInputNames() now take an EvalState, like getSourcePath() already did. The version 7 implementations don't need it (their lock files contain the full dependency graph), but the version 8 implementations will have to fetch inputs and read their flake.nix / flake.lock files on demand. Assisted-by: Claude Fable 5 <noreply@anthropic.com>
GCC segfaults while parsing the recursive lambda with an explicit
object parameter ("deducing this") when it contains a call to a
member function of the enclosing class. Use the std::function
recursion pattern instead.
Assisted-by: Claude Fable 5 <noreply@anthropic.com>
Add lockfile-v8.cc containing the in-memory representation of the new sparse lock file format: a flat map from slash-separated input attribute paths to locks (with recursive inline `locks` for inputs that lack a lock file of their own), along with JSON parsing, serialization, isUnlocked() and diff(). The eval-time methods (getInputTargets() etc.) and the lock algorithm are stubs for now. Assisted-by: Claude Fable 5 <noreply@anthropic.com>
lockFlakeV8() locks the immediate inputs of the flake, plus any overrides of transitive inputs declared in its flake.nix (stored as slash-separated paths like "foo/nixpkgs"), reusing entries from the old lock file when the flakeref is unchanged. 'follows' and relative path inputs are not stored; the latter are required to have a lock file of their own. Inputs that don't have a lock file get their transitive inputs locked inline in a recursive `locks` attribute, which is pruned when the input gains a lock file on a later update. Command line overrides are applied by merging them into the top-level flake's input tree, so they're handled uniformly with overrides declared in flake.nix (with the outermost override winning). Assisted-by: Claude Fable 5 <noreply@anthropic.com>
The old lock file is parsed with the parser matching its version. The version of the new lock file is that of the existing lock file; only `--recreate-lock-file` (or the absence of a lock file) switches to the version configured by the `lock-file-format` setting. Version changes are shown in the lock file diff. lockFlakeV7() now ignores a non-v7 old lock file instead of throwing `std::bad_cast`, so migrating from version 8 back to 7 works. Assisted-by: Claude Fable 5 <noreply@anthropic.com>
Say "takes precedence over" instead of "shadows", and refer to the entries in the recursive `locks` attribute as "nested" entries rather than "inline" ones. Assisted-by: Claude Fable 5 <noreply@anthropic.com>
LockedFlakeV8 now implements getInputTargets(), findInput() and getSourcePath(), making `nix build`/`nix eval` etc. work on version 8 lock files. Since a sparse lock file only contains the locks of the immediate inputs of a flake (plus overrides and the nested `locks` of inputs that don't have a lock file of their own), the resolution of transitive inputs is *delegated*: for each input, a child `LockedFlake` is constructed lazily by fetching the input and reading its own lock file (which may use any lock file version - so the entire dependency graph of a version 7 lock file is honored for its subtree), or from the nested `locks` of its entry. Queries below an input are forwarded to its child, translating input attribute paths and 'follows' targets between the child's namespace and ours. Override entries in our lock file (slash-separated keys like "foo/nixpkgs") take precedence over the delegated resolution, and relative path inputs are resolved against our own source tree and delegated via their own lock file. Per the LockedFlake contract, query paths must be fully resolved: an error is thrown when the descent encounters an input declared as a 'follows'. Note that unlike version 7, which stores 'follows' in the lock file, we have to check the flake.nix declarations for this - without it, a path crossing one of our 'follows' overrides would be delegated to a child that doesn't know about the override. Note that a missing entry in a transitive lock file is a fatal evaluation error: unlike version 7, version 8 does not silently repair incomplete transitive lock files. (The top-level lock file is still completed by lockFlake() before evaluation, as before.) Assisted-by: Claude Fable 5 <noreply@anthropic.com>
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/libflake/call-flake.nix (1)
96-103: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a clear error for a dangling follows target.
getInputByAttrPathindexesinput.edges.${name}without a membership check. If a lock file names a follows target that does not exist, evaluation fails with a generic missing-attribute error. Lock parsing is expected to reject such targets, so this only affects the error message quality.🤖 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 `@src/libflake/call-flake.nix` around lines 96 - 103, Update getInputByAttrPath to validate that each name exists in input.edges before indexing it, and raise a clear error identifying the dangling follows target when it is absent. Preserve the existing traversal and final-input behavior for valid attribute paths.
🤖 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.
Nitpick comments:
In `@src/libflake/call-flake.nix`:
- Around line 96-103: Update getInputByAttrPath to validate that each name
exists in input.edges before indexing it, and raise a clear error identifying
the dangling follows target when it is absent. Preserve the existing traversal
and final-input behavior for valid attribute paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d7872156-fc45-42b7-a53b-07774c2dbec2
📒 Files selected for processing (29)
CLAUDE.mdmaintainers/format.shsrc/libcmd/flake-schemas.ccsrc/libcmd/include/nix/cmd/command.hhsrc/libcmd/installable-flake.ccsrc/libcmd/installables.ccsrc/libcmd/repl.ccsrc/libflake-c/nix_api_flake.ccsrc/libflake/call-flake.nixsrc/libflake/diff.ccsrc/libflake/flake-impl.hhsrc/libflake/flake.ccsrc/libflake/include/nix/flake/flake.hhsrc/libflake/include/nix/flake/input-attr-path.hhsrc/libflake/include/nix/flake/lockfile.hhsrc/libflake/include/nix/flake/meson.buildsrc/libflake/include/nix/flake/settings.hhsrc/libflake/input-attr-path.ccsrc/libflake/lockfile-v7.ccsrc/libflake/lockfile-v8.ccsrc/libflake/lockfile.ccsrc/libflake/meson.buildsrc/nix/develop.ccsrc/nix/flake-command.hhsrc/nix/flake-diff-locks.ccsrc/nix/flake-diff-locks.mdsrc/nix/flake-prefetch-inputs.ccsrc/nix/flake.ccsrc/nix/meson.build
💤 Files with no reviewable changes (4)
- src/libflake/lockfile.cc
- src/libcmd/include/nix/cmd/command.hh
- src/libcmd/repl.cc
- src/libflake/include/nix/flake/lockfile.hh
🚧 Files skipped from review as they are similar to previous changes (22)
- src/libflake-c/nix_api_flake.cc
- src/libcmd/installables.cc
- src/libflake/include/nix/flake/input-attr-path.hh
- src/nix/flake-diff-locks.cc
- src/nix/meson.build
- CLAUDE.md
- src/nix/develop.cc
- src/libcmd/installable-flake.cc
- src/libflake/include/nix/flake/meson.build
- src/libflake/include/nix/flake/settings.hh
- maintainers/format.sh
- src/libflake/input-attr-path.cc
- src/nix/flake-command.hh
- src/libflake/lockfile-v7.cc
- src/libflake/meson.build
- src/nix/flake-prefetch-inputs.cc
- src/libflake/flake-impl.hh
- src/libflake/lockfile-v8.cc
- src/libcmd/flake-schemas.cc
- src/nix/flake.cc
- src/libflake/include/nix/flake/flake.hh
- src/libflake/flake.cc
Reading or creating version 8 (sparse) lock files now requires the `lock-file-v8` experimental feature. The checks are in parseLockFileV8() and lockFlakeV8(), so they also cover transitive version 8 lock files encountered during evaluation and commands like `nix flake diff-locks`. Version 7 lock files are unaffected. Assisted-by: Claude Fable 5 <noreply@anthropic.com>
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.
Motivation
Context
Summary by CodeRabbit
New Features
nix flake diff-locksto compare lock-file changes, including optional transitive dependencies.Improvements
Documentation
nix flake diff-locks.