Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Flake Schemas subsystem: Nix-level builtin schemas, flake-driven schema extraction, C++ runtime (evaluation cache integration, inventory traversal, APIs), command-layer integration (InstallableFlake, SourceExprCommand, flake commands), and eval-cache AttrPath API updates; plus docs, tests, and build adjustments. Changes
Sequence DiagramsequenceDiagram
participant User as User/CLI
participant Installable as InstallableFlake
participant Flake as LockedFlake
participant FlakeSchemas as nix::flake_schemas
participant EvalCache as eval_cache::EvalCache
participant Inventory as Inventory (AttrCursor)
User->>Installable: construct(fragment, roles, defaultSchemasFlake?)
Installable->>Flake: lockFlake(...)
Installable->>FlakeSchemas: call(state, Flake, defaultSchemasFlake)
FlakeSchemas->>EvalCache: build merged EvalCache (builtin + flake + call-flake-schemas)
EvalCache-->>FlakeSchemas: evaluated cache / inventory root
FlakeSchemas->>Inventory: expose inventory cursor
Installable->>Inventory: forEachOutput / visit (filter by roles/system)
Inventory-->>Installable: leaf/non-leaf nodes, derivation info, paths
Installable->>User: resolved installable cursors / attr paths
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
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.
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)
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).
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/nix/flake.cc (1)
833-833: Add bounds checking before accessing attrPath[0].The code accesses
node->getAttrPath()[0]without verifying the attrPath is non-empty. While this may work in practice due to how nodes are constructed, defensive bounds checking would prevent potential out-of-bounds access.Apply this diff:
- if (node->root->state.symbols[node->getAttrPath()[0]] == "legacyPackages") + auto & attrPath = node->getAttrPath(); + if (!attrPath.empty() && node->root->state.symbols[attrPath[0]] == "legacyPackages")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
doc/manual/source/SUMMARY.md.in(1 hunks)src/nix/flake.cc(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/nix/flake.cc (2)
src/libcmd/flake-schemas.cc (14)
call(39-122)call(40-40)getDefaultFlakeSchemas(297-303)getDefaultFlakeSchemas(297-297)visit(148-188)visit(148-153)derivation(208-211)derivation(208-208)forEachOutput(124-146)forEachOutput(124-126)what(190-196)what(190-190)shortDescription(198-206)shortDescription(198-198)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)
⏰ 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 (9)
doc/manual/source/SUMMARY.md.in (1)
119-119: LGTM! Well-placed documentation entry.The Flake Schemas entry is correctly formatted, properly indented, and logically placed within the "Formats and Protocols" section.
src/nix/flake.cc (8)
21-21: LGTM!The include is necessary for the flake_schemas API used throughout this file.
303-303: LGTM!Adding
MixFlakeSchemasprovides the schema configuration interface needed for the refactored implementation.
375-416: LGTM - Schema-driven traversal implementation is well-structured.The visit function correctly uses
flake_schemas::visitwith three callbacks for leaves, non-leaves, and filtered nodes. The parallel evaluation with futures, error handling, and derivation path collection are all implemented appropriately.
418-428: LGTM - forEachOutput integration is correct.The code properly handles both known outputs (with schema information) and unknown outputs, tracking unchecked outputs for later warning.
481-481: LGTM!Adding
MixFlakeSchemasis consistent with the schema-driven refactor and enables the template fragment handling below.
759-759: LGTM!Adding
MixFlakeSchemasis consistent with the schema-driven refactor implemented in the run() method below.
802-846: LGTM - Schema-driven JSON construction is well-implemented.The visit function correctly populates the JSON structure with leaf attributes (what, shortDescription, derivationName) and hierarchical children, with proper error handling for legacyPackages.
511-520: No issues found. Constructor parameters are correctly specified.The InstallableFlake constructor signature confirms all three parameters in the call are correct:
{"nix-template"}→StringSet roles(parameter 6)lockFlags→const flake::LockFlags & lockFlags(parameter 7){}→std::optional<FlakeRef> defaultFlakeSchemas(parameter 8)The empty braces
{}is valid initialization of the optional parameter representing no default schema.
…tions The `packages` output was always intended to be very strict, unlike `legacyPackages`. So this test was misguided. Of course, a user could provide an alternative flake schema for `packages` if they really want to.
E.g. running `nix run` on the empty flake
{
outputs = { self }: { };
}
now prints
error: flake 'path:/home/eelco/Dev/nix/flake' does not provide attribute 'apps.x86_64-linux.default' or 'packages.x86_64-linux.default'
OTOH, if there are no schemas, as in
{
outputs = { self }: { schemas = { }; };
}
you get
error: Flake 'path:/home/eelco/Dev/nix/flake' does not have any schema that provides a default output for the role(s) nix-run.
cole-h
left a comment
There was a problem hiding this comment.
LGTM (aside from the CI failure in flake regressions which is probably because we now handle unknown outputs lol) and works as I expected in my testing.
`derivation` was fishy because it allows the schema to transform the derivation, or to construct an entirely new derivation. That might be useful sometimes, but flake schemas should describe what's in the output, not add entirely new stuff.
We don't show derivation names/versions by default because computing the name attribute is potentially very expensive (e.g. for a NixOS configuration, it can take several seconds).
Motivation
Updated version of NixOS#8892.
Context
Summary by CodeRabbit
New Features
nix flake checkand enhancednix flake show.--output-paths/--drv-pathsfor flake show.Documentation