Flake schemas #217
Flake schemas #217
Conversation
WalkthroughAdds a flake schema subsystem: documentation and Nix schema definitions, a C++ flake-schemas API and EvalCache wiring, eval-cache API refactor to use AttrPath, command and InstallableFlake changes to use role-based defaults, and test updates to match schema-driven flake show/check behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Cmd
participant flake_schemas
participant EvalCache
participant Flake
User->>Cmd: run (e.g., nix flake show/check)
Cmd->>flake_schemas: call(state, lockedFlake, defaultSchemasFlake)
flake_schemas->>Flake: mount/read builtin/default schema flake
Flake-->>flake_schemas: LockedFlake
flake_schemas->>EvalCache: evaluate schema extraction
EvalCache-->>flake_schemas: inventory (AttrCursor root)
flake_schemas-->>Cmd: inventory/ref<EvalCache>
Cmd->>flake_schemas: forEachOutput(inventory, cb)
loop per output
flake_schemas->>flake_schemas: visit(node)
alt leaf
flake_schemas->>EvalCache: evalChecks / derivation
EvalCache-->>flake_schemas: result / drv path
else non-leaf
flake_schemas->>flake_schemas: recurse children
end
flake_schemas-->>Cmd: callback with node/leaf info
end
Cmd->>User: render (tree/JSON) and build derivations as needed
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/libexpr/include/nix/expr/eval-cache.hh (1)
8-9: Missing<vector>include forAttrPath.Prevents brittle transitive include reliance.
#include <functional> #include <variant> +#include <vector>
🧹 Nitpick comments (20)
tests/functional/completions.sh (1)
72-72: Don’t hide a completion regression; make the assertion resilient or gate it.Commenting out this check can mask breakage. Either:
- Restore a looser assertion that only checks for presence of “./foo#sampleOutput” regardless of the category line, or
- Guard the test behind a capability check and leave a clear FIXME.
Example making the check robust to category changes:
-#[[ "$(NIX_GET_COMPLETIONS=2 nix eval ./foo\#sam)" == $'attrs\n./foo#sampleOutput\t' ]] +NIX_GET_COMPLETIONS=2 nix eval ./foo#sa | tail -n +2 | grep -F -- './foo#sampleOutput'Please confirm if schema-driven completions changed categories; if so, add a TODO comment with a tracking issue.
tests/functional/chroot-store.sh (1)
49-51: Avoid blanket skip with “if false”; restore conditional and document why skip is needed.Replacing the sandbox guard with a hard skip drops coverage and can hide regressions. Revert to the original capability check (or a named env toggle), and keep the FIXME if instability persists.
-if false; then # FIXME -#if canUseSandbox; then +if canUseSandbox; then # FIXME: re-evaluate once chroot path is stabilizedIf there’s a reproducible flake-schemas interaction here, reference it in the FIXME so we can track re-enabling.
src/libcmd/include/nix/cmd/installable-flake.hh (1)
66-70: Update docstring to match schema/fragment model.Comment still references getActualAttrPaths(); replace with wording around schema-driven fragments and cursor retrieval from the eval cache.
src/libcmd/builtin-flake-schemas.nix (2)
7-8: Remove dead bindingmapAttrsToList.Defined but unused; drop to keep the module clean.
- mapAttrsToList = f: attrs: map (name: f name attrs.${name}) (builtins.attrNames attrs);
248-263: Harden overlay validation to avoid spurious throws.Some overlays force attributes even during shape checks; wrap in
lib.tryso schema inventory doesn’t abort on non-critical overlays.- evalChecks.isOverlay = - # FIXME: should try to apply the overlay to an actual - # Nixpkgs. But we don't have access to a nixpkgs - # flake here. Maybe this schema should be moved to the - # nixpkgs flake, where it does have access. - if !builtins.isFunction overlay then - throw "overlay is not a function, but a set instead" - else - builtins.isAttrs (overlay { } { }); + evalChecks.isOverlay = + # FIXME: ideally validate against real nixpkgs. + if !builtins.isFunction overlay then + false + else + self.lib.try (builtins.isAttrs (overlay { } { })) false;tests/functional/formatter.sh (1)
87-89: Improve test portability and robustness.
- Replace
grep -P(PCRE) withgrep -Efor broader availability.- Prefer
clearStoreIfPossibleto avoid failures on environments without permissions.-clearStore -nix flake show | grep -P "package.*\[formatter\]" +clearStoreIfPossible +nix flake show | grep -E "package.*\[formatter\]"doc/manual/source/protocols/flake-schemas.md (1)
1-64: Excellent documentation with one minor formatting issue.The new Flake Schemas documentation is comprehensive and well-structured. It clearly explains:
- The purpose and mechanism of flake schemas
- Schema structure and attributes
- Inventory node types and their attributes
- A practical example for validation
Apply this diff to fix the bare URL on line 7:
-A flake can define schemas for its outputs by defining a `schemas` output. `schemas` should be an attribute set with an attribute for -every output type that you want to be supported. If a flake does not have a `schemas` attribute, Nix uses a built-in set of schemas (namely https://github.com/DeterminateSystems/flake-schemas). +A flake can define schemas for its outputs by defining a `schemas` output. `schemas` should be an attribute set with an attribute for +every output type that you want to be supported. If a flake does not have a `schemas` attribute, Nix uses a built-in set of schemas (namely [https://github.com/DeterminateSystems/flake-schemas](https://github.com/DeterminateSystems/flake-schemas)).Based on static analysis hints.
src/libflake/include/nix/flake/flake.hh (2)
228-239: API polish: document parameters and mark results nodiscard.Recommend tightening the API:
- Document lockedRef vs resolvedRef vs originalRef and lockRootPath explicitly.
- Mark results [[nodiscard]] to discourage accidental discards.
-/** - * Return a `Flake` object representing the flake read from the - * `flake.nix` file in `rootDir`. - */ -Flake readFlake( +/** + * Return a `Flake` read from `flake.nix` in `rootDir`. + * - originalRef: user-specified ref + * - resolvedRef: registry-resolved ref + * - lockedRef: fetched ref (may include commit hash) + * - lockRootPath: input path in the lock graph that this flake anchors to + */ +[[nodiscard]] Flake readFlake( EvalState & state, const FlakeRef & originalRef, const FlakeRef & resolvedRef, const FlakeRef & lockedRef, const SourcePath & rootDir, const InputAttrPath & lockRootPath);
247-249: Avoid an unnecessary copy of Flake (or clarify move intent).Passing Flake by value can copy a large structure. Prefer const Flake& if not modifying, or Flake&& if you intend to move.
-LockedFlake lockFlake( - const Settings & settings, EvalState & state, const FlakeRef & topRef, const LockFlags & lockFlags, Flake flake); +LockedFlake lockFlake( + const Settings & settings, EvalState & state, const FlakeRef & topRef, const LockFlags & lockFlags, const Flake & flake);If move semantics are intended:
- ..., Flake flake); + ..., Flake&& flake);src/nix/profile.cc (1)
730-735: Optional defensive check: consider guarding against leading "." to prevent edge cases.The verification confirms that:
element.source->attrPathis stored without a leading "." (sourced from manifest JSON and flake info, displayed as-is in logs and UI)- Empty roles behavior is correct and intentional—the loop over roles simply doesn't execute, yielding an empty attrPaths list, which is valid for commands not using flake schemas
The suggested defensive check
!element.source->attrPath.empty() && element.source->attrPath[0] == '.'is optional robustness: while no current code path creates a leading ".", guarding against it would future-proof the absolute lookup against accidental malformed attrPath values.- "." + element.source->attrPath, // absolute lookup + ( !element.source->attrPath.empty() && element.source->attrPath[0] == '.' + ? element.source->attrPath + : "." + element.source->attrPath ), // absolute lookup (defensive) element.source->outputs, StringSet{}, lockFlags, getDefaultFlakeSchemas());src/libcmd/include/nix/cmd/command.hh (2)
135-142: Make MixFlakeSchemas methods const and ensure FlakeRef is visible in this header
- getDefaultFlakeSchemas() and the data member access can be const; no mutation implied.
- Ensure FlakeRef is declared/visible here (include flake.hh or add a forward decl) to avoid ODR/visibility surprises if lockfile.hh stops re-exporting it.
Proposed header tweaks:
-struct MixFlakeSchemas : virtual Args, virtual StoreCommand +struct MixFlakeSchemas : virtual Args, virtual StoreCommand { std::optional<std::string> defaultFlakeSchemas; MixFlakeSchemas(); - std::optional<FlakeRef> getDefaultFlakeSchemas(); + std::optional<FlakeRef> getDefaultFlakeSchemas() const; };If FlakeRef isn’t guaranteed via current includes, add:
+#include "nix/flake/flake.hh"or forward-declare in the correct namespace.
144-144: Prefer const on getRoles()The roles a command implements are immutable per command type. Mark getRoles() const in the base to encourage consistent overrides.
- virtual StringSet getRoles(); + virtual StringSet getRoles() const;src/libflake/flake.cc (1)
207-214: Clarify readFlake linkagereadFlake is no longer static. If it’s meant to be TU-internal, keep it static; if public, add the declaration to the appropriate header to avoid accidental ODR exposure.
src/libcmd/installables.cc (2)
236-239: Make getRoles() const and document overridesThe default {"nix-build"} is fine, but mark it const and document that subcommands (run, develop, repl, etc.) should override to avoid mis-scoping defaults.
-StringSet SourceExprCommand::getRoles() +StringSet SourceExprCommand::getRoles() const { return {"nix-build"}; }Also update the declaration in command.hh accordingly.
428-459: openEvalCache helper duplicationThis helper overlaps with flake_schemas::call’s cache setup. Consider consolidating to a single code path to avoid divergence, or clearly mark this as legacy path and add a TODO with the intended removal milestone.
src/nix/flake.cc (3)
418-429: Tighten lambda capture for readabilityInitializer capture
output(ref(output))shadows the outer parameter. Works, but is non-obvious. Prefer an explicit local or different capture name to avoid confusion.- if (output) - futures.spawn(1, [&visit, output(ref(output))]() { visit(output); }); + if (output) { + auto outRef = ref(output); + futures.spawn(1, [&visit, outRef]() { visit(outRef); }); + }
432-434: Unchecked outputs warning: great, add hint for schemasMessage is helpful. Optionally mention that missing schemas in the flake or defaults could cause “unknown” outputs.
850-869: Parallel JSON rendering is fine; guard unknown/skipped renderingCurrently, the tree renderer always calls render(j["output"]) even when “unknown” or “skipped”. Consider skipping render() in those cases to avoid creating empty nodes and to annotate “skipped” explicitly.
- if (!showLegacy && state->symbols[outputName] == "legacyPackages") { - j.emplace("skipped", true); - } else if (output) { + if (!showLegacy && state->symbols[outputName] == "legacyPackages") { + j.emplace("skipped", true); + } else if (output) { ... } else j.emplace("unknown", true);And in the tree output loop:
- render(child.value()["output"], ...); + if (child.value().contains("output")) + render(child.value()["output"], ...); + else if (child.value().contains("skipped")) + logger->cout(ANSI_GREEN "%s%s" ANSI_NORMAL ANSI_ITALIC "(skipped)" ANSI_NORMAL, nextPrefix, treeLast); + else if (child.value().contains("unknown")) + logger->cout(ANSI_GREEN "%s%s" ANSI_NORMAL ANSI_ITALIC "(unknown flake output)" ANSI_NORMAL, nextPrefix, treeLast);src/libcmd/installable-flake.cc (1)
147-216: Schema/role-based resolution logic: good, but add small hardening
- Flow correctly prioritizes schemas and falls back to outputs traversal and repl-compat hack.
- Suggest caching parsedFragment emptiness before loops to avoid repeated checks, and dedup attrPaths to reduce duplicate probes when multiple roles overlap.
- std::vector<eval_cache::AttrPath> attrPaths; + std::vector<eval_cache::AttrPath> attrPaths; + auto pushUnique = [&](eval_cache::AttrPath &&p) { + if (std::find(attrPaths.begin(), attrPaths.end(), p) == attrPaths.end()) + attrPaths.push_back(std::move(p)); + }; @@ - attrPaths.push_back(parseAttrPath(state, fragment.substr(1))); + pushUnique(parseAttrPath(state, fragment.substr(1))); @@ - attrPaths.push_back(attrPath2); + pushUnique(std::move(attrPath2)); @@ - attrPaths.push_back(attrPath2); + pushUnique(std::move(attrPath2)); @@ - attrPaths.push_back(parsedFragment); + pushUnique(parsedFragment); @@ - attrPaths.push_back({}); + pushUnique({});Also applies to: 221-238, 240-242
src/libcmd/include/nix/cmd/flake-schemas.hh (1)
18-26: Preferusingovertypedeffor clarityStyle nit: use
using ForEachChild = std::function<...>;.-typedef std::function<void(Symbol attrName, ref<AttrCursor> attr, bool isLast)> ForEachChild; +using ForEachChild = std::function<void(Symbol attrName, ref<AttrCursor> attr, bool isLast)>;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
doc/manual/source/SUMMARY.md.in(1 hunks)doc/manual/source/protocols/flake-schemas.md(1 hunks)src/libcmd/builtin-flake-schemas.nix(1 hunks)src/libcmd/call-flake-schemas.nix(1 hunks)src/libcmd/flake-schemas.cc(1 hunks)src/libcmd/include/nix/cmd/command.hh(3 hunks)src/libcmd/include/nix/cmd/flake-schemas.hh(1 hunks)src/libcmd/include/nix/cmd/installable-flake.hh(2 hunks)src/libcmd/include/nix/cmd/meson.build(1 hunks)src/libcmd/installable-flake.cc(5 hunks)src/libcmd/installables.cc(7 hunks)src/libcmd/meson.build(2 hunks)src/libcmd/package.nix(1 hunks)src/libexpr/eval-cache.cc(8 hunks)src/libexpr/include/nix/expr/eval-cache.hh(6 hunks)src/libflake/flake.cc(3 hunks)src/libflake/include/nix/flake/flake.hh(1 hunks)src/nix/bundle.cc(2 hunks)src/nix/develop.cc(2 hunks)src/nix/flake-check.md(1 hunks)src/nix/flake.cc(8 hunks)src/nix/formatter.cc(1 hunks)src/nix/profile.cc(1 hunks)src/nix/repl.cc(1 hunks)src/nix/run.cc(1 hunks)src/nix/search.cc(1 hunks)tests/functional/chroot-store.sh(1 hunks)tests/functional/completions.sh(1 hunks)tests/functional/flakes/check.sh(1 hunks)tests/functional/flakes/common.sh(2 hunks)tests/functional/flakes/show.sh(5 hunks)tests/functional/formatter.sh(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (10)
src/libexpr/include/nix/expr/eval-cache.hh (2)
src/libcmd/flake-schemas.cc (1)
state(248-248)src/libcmd/include/nix/cmd/installable-flake.hh (2)
state(63-63)state(69-69)
src/nix/flake.cc (4)
src/libflake/flake.cc (4)
lockFlake(379-894)lockFlake(379-380)lockFlake(896-904)lockFlake(897-897)src/libflake/include/nix/flake/flake.hh (2)
lockFlake(245-245)lockFlake(247-248)src/libcmd/flake-schemas.cc (14)
call(37-120)call(38-38)getDefaultFlakeSchemas(295-301)getDefaultFlakeSchemas(295-295)visit(146-186)visit(146-151)derivation(206-209)derivation(206-206)forEachOutput(122-144)forEachOutput(122-124)what(188-194)what(188-188)shortDescription(196-204)shortDescription(196-196)src/libcmd/include/nix/cmd/flake-schemas.hh (6)
call(12-12)visit(20-25)derivation(31-31)forEachOutput(14-16)what(27-27)shortDescription(29-29)
src/nix/search.cc (1)
src/libexpr/include/nix/expr/eval-cache.hh (1)
attrPath(158-158)
src/libcmd/installable-flake.cc (5)
src/libcmd/flake-schemas.cc (7)
state(248-248)call(37-120)call(38-38)getSchema(246-277)getSchema(246-246)getOutput(211-244)getOutput(211-211)src/libexpr/eval-cache.cc (2)
toAttrPathStr(398-401)toAttrPathStr(398-398)src/libcmd/include/nix/cmd/flake-schemas.hh (3)
call(12-12)getSchema(52-52)getOutput(40-40)src/libexpr/attr-path.cc (4)
parseAttrPath(6-31)parseAttrPath(6-6)parseAttrPath(33-39)parseAttrPath(33-33)src/libcmd/installables.cc (2)
openEvalCache(429-458)openEvalCache(429-429)
src/libflake/flake.cc (3)
src/libflake/include/nix/flake/flake.hh (4)
settings(68-68)lockFlake(245-245)lockFlake(247-248)getFlake(124-125)src/nix/flake.cc (2)
lockFlake(58-61)lockFlake(58-58)src/libflake/flake-primops.cc (2)
getFlake(9-56)getFlake(9-9)
src/nix/profile.cc (1)
src/libcmd/flake-schemas.cc (2)
getDefaultFlakeSchemas(295-301)getDefaultFlakeSchemas(295-295)
src/libcmd/installables.cc (3)
src/libcmd/include/nix/cmd/command.hh (3)
completeFlakeRefWithFragment(390-395)completions(166-166)prefix(315-315)src/libcmd/command.cc (2)
getEvalState(125-145)getEvalState(125-125)src/libcmd/flake-schemas.cc (2)
getDefaultFlakeSchemas(295-301)getDefaultFlakeSchemas(295-295)
src/libcmd/include/nix/cmd/flake-schemas.hh (2)
src/libcmd/flake-schemas.cc (1)
state(248-248)src/libexpr/include/nix/expr/eval-cache.hh (1)
attrPath(158-158)
src/libcmd/flake-schemas.cc (3)
src/libflake/flake.cc (8)
readFlake(207-311)readFlake(207-213)lockFlake(379-894)lockFlake(379-380)lockFlake(896-904)lockFlake(897-897)callFlake(927-970)callFlake(927-927)src/libcmd/include/nix/cmd/flake-schemas.hh (1)
what(27-27)src/libcmd/installables.cc (2)
completeFlakeRef(387-408)completeFlakeRef(387-387)
src/libexpr/eval-cache.cc (2)
src/libexpr/include/nix/expr/eval-cache.hh (5)
attrPath(158-158)name(136-136)name(138-138)name(142-142)toAttrPathStr(18-18)src/libutil/include/nix/util/strings.hh (4)
dropEmptyInitThenConcatStringsSep(87-87)dropEmptyInitThenConcatStringsSep(89-89)dropEmptyInitThenConcatStringsSep(90-90)dropEmptyInitThenConcatStringsSep(91-91)
🪛 Clang (14.0.6)
src/libcmd/include/nix/cmd/flake-schemas.hh
[error] 3-3: 'nix/expr/eval-cache.hh' file not found
(clang-diagnostic-error)
src/libcmd/flake-schemas.cc
[error] 1-1: 'nix/cmd/flake-schemas.hh' file not found
(clang-diagnostic-error)
🪛 markdownlint-cli2 (0.18.1)
doc/manual/source/protocols/flake-schemas.md
7-7: Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (36)
src/libcmd/include/nix/cmd/meson.build (1)
12-12: LGTM: export of flake-schemas.hh.Header exposure looks correct and matches usage elsewhere.
Ensure this header has no private includes to avoid leaking build-only headers into the public surface.
src/libcmd/include/nix/cmd/installable-flake.hh (2)
58-59: LGTM: fragment-based identity and lazy eval cache.The
what()change and cachedopenEvalCache()accessor look good.Consider a brief note on thread-safety of
_evalCacheif used across threads.Also applies to: 75-80
39-45: All construction sites properly updated; lockFlags lifetime is safe.Verification confirms the changes are complete:
- Single construction site (src/nix/flake.cc:513) includes all required fields:
fragment(templateName),roles({"nix-template"}),extendedOutputsSpec,lockFlags, anddefaultFlakeSchemas.- lockFlags lifetime is safe: it's stored by const reference from a
CmdFlakeInitCommonmember variable that outlives the temporaryInstallableFlakeobject.- roles propagation is correct through schema filtering and compatibility checks.
- getActualAttrPaths is only mentioned in a documentation comment at the header; no actual method exists to update.
src/libcmd/meson.build (1)
71-71: All verification checks passed. Build wiring is properly configured.The verification confirms:
- Generator subdir is visible at line 70:
subdir('nix-meson-build-support/generate-header')- Both input files exist:
src/libcmd/call-flake-schemas.nixandsrc/libcmd/builtin-flake-schemas.nix- gen_header.process calls are correctly wired at lines 92-93
- Output header file is properly generated and included in multiple source files
- No presence check issues identified
src/libcmd/flake-schemas.cc (1)
286-293: Nice CLI integration for default flake schemas.Flag wiring and completer look good.
src/libcmd/package.nix (1)
52-53: The Meson infrastructure is already properly configured. Thegen_header.process()calls at lines 92–93 ofsrc/libcmd/meson.buildgenerate the.gen.hhheaders from both.nixsources, and the fileset insrc/libcmd/package.nixalready includes:
- The
.nixsources (lines 52–53, your PR addition)- The
include/nix/cmd/meson.buildsubdirectory (line 49)- All
.hhheaders via the file filter (line 51)No further action needed; the changes are correct.
doc/manual/source/SUMMARY.md.in (1)
119-119: LGTM!The addition of the Flake Schemas entry to the Table of Contents is well-placed within the "Formats and Protocols" section and follows the existing formatting conventions.
src/nix/formatter.cc (1)
37-40: LGTM!The migration from
getDefaultFlakeAttrPaths()togetRoles()is clean and aligns with the role-based flake schema approach introduced in this PR. The role name"nix-fmt"is descriptive and appropriate for the formatter component.src/nix/repl.cc (1)
48-51: LGTM!The migration to
getRoles()returning{"nix-repl"}is appropriate and consistent with the role-based approach. This is a clear improvement over the previous empty string return value.tests/functional/flakes/common.sh (2)
98-104: LGTM!The refactoring of
writeIfdFlaketo wrap the IFD import in a proper derivation structure aligns well with the schema-driven approach. The derivation name "top" matches the test expectations inshow.sh.
5-5: No issues found—change maintains IFD test coverage through controlled, opt-in approach.The variable was intentionally moved from a global export to explicit per-test invocation. Evidence shows:
- Still actively used in tests that require it (non-flake-inputs.sh, flake-in-submodule.sh, mercurial.sh) by explicitly setting
_NIX_TEST_BARF_ON_UNCACHEABLE=''- Dedicated IFD test coverage exists via
allow-import-from-derivationandtrace-import-from-derivationoptions (trace-ifd.sh, eval-cache.sh)- This refactoring provides more granular control than the previous global setting
src/nix/flake-check.md (1)
21-31: LGTM!The updated description clearly explains the new schema-driven validation approach for
nix flake check. The documentation accurately describes:
- Schema-based content extraction from flake outputs
- Evaluation of
evalChecksattributes with error reporting- Conditional building of derivations based on
isFlakeCheckThis aligns well with the new flake schemas documentation and provides users with a clear understanding of the command's behavior.
tests/functional/flakes/show.sh (5)
18-20: LGTM!The updated assertions correctly validate the new schema-driven output structure. The changes properly test:
- System filtering via
.filteredattribute- Derivation name extraction via
.derivationName- Legacy packages skip behavior via
.skipped
29-30: LGTM!The
--all-systemstest correctly verifies that packages for all systems are shown with proper derivation names, while legacy packages remain skipped by default.
39-39: LGTM!The
--legacyflag test properly validates that legacy packages are shown with the correct nested structure and derivation name.
59-61: LGTM!The error handling test correctly validates that failed evaluations and successful derivations coexist in the output structure with appropriate
.failedand.derivationNameattributes.
70-70: LGTM!The simplified jq-based assertion is more concise and directly validates that the IFD test produces the expected derivation name "top".
tests/functional/flakes/check.sh (1)
115-115: Unable to verify pattern match in sandbox environment.The test script requires Nix tooling and environment variables (
_NIX_TEST_SOURCE_DIR) that are not available in the sandbox. The pattern"Evaluation check.*apps.system-1.default.isValidApp.*failed"appears structurally sound based on code context (it targets schema validation errors for app definitions), but actual verification requires running the full test suite locally.The pattern uses flexible matching (.*) for the middle section, which is appropriate for accommodating variation in error messages while remaining specific enough to target the intended error case.
src/nix/run.cc (1)
137-140: LGTM: role matches command semantics.Returning {"nix-run"} aligns with run’s schema-based resolution.
src/nix/develop.cc (2)
461-464: LGTM: role name is appropriate for develop.Using {"nix-develop"} for default resolution fits the feature.
644-647: Role nix-build is properly supported in the schema and exception handling is already present; review comment is incorrect.The nix-build role is explicitly defined in the flake schema for legacyPackages output, which means bashInteractive lookup via this role is schema-compliant. Additionally, the code already includes exception handling (lines 663-665) that silently falls back to the system shell if resolution fails. The suggested diff only adds a comment without implementing functional fallback logic. No changes are required.
Likely an incorrect or invalid review comment.
src/nix/bundle.cc (1)
61-64: Verify semantic alignment of getRoles() with bundle command purpose.CmdBundle declares role
"nix-run"(for apps), but conceptually operates on derivations for bundling. While this technically works—because thepackagesschema also carries thenix-runrole—the role declaration is semantically misaligned. When a user runsnix bundle <flake>without an explicit fragment, the role"nix-run"filters to schemas with that role; bothpackagesandappsmatch, but declaring"nix-run"obscures the bundling intent. Consider confirming whether this is intentional or ifgetRoles()should reflect the bundling semantic (e.g.,{"nix-bundler"}if feasible, or a clarifying comment if current design is deliberate).src/nix/search.cc (1)
91-95: AttrPath migration looks correctSwitch to eval_cache::AttrPath and resolving via symbols is consistent with eval-cache APIs. No issues spotted.
src/libflake/flake.cc (2)
379-405: New lockFlake overload: solid factoringAccepting a precomputed Flake avoids refetching and simplifies call chains. The by-value parameter is appropriate since it’s mutated and moved into the result.
896-904: Delegating overload is correctThe legacy signature now delegates to the new overload using getFlake; nice for backwards compatibility.
src/nix/flake.cc (3)
303-321: Good: schema-driven wiring in CmdFlakeCheckAdopting MixFlakeSchemas and deferring to flake_schemas::call is the right direction.
483-504: Template completion uses roles correctlyUsing {"nix-template"} for template completion aligns with the new roles model.
761-779: CmdFlakeShow: role/schema integration LGTMMixFlakeSchemas integration and inventory traversal look correct.
src/libcmd/installables.cc (1)
292-293: Completion pipeline correctly wired through getRoles()Verification confirms the implementation is sound: derived commands appropriately override
getRoles()with their specific role identifiers (nix-repl, nix-develop, nix-run, nix-fmt, etc.), and the base implementation returns{"nix-build"}. The call at line 292 correctly passes roles to the completion system.src/libcmd/installable-flake.cc (3)
29-37: Stateful attr-path rendering looks correctUses eval_cache::toAttrPathStr(state, ...) and clear separators; no issues spotted.
48-58: Constructor API change verified: roles/defaultFlakeSchemas wiring confirmedVerification complete. Found one call site at src/nix/flake.cc:513-521, which correctly passes all 8 parameters to the InstallableFlake constructor:
rolesis non-empty:{"nix-template"}defaultFlakeSchemasis properly passed as an empty optional{}All other parameters (cmd, state, flakeRef, fragment, extendedOutputsSpec, lockFlags) are correctly wired. No outdated constructor calls remain.
258-264: Verify cache consolidation before mergingThe concern is valid. The codebase has multiple cache implementations:
- The method under review creates a schema-aware cache (using
flake_schemas::call()withfingerprint2)- A free function at
src/libcmd/installables.cc:429(marked// FIXME: remove) creates a non-schema cache and is still called at line 327- Separate schema cache creation in
src/libcmd/flake-schemas.cc:61(with// FIXME: merge with openEvalCache())These use different fingerprints and may cause inconsistent
cleanupAttrPathbehavior. The codebase already flags this as technical debt. Confirm that this PR doesn't introduce new inconsistencies or is part of a broader consolidation effort.src/libexpr/eval-cache.cc (3)
122-137: AttrPath-based caching looks consistentDB writes/reads now store lists as AttrPath; getAttrs returns sorted symbols; no issues spotted.
Also applies to: 257-262, 687-717
371-385: The concern in the review comment is unfounded—cleanupAttrPath already has a safe default.The member declaration in
src/libexpr/include/nix/expr/eval-cache.hh(line 45) contains an in-class initializer that provides an identity lambda as the default:std::function<AttrPath(AttrPath &&)> cleanupAttrPath = [](AttrPath && attrPath) { return std::move(attrPath); };This means all
EvalCacheinstances—whether created ininstallables.ccorflake-schemas.cc—automatically receive this safe default. The empty constructor ineval-cache.cc(lines 298–304) does not override this initialization. The call toroot->cleanupAttrPath()ineval-cache.cc(lines 383, 395) will always invoke a valid function, eliminating any risk ofstd::bad_function_call.The suggested constructor modification is redundant.
Likely an incorrect or invalid review comment.
311-315: Confirm this is intentional test infrastructure and verify test environment does not set NIX_ALLOW_EVAL=0The behavior change is real and intentional. New code throws when
NIX_ALLOW_EVAL=0, guarding against incomplete cache evaluation. Since the default is"1", normal operation is unaffected. However, if any test framework or CI sets this variable to0before triggeringgetRoot()(called from CLI code in installables.cc, installable-flake.cc, flake.cc), it will fail with "not everything is cached, but evaluation is not allowed."No existing functional tests currently set or reference this variable, so verify that test setup and CI configuration don't inadvertently introduce this condition.
src/libcmd/include/nix/cmd/flake-schemas.hh (1)
3-3: Based on my verification of the codebase:libexpr properly exports include directories: The export/meson.build in libexpr declares the dependency with
include_directories : include_dirs, whereinclude_dirspoints tosrc/libexpr/include/.libcmd correctly declares nix-expr as a public dependency: In libcmd's meson.build,
dependency('nix-expr')is listed indeps_public_maybe_subproject, which properly includes nix-expr's include paths during compilation.The header exists: eval-cache.hh is confirmed in src/libexpr/include/nix/expr/meson.build's headers list.
The Meson build system automatically propagates include directories from declared dependencies to consumers, so the #include "nix/expr/eval-cache.hh" in flake-schemas.hh should resolve correctly during the build of libcmd. The dependency chain is properly configured in the Meson files.
| allSchemas = (flake.outputs.schemas or defaultSchemasFlake.schemas) // schemaOverrides; | ||
|
|
||
| schemaOverrides = { }; # FIXME | ||
|
|
There was a problem hiding this comment.
Fix dynamic attribute membership checks (syntax/semantics).
attrs ? ${name} is invalid for dynamic keys; use builtins.hasAttr. Also guard defaultSchemasFlake.schemas with a fallback. Without this, evaluation will fail.
- allSchemas = (flake.outputs.schemas or defaultSchemasFlake.schemas) // schemaOverrides;
+ allSchemas = (flake.outputs.schemas or (defaultSchemasFlake.schemas or { })) // schemaOverrides;
@@
- if allSchemas ? ${outputName} then
+ if builtins.hasAttr outputName allSchemas then
@@
- if schemas ? ${outputName} && schemas.${outputName}.version == 1 then
+ if builtins.hasAttr outputName schemas && schemas.${outputName}.version == 1 thenOptional simplification (clearer intent, fewer passes):
- schemas = builtins.listToAttrs (
- builtins.concatLists (
- mapAttrsToList (
- outputName: output:
- if builtins.hasAttr outputName allSchemas then
- [
- {
- name = outputName;
- value = allSchemas.${outputName};
- }
- ]
- else
- [ ]
- ) flake.outputs
- )
- );
+ schemas = builtins.mapAttrs (n: _: allSchemas.${n})
+ (builtins.filterAttrs (n: _: builtins.hasAttr n allSchemas) flake.outputs);Also applies to: 22-37, 41-53
🤖 Prompt for AI Agents
In src/libcmd/call-flake-schemas.nix around lines 18-21 (and similarly for 22-37
and 41-53), dynamic attribute membership checks using the attrs ? ${name}
pattern are invalid and will break evaluation; replace those with
builtins.hasAttr name attrs, and guard accesses to defaultSchemasFlake.schemas
with a fallback (e.g., treat defaultSchemasFlake as optional and use an empty
set or defaultSchemas when it’s missing). Also simplify the logic by resolving
the base schemas via a conditional/fallback once (e.g., pick
flake.outputs.schemas if present else defaultSchemasFlake.schemas or {}), then
merge in schemaOverrides, so you avoid repeated membership checks and ensure
evaluation never fails on missing attributes.
| #include "nix/cmd/flake-schemas.hh" | ||
| #include "nix/expr/eval-settings.hh" | ||
| #include "nix/fetchers/fetch-to-store.hh" | ||
| #include "nix/util/memory-source-accessor.hh" | ||
|
|
There was a problem hiding this comment.
Include required standard headers.
This file uses std::span and std::find; add <span> and <algorithm> to avoid relying on transitive includes.
#include "nix/cmd/flake-schemas.hh"
#include "nix/expr/eval-settings.hh"
#include "nix/fetchers/fetch-to-store.hh"
#include "nix/util/memory-source-accessor.hh"
+#include <span>
+#include <algorithm>📝 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.
| #include "nix/cmd/flake-schemas.hh" | |
| #include "nix/expr/eval-settings.hh" | |
| #include "nix/fetchers/fetch-to-store.hh" | |
| #include "nix/util/memory-source-accessor.hh" | |
| #include "nix/cmd/flake-schemas.hh" | |
| #include "nix/expr/eval-settings.hh" | |
| #include "nix/fetchers/fetch-to-store.hh" | |
| #include "nix/util/memory-source-accessor.hh" | |
| #include <span> | |
| #include <algorithm> |
🧰 Tools
🪛 Clang (14.0.6)
[error] 1-1: 'nix/cmd/flake-schemas.hh' file not found
(clang-diagnostic-error)
🤖 Prompt for AI Agents
In src/libcmd/flake-schemas.cc lines 1 to 5, the file uses std::span and
std::find but does not include the corresponding standard headers; add #include
<span> and #include <algorithm> to the top of the file (adjacent to the existing
includes) so the code does not depend on transitive includes.
| if (system) { | ||
| if (auto forSystems = node->maybeGetAttr("forSystems")) { | ||
| auto systems = forSystems->getListOfStrings(); | ||
| if (std::find(systems.begin(), systems.end(), system) == systems.end()) { | ||
| visitFiltered(node, systems); | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
Compile fix: dereference system when searching.
std::find(..., system) passes std::optional<std::string> instead of std::string. Use *system.
- if (std::find(systems.begin(), systems.end(), system) == systems.end()) {
+ if (std::find(systems.begin(), systems.end(), *system) == systems.end()) {📝 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.
| if (system) { | |
| if (auto forSystems = node->maybeGetAttr("forSystems")) { | |
| auto systems = forSystems->getListOfStrings(); | |
| if (std::find(systems.begin(), systems.end(), system) == systems.end()) { | |
| visitFiltered(node, systems); | |
| return; | |
| } | |
| } | |
| if (system) { | |
| if (auto forSystems = node->maybeGetAttr("forSystems")) { | |
| auto systems = forSystems->getListOfStrings(); | |
| if (std::find(systems.begin(), systems.end(), *system) == systems.end()) { | |
| visitFiltered(node, systems); | |
| return; | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/libcmd/flake-schemas.cc around lines 156–163, the call to std::find is
passing the std::optional<std::string> named system instead of the contained
string; inside the surrounding if (system) block dereference the optional (use
*system) when calling std::find so the comparison uses std::string, e.g. replace
system with *system in the std::find call.
| #pragma once | ||
|
|
||
| #include "nix/expr/eval-cache.hh" | ||
| #include "nix/flake/flake.hh" | ||
| #include "nix/cmd/command.hh" | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Public header hygiene: add required includes and remove using namespace
- Avoid
using namespacein headers; it pollutes consumers. - Include all standard headers used here: , , , , .
- Ensure StringSet definition is available (typically nix/util/types.hh).
#pragma once
-#include "nix/expr/eval-cache.hh"
-#include "nix/flake/flake.hh"
-#include "nix/cmd/command.hh"
+#include <functional>
+#include <map>
+#include <optional>
+#include <string>
+#include <vector>
+
+#include "nix/util/types.hh"
+#include "nix/expr/eval-cache.hh"
+#include "nix/flake/flake.hh"
+#include "nix/cmd/command.hh"
-namespace nix::flake_schemas {
-
-using namespace eval_cache;
+namespace nix::flake_schemas {If desired, add explicit aliases instead of a global using:
- ref<eval_cache::EvalCache>
+ ref<eval_cache::EvalCache>
call(EvalState & state, std::shared_ptr<flake::LockedFlake> lockedFlake, std::optional<FlakeRef> defaultSchemasFlake);(no change needed beyond removing using namespace since fully-qualified names are already used)
Also applies to: 7-13, 33-53
🧰 Tools
🪛 Clang (14.0.6)
[error] 3-3: 'nix/expr/eval-cache.hh' file not found
(clang-diagnostic-error)
🤖 Prompt for AI Agents
In src/libcmd/include/nix/cmd/flake-schemas.hh (lines 1-6, and also apply fixes
around 7-13 and 33-53), remove any "using namespace" directives from the header,
and add explicit #include directives for the standard headers referenced in the
header: <functional>, <map>, <optional>, <string>, and <vector>; also include
the header that defines StringSet (e.g., nix/util/types.hh). Ensure all types
are referenced with fully-qualified names or explicit aliases in the header
rather than introducing a global using directive.
| const StringSet & roles, | ||
| std::string_view prefix) | ||
| { |
There was a problem hiding this comment.
Fragment completion is currently stubbed out; re-enable completions and use roles
The body under this function leaves the actual attribute completion disabled (#if 0) and ignores the roles argument. This breaks nix <cmd> <flake>#<fragment> completions.
Apply a minimal reimplementation that:
- Uses the eval cache root to findAlongAttrPath for the typed fragment
- Completes the last token
- Consumes roles later (or mark unused to avoid warnings)
Proposed patch (keeps schema-neutral behavior and fixes completions today):
void completeFlakeRefWithFragment(
AddCompletions & completions,
ref<EvalState> evalState,
flake::LockFlags lockFlags,
- const StringSet & roles,
+ const StringSet & roles,
std::string_view prefix)
{
/* Look for flake output attributes that match the
prefix. */
try {
auto hash = prefix.find('#');
if (hash == std::string::npos) {
completeFlakeRef(completions, evalState->store, prefix);
} else {
completions.setType(AddCompletions::Type::Attrs);
auto fragment = prefix.substr(hash + 1);
std::string prefixRoot = "";
if (fragment.starts_with(".")) {
fragment = fragment.substr(1);
prefixRoot = ".";
}
auto flakeRefS = std::string(prefix.substr(0, hash));
// TODO: ideally this would use the command base directory instead of assuming ".".
auto flakeRef =
parseFlakeRef(fetchSettings, expandTilde(flakeRefS), std::filesystem::current_path().string());
- auto evalCache = openEvalCache(
- *evalState,
- std::make_shared<flake::LockedFlake>(lockFlake(flakeSettings, *evalState, flakeRef, lockFlags)));
+ auto evalCache = openEvalCache(
+ *evalState,
+ std::make_shared<flake::LockedFlake>(lockFlake(flakeSettings, *evalState, flakeRef, lockFlags)));
auto root = evalCache->getRoot();
-
-#if 0
- if (prefixRoot == ".") {
- attrPathPrefixes.clear();
- }
- /* Complete 'fragment' relative to all the
- attrpath prefixes as well as the root of the
- flake. */
- attrPathPrefixes.push_back("");
-
- for (auto & attrPathPrefixS : attrPathPrefixes) {
- auto attrPathPrefix = parseAttrPath(*evalState, attrPathPrefixS);
- auto attrPathS = attrPathPrefixS + std::string(fragment);
- auto attrPath = parseAttrPath(*evalState, attrPathS);
-
- std::string lastAttr;
- if (!attrPath.empty() && !hasSuffix(attrPathS, ".")) {
- lastAttr = evalState->symbols[attrPath.back()];
- attrPath.pop_back();
- }
-
- auto attr = root->findAlongAttrPath(attrPath);
- if (!attr)
- continue;
-
- for (auto & attr2 : (*attr)->getAttrs()) {
- if (hasPrefix(evalState->symbols[attr2], lastAttr)) {
- auto attrPath2 = (*attr)->getAttrPath(attr2);
- /* Strip the attrpath prefix. */
- attrPath2.erase(attrPath2.begin(), attrPath2.begin() + attrPathPrefix.size());
- // FIXME: handle names with dots
- completions.add(
- flakeRefS + "#" + prefixRoot
- + concatStringsSep(".", evalState->symbols.resolve(attrPath2)));
- }
- }
- }
-
- /* And add an empty completion for the default
- attrpaths. */
- if (fragment.empty()) {
- for (auto & attrPath : defaultFlakeAttrPaths) {
- auto attr = root->findAlongAttrPath(parseAttrPath(*evalState, attrPath));
- if (!attr)
- continue;
- completions.add(flakeRefS + "#" + prefixRoot);
- }
- }
-#endif
+ // Temporary: roles currently unused in completion logic
+ (void)roles;
+
+ // Minimal reimplementation without attrPathPrefixes:
+ const std::string fragS(fragment);
+ auto attrPath = parseAttrPath(*evalState, fragS);
+ std::string lastToken;
+ if (!fragS.empty() && !hasSuffix(fragS, ".")) {
+ lastToken = evalState->symbols[attrPath.back()];
+ attrPath.pop_back();
+ }
+ auto cur = root->findAlongAttrPath(attrPath);
+ if (!cur) return;
+ for (auto & name : (*cur)->getAttrs()) {
+ auto s = std::string(evalState->symbols[name]);
+ if (!lastToken.empty() && !hasPrefix(s, lastToken)) continue;
+ auto p2 = (*cur)->getAttrPath(name);
+ auto rel = concatStringsSep(".", evalState->symbols.resolve(p2).substr(attrPath.size()));
+ // TODO: handle names with dots
+ completions.add(flakeRefS + "#" + prefixRoot + rel);
+ }
}
} catch (Error & e) {
warn(e.msg());
}
}If you want role-aware completions now, I can wire this to flake_schemas::call and enumerate inventory paths filtered by roles.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/libcmd/installables.cc around lines 303 to 305, the function body
currently leaves attribute completion disabled and ignores the roles parameter;
re-enable a minimal completion implementation that uses the eval cache root to
call findAlongAttrPath for the typed fragment, generate completions for the last
token only, and return them; ensure the roles parameter is either consumed later
or explicitly marked unused (e.g. (void)roles) to avoid warnings; keep the
implementation schema-neutral and minimal so completions work for `nix <cmd>
<flake>#<fragment>` today.
| {"nix-bundler"}, | ||
| lockFlags, | ||
| std::nullopt}; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Bundler InstallableFlake: verify role name and consider default schemas.
Roles={"nix-bundler"} assumes schema support; if missing, resolution will fail. Also consider honoring a user-provided default schemas flake here for consistency with other commands.
Apply if CmdBundle has MixFlakeSchemas; otherwise ignore:
- {"nix-bundler"},
- lockFlags,
- std::nullopt};
+ {"nix-bundler"},
+ lockFlags,
+ getDefaultFlakeSchemas()};Verification:
🏁 Script executed:
#!/bin/bash
# Ensure CmdBundle inherits a mixin that provides getDefaultFlakeSchemas.
rg -n --no-ignore 'struct\s+CmdBundle\s*:\s*[^\\{]*' -C2
rg -n --no-ignore 'struct\s+InstallableValueCommand\b' -C10
# Confirm "nix-bundler" appears in schema code.
rg -n --no-ignore -S 'nix-bundler'Length of output: 2062
---
🏁 Script executed:
```shell
#!/bin/bash
# Find InstallableCommand definition and its inheritance chain
rg -n --no-ignore 'struct\s+InstallableCommand\b' -A5
# Search for MixFlakeSchemas in the codebase
rg -n --no-ignore 'MixFlakeSchemas' -C2
# Check if CmdBundle or its parents use MixFlakeSchemas
rg -n --no-ignore 'class CmdBundle\|struct CmdBundle' -A20
# Find getDefaultFlakeSchemas definition
rg -n --no-ignore 'getDefaultFlakeSchemas' -C3
Length of output: 5042
---
🏁 Script executed:
```shell
#!/bin/bash
# View the full context around the code in question
sed -n '75,90p' src/nix/bundle.cc
# Verify InstallableFlake constructor signature
rg -n --no-ignore 'InstallableFlake\s*\(' -A3 src/libcmd/installables.hh
Length of output: 602
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for InstallableFlake class definition
rg -n --no-ignore 'struct\s+InstallableFlake\b|class\s+InstallableFlake\b' -A15
# Find where InstallableFlake is declared
fd -name 'installables*' -type f | head -20
Length of output: 1557
---
🏁 Script executed:
```shell
#!/bin/bash
# View the full InstallableFlake constructor signature
sed -n '46,65p' src/libcmd/include/nix/cmd/installable-flake.hh
Length of output: 557
Replace std::nullopt with getDefaultFlakeSchemas() in InstallableFlake constructor call.
The review comment is correct. CmdBundle inherits from InstallableValueCommand → InstallableCommand → SourceExprCommand, which extends MixFlakeSchemas. This inheritance provides access to getDefaultFlakeSchemas(). The last parameter of the InstallableFlake constructor at line 83 should pass the default schemas instead of std::nullopt, consistent with usage patterns in installables.cc and profile.cc. The "nix-bundler" role is properly defined in the schema (builtin-flake-schemas.nix:364).
🤖 Prompt for AI Agents
In src/nix/bundle.cc around lines 81-84, the InstallableFlake constructor call
currently passes std::nullopt for the flake schemas; replace that argument with
getDefaultFlakeSchemas() so the call uses the class-provided default schemas
(accessible via the MixFlakeSchemas inheritance chain) consistent with
installables.cc and profile.cc and matching the defined "nix-bundler" role in
builtin-flake-schemas.nix.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/libcmd/flake-schemas.cc (2)
1-6: Include required standard headers.This file uses
std::span(line 228) andstd::find(line 161) but does not include the corresponding headers. Add#include <span>and#include <algorithm>to avoid relying on transitive includes.Apply this diff:
#include "nix/cmd/flake-schemas.hh" #include "nix/expr/eval-settings.hh" #include "nix/fetchers/fetch-to-store.hh" #include "nix/util/memory-source-accessor.hh" #include "nix/util/mounted-source-accessor.hh" +#include <span> +#include <algorithm>
158-165: Dereference optional when searching systems.Inside the
if (system)block,systemisstd::optional<std::string>and must be dereferenced when passed tostd::find.Apply this diff:
if (auto forSystems = node->maybeGetAttr("forSystems")) { auto systems = forSystems->getListOfStrings(); - if (std::find(systems.begin(), systems.end(), system) == systems.end()) { + if (std::find(systems.begin(), systems.end(), *system) == systems.end()) { visitFiltered(node, systems); return; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/libcmd/flake-schemas.cc(1 hunks)src/libstore/include/nix/store/store-dir-config.hh(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/libcmd/flake-schemas.cc (3)
src/libflake/include/nix/flake/flake.hh (4)
readFlake(232-238)lockFlake(245-245)lockFlake(247-248)callFlake(250-250)src/libflake/flake.cc (8)
readFlake(207-311)readFlake(207-213)lockFlake(379-894)lockFlake(379-380)lockFlake(896-904)lockFlake(897-897)callFlake(927-970)callFlake(927-927)src/libcmd/installables.cc (2)
completeFlakeRef(387-408)completeFlakeRef(387-387)
🪛 Clang (14.0.6)
src/libcmd/flake-schemas.cc
[error] 1-1: 'nix/cmd/flake-schemas.hh' file not found
(clang-diagnostic-error)
🔇 Additional comments (8)
src/libcmd/flake-schemas.cc (8)
12-37: LGTM: Synthetic flake construction is well-structured.The function correctly constructs a virtual flake from builtin schemas, materializes it to the store, and returns a locked flake. The approach of using
MemorySourceAccessorwith a mounted virtual store path is sound.
39-122: LGTM: Eval cache wiring with fingerprint-based invalidation.The function correctly:
- Constructs a composite fingerprint from all inputs for cache invalidation
- Conditionally skips default schemas when the flake provides its own (lines 76-79)
- Transforms inventory attribute paths back to output paths via
cleanupAttrPath(lines 91-119)The lazy evaluation and cache integration are well-designed.
124-146: LGTM: Output iteration with graceful handling of unknown outputs.The function correctly iterates over inventory outputs and handles outputs that couldn't be fully evaluated (lines 135-139). Error tracing is appropriate.
168-188: LGTM: Node traversal logic with appropriate error handling.The recursive traversal correctly distinguishes between leaf and non-leaf nodes. The special case for
legacyPackages(line 176) to suppress evaluation errors is appropriate, as legacy packages often have platform-specific or broken attributes.
190-211: LGTM: Helper functions are straightforward.The attribute accessor helpers (
what,shortDescription,derivation) are correctly implemented with appropriate null handling and string trimming.
213-246: LGTM: Output resolution correctly walks the attribute path.The function appropriately:
- Uses
std::spanfor efficient path traversal without copying (line 228)- Walks through nested children to resolve the target node (lines 230-239)
- Returns
std::nulloptwhen the path doesn't exist- Constructs
OutputInfowith the resolved cursors and remaining path
248-279: LGTM: Schema collection logic is correct.The function correctly:
- Iterates inventory attributes to collect schema definitions
- Extracts roles, appendSystem flag, and defaultAttrPath from each schema
- Converts string lists to symbol-based
AttrPath(lines 270-271)- Builds and returns the
Schemasmap
285-303: LGTM: CLI integration is standard and correct.The
MixFlakeSchemasclass correctly:
- Registers the
--default-flake-schemasflag with flake ref completion (lines 287-294)- Parses the flag value into a
FlakeRefwith proper base directory resolution (line 302)
| std::string_view name, | ||
| const SourcePath & path, | ||
| ContentAddressMethod method = FileIngestionMethod::NixArchive, | ||
| ContentAddressMethod method = ContentAddressMethod::Raw::NixArchive, |
There was a problem hiding this comment.
🧩 Analysis chain
Verify migration from FileIngestionMethod to ContentAddressMethod.
The default parameter update looks correct and aligns with the type system refactoring.
Run the following script to verify all references to FileIngestionMethod have been properly migrated:
🏁 Script executed:
#!/bin/bash
# Description: Search for remaining references to FileIngestionMethod that may need updating
# Search for FileIngestionMethod usage in the codebase
echo "=== Searching for FileIngestionMethod references ==="
rg -n --type=cpp --type=c 'FileIngestionMethod' -C 2
# Search for calls to computeStorePath that might need updating
echo -e "\n=== Searching for computeStorePath calls ==="
rg -n --type=cpp --type=c 'computeStorePath\s*\(' -C 2Length of output: 44797
🏁 Script executed:
#!/bin/bash
# Verify the exact context of ContentAddressMethod parameters with FileIngestionMethod defaults
echo "=== Checking dummy-store.cc line 87 ==="
sed -n '82,92p' src/libstore/dummy-store.cc
echo -e "\n=== Checking legacy-ssh-store.hh line 116 ==="
sed -n '111,121p' src/libstore/include/nix/store/legacy-ssh-store.hh
echo -e "\n=== Checking remote-store.hh line 89 ==="
sed -n '84,94p' src/libstore/include/nix/store/remote-store.hh
echo -e "\n=== Checking content-address.hh for ContentAddressMethod definition ==="
grep -A 5 "class ContentAddressMethod" src/libstore/include/nix/store/content-address.hh | head -20
echo -e "\n=== Check for any implicit constructor/conversion in ContentAddressMethod ==="
rg -A 10 "struct ContentAddressMethod\|class ContentAddressMethod" src/libstore/include/nix/store/content-address.hh | head -30Length of output: 1716
🏁 Script executed:
#!/bin/bash
# Find the ContentAddressMethod type definition
echo "=== Full grep for ContentAddressMethod definition ==="
rg "ContentAddressMethod\s+\{" src/libstore/include/nix/store/content-address.hh -A 20 | head -40
echo -e "\n=== Search for ContentAddressMethod in header ==="
cat src/libstore/include/nix/store/content-address.hh | grep -n "ContentAddressMethod" | head -15
echo -e "\n=== Check for implicit conversion or constructor ==="
rg "ContentAddressMethod\(" src/libstore/include/nix/store/content-address.hh
echo -e "\n=== Check the actual struct definition (not class) ==="
sed -n '20,80p' src/libstore/include/nix/store/content-address.hhLength of output: 2852
🏁 Script executed:
#!/bin/bash
# Find MAKE_WRAPPER_CONSTRUCTOR definition
echo "=== Search for MAKE_WRAPPER_CONSTRUCTOR definition ==="
rg "MAKE_WRAPPER_CONSTRUCTOR" --type=cpp --type=h -B 2 -A 5 | head -40
echo -e "\n=== Check if there's a conversion from FileIngestionMethod to ContentAddressMethod ==="
rg "FileIngestionMethod.*ContentAddressMethod\|ContentAddressMethod.*FileIngestionMethod" --type=cpp --type=h | head -10
echo -e "\n=== Verify the method signature patterns in the three files have same pattern ==="
echo "dummy-store.cc:"
sed -n '85,90p' src/libstore/dummy-store.cc | grep -A 3 "ContentAddressMethod"
echo -e "\nlegacy-ssh-store.hh:"
sed -n '115,120p' src/libstore/include/nix/store/legacy-ssh-store.hh | grep -A 3 "ContentAddressMethod"
echo -e "\nremote-store.hh:"
sed -n '88,93p' src/libstore/include/nix/store/remote-store.hh | grep -A 3 "ContentAddressMethod"Length of output: 3848
Update three addToStoreFromDump method default parameters to match the refactoring pattern.
The default parameter change in store-dir-config.hh is correct, but three related addToStoreFromDump method declarations still use the old FileIngestionMethod::NixArchive as defaults instead of the explicit ContentAddressMethod::Raw::NixArchive. While these work due to implicit conversion via MAKE_WRAPPER_CONSTRUCTOR, they should be updated for consistency:
- src/libstore/dummy-store.cc line 87
- src/libstore/include/nix/store/legacy-ssh-store.hh line 116
- src/libstore/include/nix/store/remote-store.hh line 89
Update all three to use ContentAddressMethod::Raw::NixArchive.
🤖 Prompt for AI Agents
In src/libstore/dummy-store.cc around line 87,
src/libstore/include/nix/store/legacy-ssh-store.hh around line 116, and
src/libstore/include/nix/store/remote-store.hh around line 89, update the
addToStoreFromDump method declarations so their default parameter uses
ContentAddressMethod::Raw::NixArchive instead of the old
FileIngestionMethod::NixArchive; change the default value in each declaration to
ContentAddressMethod::Raw::NixArchive to match the refactored
store-dir-config.hh and ensure consistency with the new enum wrapper.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/libcmd/installables.cc (1)
303-385: Re-enable fragment completion logic.The completion implementation is stubbed out with
#if 0, breakingnix <cmd> <flake>#<fragment>completions. Therolesparameter is also unused.This issue was previously flagged with a detailed fix proposal. Please address the past review comment that provides a reimplementation approach.
🧹 Nitpick comments (2)
src/libcmd/installables.cc (1)
428-458: Track removal of legacy openEvalCache.The
FIXME: removecomment indicates this is transitional code. Consider opening a tracking issue for its removal once all call sites migrate to the new schema-based approach.Would you like me to search the codebase for remaining call sites to assess when this can be safely removed?
src/libflake/flake.cc (1)
897-905: Consider eliminating redundant computation.The calculation of
useRegistriesanduseRegistriesTopat lines 900-901 is duplicated in the mainlockFlakeimplementation at lines 383-385. While this doesn't affect correctness, it introduces minor redundancy.Consider refactoring to avoid duplicate computation:
LockedFlake lockFlake(const Settings & settings, EvalState & state, const FlakeRef & topRef, const LockFlags & lockFlags) { - auto useRegistries = lockFlags.useRegistries.value_or(settings.useRegistries); - auto useRegistriesTop = useRegistries ? fetchers::UseRegistries::All : fetchers::UseRegistries::No; - return lockFlake( - settings, state, topRef, lockFlags, getFlake(state, topRef, useRegistriesTop, {}, lockFlags.requireLockable)); + settings, state, topRef, lockFlags, + getFlake(state, topRef, + (lockFlags.useRegistries.value_or(settings.useRegistries) ? fetchers::UseRegistries::All : fetchers::UseRegistries::No), + {}, lockFlags.requireLockable)); }Alternatively, calculate once and pass through, though the current pattern is acceptable given the minor cost.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
doc/manual/source/SUMMARY.md.in(1 hunks)src/libcmd/installable-flake.cc(5 hunks)src/libcmd/installables.cc(7 hunks)src/libcmd/meson.build(2 hunks)src/libexpr/eval-cache.cc(8 hunks)src/libflake/flake.cc(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- doc/manual/source/SUMMARY.md.in
🧰 Additional context used
🧬 Code graph analysis (4)
src/libcmd/installables.cc (2)
src/libcmd/include/nix/cmd/command.hh (3)
completeFlakeRefWithFragment(390-395)completions(166-166)prefix(315-315)src/libcmd/flake-schemas.cc (2)
getDefaultFlakeSchemas(297-303)getDefaultFlakeSchemas(297-297)
src/libexpr/eval-cache.cc (2)
src/libexpr/include/nix/expr/parser-state.hh (3)
attrs(93-94)attrs(95-95)attrPath(91-91)src/libexpr/include/nix/expr/eval-cache.hh (10)
attrPath(158-158)name(136-136)name(138-138)name(142-142)name(144-144)name(146-146)name(148-148)name(150-150)name(152-152)toAttrPathStr(18-18)
src/libflake/flake.cc (2)
src/libflake/include/nix/flake/flake.hh (4)
settings(68-68)lockFlake(245-245)lockFlake(247-248)getFlake(124-125)src/nix/flake.cc (2)
lockFlake(58-61)lockFlake(58-58)
src/libcmd/installable-flake.cc (3)
src/libcmd/flake-schemas.cc (7)
state(250-250)call(39-122)call(40-40)getSchema(248-279)getSchema(248-248)getOutput(213-246)getOutput(213-213)src/libexpr/eval-cache.cc (2)
toAttrPathStr(398-401)toAttrPathStr(398-398)src/libcmd/installables.cc (2)
openEvalCache(429-458)openEvalCache(429-429)
⏰ 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 (21)
src/libcmd/meson.build (3)
79-79: flake-schemas.cc exists and is correctly added to sources.The file has been verified to exist at
src/libcmd/flake-schemas.cc. The addition to the meson.build sources list is valid.
92-95: Both schema files exist — code is correct.Verified that
call-flake-schemas.nixandbuiltin-flake-schemas.nixare present insrc/libcmd/. The header generation pattern is correctly configured.
71-71: All referenced files and directories exist; no issues found.Verification confirms:
- ✓ generate-header subproject directory exists
- ✓ flake-schemas.cc source file exists
- ✓ call-flake-schemas.nix and builtin-flake-schemas.nix files exist
- ✓ Proper ordering: subdir() on line 71 is defined before gen_header.process() usage on lines 93-94
src/libcmd/installables.cc (3)
236-239: LGTM! Role-based default is clean.The new
getRoles()method clearly replaces the old path-based defaults with a role-based approach, aligning with the schema-driven design.
292-292: LGTM! Updated to pass roles.The call correctly uses
getRoles()to provide role-based context for completions.
520-532: LGTM! Constructor call updated correctly.The
InstallableFlakeinstantiation properly passes the new role-based parameters (getRoles(),lockFlags,getDefaultFlakeSchemas()), aligning with the updated constructor signature.src/libcmd/installable-flake.cc (5)
20-20: LGTM! Required header added.The flake-schemas header is necessary for the new schema-driven resolution logic.
29-40: LGTM! Updated to use AttrPath.The signature correctly uses
eval_cache::AttrPathand leverages the newtoAttrPathStrhelper for path formatting.
42-61: LGTM! Constructor signature updated for role-based schema.The constructor properly accepts
rolesanddefaultFlakeSchemas, replacing the old path-based approach. Fields are correctly initialized.
145-244: Schema-driven resolution implemented correctly, with noted hacks.The refactored
getCursorsproperly integrates with the flake-schemas subsystem:
- Uses
flake_schemas::callto get inventory and outputs- Correctly filters schemas by roles
- Handles both absolute (
.-prefixed) and schema-based fragments- Provides suggestions on attribute lookup failure
The implementation includes two acknowledged compatibility hacks (lines 163-180 for schema precedence, lines 212-215 for nix-repl), which are reasonable for maintaining backward compatibility during the transition.
Note: The schema precedence ordering preserves historical behavior but may benefit from a more declarative approach in future schema definitions, as the FIXME suggests.
258-264: LGTM! Lazy evaluation cache initialization.The
openEvalCache()method properly implements lazy initialization with memoization, callingflake_schemas::callonly once per instance.src/libexpr/eval-cache.cc (8)
122-137: LGTM! Updated to use AttrPath.The
setAttrssignature correctly adopts the newAttrPathtype, maintaining the same logic for attribute storage.
257-262: LGTM! Local variable updated to AttrPath.The type change is consistent with the broader refactor.
310-318: LGTM! Testing utility for cache completeness.The
NIX_ALLOW_EVALcheck is useful for verifying that evaluation caching is complete during testing. The default value of "1" ensures no behavior change in production.
371-396: LGTM! Raw and cleaned path variants.The split between
getAttrPathRaw()andgetAttrPath()properly separates raw traversal paths from cleaned/transformed paths. This supports the schema-driven design where inventory paths need post-processing (viacleanupAttrPath) to derive output paths.
398-411: LGTM! Extracted path formatting utility.The
toAttrPathStrhelper provides consistent path-to-string conversion, properly handling symbol resolution and formatting. This is correctly reused by thegetAttrPathStrmethods.
463-467: LGTM! Variant type updated to AttrPath.The cached value variant correctly uses
AttrPathinstead ofstd::vector<Symbol>.
533-545: LGTM! Signature updated to use AttrPath.The
findAlongAttrPathmethod correctly acceptsconst AttrPath &instead of the old vector type. The implementation logic remains sound.
687-717: LGTM! getAttrs updated to return AttrPath.The method correctly returns
AttrPathand uses it consistently for both cached and fresh attribute retrieval. The sorting logic is preserved.src/libflake/flake.cc (2)
380-381: LGTM: Valid API extension with backward compatibility.The addition of a
Flakeparameter enables callers to pass precomputed flake objects, reducing redundant fetching. The original signature is preserved via the overload at lines 897-905, maintaining backward compatibility.
207-214: Verify that exposingreadFlake(removingstatic) is intentional.Removing
staticfrom the file-scope function changes its linkage from internal (translation-unit-local) to external (visible across translation units). This makesreadFlakepart of the library's internal API. Confirm this visibility change is intentional and that there are no conflicts (e.g., the function should be declared in a header if other translation units will reference it).
Motivation
Updated version of NixOS#8892.
Context
Summary by CodeRabbit
New Features
Documentation