Skip to content

Flake schemas#217

Merged
edolstra merged 68 commits intomainfrom
flake-schemas-detsys
Feb 27, 2026
Merged

Flake schemas#217
edolstra merged 68 commits intomainfrom
flake-schemas-detsys

Conversation

@edolstra
Copy link
Collaborator

@edolstra edolstra commented Oct 1, 2025

Motivation

Updated version of NixOS#8892.

Context

Summary by CodeRabbit

  • New Features

    • Added Flake Schemas to describe and validate flake outputs.
    • Schema-driven behavior for nix flake check and enhanced nix flake show.
    • New CLI options: default-flake-schemas, build-all, and --output-paths / --drv-paths for flake show.
  • Documentation

    • New comprehensive Flake Schemas documentation and table-of-contents entry describing schema format and usage.

@coderabbitai
Copy link

coderabbitai bot commented Oct 1, 2025

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds 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

Cohort / File(s) Summary
Documentation
doc/manual/source/SUMMARY.md.in, doc/manual/source/protocols/flake-schemas.md
New TOC entry and full documentation for Flake Schemas: schema format, node types, inventory semantics, and usage examples.
Nix builtin schemas & generators
src/libcmd/builtin-flake-schemas.nix, src/libcmd/call-flake-schemas.nix
Added builtin schema collection and call-time schema merging/assembly used to build per-flake inventories.
Runtime implementation
src/libcmd/flake-schemas.cc, src/libcmd/include/nix/cmd/flake-schemas.hh
New C++ flake_schemas subsystem: mounting builtin schemas, creating EvalCache, inventory traversal APIs, output resolution, and MixFlakeSchemas CLI integration.
Build & packaging
src/libcmd/meson.build, src/libcmd/include/nix/cmd/meson.build, src/libcmd/package.nix
Included new sources and header-generation steps; exported flake-schemas.hh and added generated Nix modules to package.
Eval-cache / AttrPath API
src/libexpr/eval-cache.cc, src/libexpr/include/nix/expr/eval-cache.hh
Introduced AttrPath-based APIs and helpers (getAttrPathRaw/getAttrPath, toAttrPathStr), made AttrCursor path accessors public, and updated AttrDb to accept AttrPath.
Command API surface and InstallableFlake
src/libcmd/include/nix/cmd/command.hh, src/libcmd/include/nix/cmd/installable-flake.hh, src/libcmd/installable-flake.cc, src/libcmd/installables.cc
Replaced default attr-paths model with role-based getRoles(); added MixFlakeSchemas; refactored InstallableFlake to use fragment+roles, cache EvalCache via flake_schemas, and perform schema-driven cursor resolution.
Flake & lock plumbing
src/libflake/flake.cc, src/libflake/include/nix/flake/flake.hh
Made readFlake public and added lockFlake overload accepting a precomputed Flake to support new evaluation flows.
Command implementations
src/nix/flake.cc, src/nix/flake-check.md, src/nix/flake-show*, src/nix/* (bundle, develop, formatter, repl, run, profile, search)
Reworked flake show/check to use schema-driven inventory traversal; updated many commands to implement getRoles(); adjusted InstallableFlake call sites and flags (MixFlakeSchemas) and added related CLI flags.
Tests
tests/functional/* (flakes/*, completions.sh, chroot-store.sh, formatter.sh, ...)
Updated tests to expect inventory/schema-driven outputs, added failing-check scenarios, adjusted assertions, and temporarily disabled/skipped some checks.
CI
.github/workflows/build.yml
Adjusted checkout sources for regressions and added USE_NIX_FLAKE_SHOW env flag.

Sequence Diagram

sequenceDiagram
    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
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

Suggested reviewers

  • cole-h
  • grahamc

Poem

🐰🌱 I hopped through flakes to build a map,
Nodes and leaves in my little nap trap,
Inventories laid out neat and clear,
Derivations hum — the schema's here! ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 3.64% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Flake schemas' accurately summarizes the main change in this comprehensive pull request that introduces flake schemas support throughout the Nix codebase.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch flake-schemas-detsys

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link

github-actions bot commented Oct 1, 2025

@github-actions github-actions bot temporarily deployed to pull request October 1, 2025 12:03 Inactive
@github-actions github-actions bot temporarily deployed to pull request October 20, 2025 17:19 Inactive
@github-actions github-actions bot temporarily deployed to pull request October 20, 2025 18:00 Inactive
@github-actions github-actions bot temporarily deployed to pull request October 20, 2025 18:16 Inactive
@edolstra edolstra marked this pull request as ready for review October 21, 2025 13:44
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 for AttrPath.

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 stabilized

If 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 binding mapAttrsToList.

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.try so 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) with grep -E for broader availability.
  • Prefer clearStoreIfPossible to 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->attrPath is 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 linkage

readFlake 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 overrides

The 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 duplication

This 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 readability

Initializer 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 schemas

Message 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 rendering

Currently, 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: Prefer using over typedef for clarity

Style 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

📥 Commits

Reviewing files that changed from the base of the PR and between dd7140a and e5797f8.

📒 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 cached openEvalCache() accessor look good.

Consider a brief note on thread-safety of _evalCache if 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, and defaultFlakeSchemas.
  • lockFlags lifetime is safe: it's stored by const reference from a CmdFlakeInitCommon member variable that outlives the temporary InstallableFlake object.
  • 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.nix and src/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. The gen_header.process() calls at lines 92–93 of src/libcmd/meson.build generate the .gen.hh headers from both .nix sources, and the fileset in src/libcmd/package.nix already includes:

  • The .nix sources (lines 52–53, your PR addition)
  • The include/nix/cmd/meson.build subdirectory (line 49)
  • All .hh headers 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() to getRoles() 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 writeIfdFlake to wrap the IFD import in a proper derivation structure aligns well with the schema-driven approach. The derivation name "top" matches the test expectations in show.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-derivation and trace-import-from-derivation options (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 evalChecks attributes with error reporting
  • Conditional building of derivations based on isFlakeCheck

This 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 .filtered attribute
  • Derivation name extraction via .derivationName
  • Legacy packages skip behavior via .skipped

29-30: LGTM!

The --all-systems test correctly verifies that packages for all systems are shown with proper derivation names, while legacy packages remain skipped by default.


39-39: LGTM!

The --legacy flag 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 .failed and .derivationName attributes.


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 the packages schema also carries the nix-run role—the role declaration is semantically misaligned. When a user runs nix bundle <flake> without an explicit fragment, the role "nix-run" filters to schemas with that role; both packages and apps match, but declaring "nix-run" obscures the bundling intent. Consider confirming whether this is intentional or if getRoles() 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 correct

Switch 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 factoring

Accepting 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 correct

The 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 CmdFlakeCheck

Adopting MixFlakeSchemas and deferring to flake_schemas::call is the right direction.


483-504: Template completion uses roles correctly

Using {"nix-template"} for template completion aligns with the new roles model.


761-779: CmdFlakeShow: role/schema integration LGTM

MixFlakeSchemas 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 correct

Uses eval_cache::toAttrPathStr(state, ...) and clear separators; no issues spotted.


48-58: Constructor API change verified: roles/defaultFlakeSchemas wiring confirmed

Verification complete. Found one call site at src/nix/flake.cc:513-521, which correctly passes all 8 parameters to the InstallableFlake constructor:

  • roles is non-empty: {"nix-template"}
  • defaultFlakeSchemas is 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 merging

The concern is valid. The codebase has multiple cache implementations:

  • The method under review creates a schema-aware cache (using flake_schemas::call() with fingerprint2)
  • 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 cleanupAttrPath behavior. 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 consistent

DB 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 EvalCache instances—whether created in installables.cc or flake-schemas.cc—automatically receive this safe default. The empty constructor in eval-cache.cc (lines 298–304) does not override this initialization. The call to root->cleanupAttrPath() in eval-cache.cc (lines 383, 395) will always invoke a valid function, eliminating any risk of std::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=0

The 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 to 0 before triggering getRoot() (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, where include_dirs points to src/libexpr/include/.

libcmd correctly declares nix-expr as a public dependency: In libcmd's meson.build, dependency('nix-expr') is listed in deps_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.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) and std::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, system is std::optional<std::string> and must be dereferenced when passed to std::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

📥 Commits

Reviewing files that changed from the base of the PR and between e5797f8 and 46aa449.

📒 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 MemorySourceAccessor with 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::span for efficient path traversal without copying (line 228)
  • Walks through nested children to resolve the target node (lines 230-239)
  • Returns std::nullopt when the path doesn't exist
  • Constructs OutputInfo with 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 Schemas map

285-303: LGTM: CLI integration is standard and correct.

The MixFlakeSchemas class correctly:

  • Registers the --default-flake-schemas flag with flake ref completion (lines 287-294)
  • Parses the flag value into a FlakeRef with proper base directory resolution (line 302)

@github-actions github-actions bot temporarily deployed to pull request October 21, 2025 21:38 Inactive
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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, breaking nix <cmd> <flake>#<fragment> completions. The roles parameter 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: remove comment 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 useRegistries and useRegistriesTop at lines 900-901 is duplicated in the main lockFlake implementation 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

📥 Commits

Reviewing files that changed from the base of the PR and between 46aa449 and fb58ac4.

📒 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.nix and builtin-flake-schemas.nix are present in src/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 InstallableFlake instantiation 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::AttrPath and leverages the new toAttrPathStr helper for path formatting.


42-61: LGTM! Constructor signature updated for role-based schema.

The constructor properly accepts roles and defaultFlakeSchemas, replacing the old path-based approach. Fields are correctly initialized.


145-244: Schema-driven resolution implemented correctly, with noted hacks.

The refactored getCursors properly integrates with the flake-schemas subsystem:

  • Uses flake_schemas::call to 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, calling flake_schemas::call only once per instance.

src/libexpr/eval-cache.cc (8)

122-137: LGTM! Updated to use AttrPath.

The setAttrs signature correctly adopts the new AttrPath type, 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_EVAL check 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() and getAttrPath() properly separates raw traversal paths from cleaned/transformed paths. This supports the schema-driven design where inventory paths need post-processing (via cleanupAttrPath) to derive output paths.


398-411: LGTM! Extracted path formatting utility.

The toAttrPathStr helper provides consistent path-to-string conversion, properly handling symbol resolution and formatting. This is correctly reused by the getAttrPathStr methods.


463-467: LGTM! Variant type updated to AttrPath.

The cached value variant correctly uses AttrPath instead of std::vector<Symbol>.


533-545: LGTM! Signature updated to use AttrPath.

The findAlongAttrPath method correctly accepts const AttrPath & instead of the old vector type. The implementation logic remains sound.


687-717: LGTM! getAttrs updated to return AttrPath.

The method correctly returns AttrPath and 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 Flake parameter 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 exposing readFlake (removing static) is intentional.

Removing static from the file-scope function changes its linkage from internal (translation-unit-local) to external (visible across translation units). This makes readFlake part 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).

@github-actions github-actions bot temporarily deployed to pull request October 23, 2025 12:10 Inactive
@github-actions github-actions bot temporarily deployed to pull request November 5, 2025 17:28 Inactive
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between fb58ac4 and adf137a.

📒 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 MixFlakeSchemas provides 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::visit with 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 MixFlakeSchemas is consistent with the schema-driven refactor and enables the template fragment handling below.


759-759: LGTM!

Adding MixFlakeSchemas is 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)
  • lockFlagsconst 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.

@github-actions github-actions bot temporarily deployed to pull request February 25, 2026 21:12 Inactive
…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.
@github-actions github-actions bot temporarily deployed to pull request February 25, 2026 22:22 Inactive
@github-actions github-actions bot temporarily deployed to pull request February 26, 2026 21:13 Inactive
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.
@github-actions github-actions bot temporarily deployed to pull request February 26, 2026 22:16 Inactive
@github-actions github-actions bot temporarily deployed to pull request February 26, 2026 22:45 Inactive
Copy link
Member

@cole-h cole-h left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@github-actions github-actions bot temporarily deployed to pull request February 27, 2026 10:25 Inactive
`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.
@github-actions github-actions bot temporarily deployed to pull request February 27, 2026 12:21 Inactive
@github-actions github-actions bot temporarily deployed to pull request February 27, 2026 15:10 Inactive
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).
@edolstra edolstra enabled auto-merge February 27, 2026 15:37
@github-actions github-actions bot temporarily deployed to pull request February 27, 2026 15:46 Inactive
@edolstra edolstra added this pull request to the merge queue Feb 27, 2026
Merged via the queue into main with commit ba48af2 Feb 27, 2026
134 checks passed
@edolstra edolstra deleted the flake-schemas-detsys branch February 27, 2026 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

flake-regression-test Run the flake regressions test suite on this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants