Conversation
This makes `nix flake check` skip `legacyPackages` again.
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds an explicit isLegacy attribute to schema nodes and propagates legacy-aware traversal: the visitor API gains includeLegacy and visitLegacy, CLI gains a --legacy flag to control inclusion, rendering and tests updated to expose and handle legacy nodes. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as "flake show/check"
participant Visitor as "flake_schemas::visit"
participant Schema as "Schema Node"
participant Renderer as "Output Renderer"
User->>CLI: run command (with or without --legacy)
CLI->>Visitor: visit(system, includeLegacy, ...)
Visitor->>Schema: evaluate node attrs (includes isLegacy)
alt Schema.isLegacy == true and includeLegacy == false
Visitor->>Renderer: visitLegacy(node)
Renderer->>CLI: mark/display legacy-omitted info
else
Visitor->>Renderer: visitLeaf / visitNonLeaf
Renderer->>CLI: include in normal output
end
CLI-->>User: display JSON/tree (with isLegacy annotations)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
|
I wonder if we should make this a more futuristic flag, like "--large" or "--deep" or similar, to give a more meaningful name to the flag in a way that makes sense for use cases where folks will inevitably use the flag. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/nix/flake.cc (1)
383-386: Address the FIXME: Consider adding--legacyflag tonix flake check.The hardcoded
falsewith FIXME suggests this was intentionally deferred. For consistency withnix flake show, consider whethernix flake checkshould also support a--legacyflag to allow checking legacy packages when explicitly requested.Would you like me to open an issue to track adding
--legacysupport tonix flake check?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nix/flake.cc` around lines 383 - 386, The call to flake_schemas::visit is passing a hardcoded false for the legacy parameter (see flake_schemas::visit, checkAllSystems, localSystem) and should respect a new --legacy option for "nix flake check"; add a --legacy boolean option to the command's argument parsing/flags, store it in a variable (e.g. legacyFlag), and pass that variable instead of false to flake_schemas::visit so legacy package checking is enabled when the user supplies --legacy; update the command help text and any related option parsing code where checkAllSystems and localSystem are handled to include this new flag.src/libcmd/flake-schemas.cc (1)
198-203: Address the FIXME: useisLegacyattribute for error suppression.The error handling still uses a hardcoded
"legacyPackages"string check to decide whether to suppress errors. SinceisLegacyis now available in the schema, this should be refactored to check theisLegacyattribute instead, making the behavior consistent and extensible to other potential legacy outputs.Would you like me to help implement this refactor to use the
isLegacyattribute for error suppression?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libcmd/flake-schemas.cc` around lines 198 - 203, Replace the hardcoded "legacyPackages" string check with a check of the symbol's isLegacy attribute: fetch the symbol via node->root->state.symbols[node->getAttrPath()[0]] and inspect its isLegacy flag (rather than comparing the symbol name to "legacyPackages"); if isLegacy is true, suppress the error, otherwise call e.addTrace(..., node->getAttrPathStr()) and rethrow; update the conditional around e.addTrace/throw to use symbol.isLegacy (or equivalent accessor) so other legacy outputs are handled consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/libcmd/flake-schemas.cc`:
- Around line 198-203: Replace the hardcoded "legacyPackages" string check with
a check of the symbol's isLegacy attribute: fetch the symbol via
node->root->state.symbols[node->getAttrPath()[0]] and inspect its isLegacy flag
(rather than comparing the symbol name to "legacyPackages"); if isLegacy is
true, suppress the error, otherwise call e.addTrace(..., node->getAttrPathStr())
and rethrow; update the conditional around e.addTrace/throw to use
symbol.isLegacy (or equivalent accessor) so other legacy outputs are handled
consistently.
In `@src/nix/flake.cc`:
- Around line 383-386: The call to flake_schemas::visit is passing a hardcoded
false for the legacy parameter (see flake_schemas::visit, checkAllSystems,
localSystem) and should respect a new --legacy option for "nix flake check"; add
a --legacy boolean option to the command's argument parsing/flags, store it in a
variable (e.g. legacyFlag), and pass that variable instead of false to
flake_schemas::visit so legacy package checking is enabled when the user
supplies --legacy; update the command help text and any related option parsing
code where checkAllSystems and localSystem are handled to include this new flag.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d931a827-e62f-414d-8982-7fe10db24768
📒 Files selected for processing (6)
doc/manual/source/protocols/flake-schemas.mdsrc/libcmd/builtin-flake-schemas.nixsrc/libcmd/flake-schemas.ccsrc/libcmd/include/nix/cmd/flake-schemas.hhsrc/nix/flake.cctests/functional/flakes/show.sh
"Ignored" suggests that the command would succeed if it wasn't for the error that finishAll() propagates, but that's not the case.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/libexpr/parallel-eval.cc`:
- Around line 173-175: The current branch inside parallel-eval.cc logs secondary
exceptions with the normal error prefix even though `ex` already holds the
primary exception that `finishAll()` will rethrow; update the handling in the
block that checks `if (ex) { if (!getInterrupted())
logExceptionExceptInterrupt(); }` to mark these later exceptions as
suppressed/ignored in their log output (e.g., use a suppressed/ignored prefix or
a different logging level/message) so callers can distinguish them from the
primary exception that `finishAll()` rethrows; locate and change the call site
using the symbols `ex`, `getInterrupted()`, and `logExceptionExceptInterrupt()`
to emit a suppressed/ignored-prefixed message for these secondary failures.
In `@src/libutil/util.cc`:
- Around line 240-254: The prefix passed from ignoreExceptionExceptInterrupt
lacks a trailing space so messages are concatenated; update
logExceptionExceptInterrupt (or the print invocation it uses) to ensure a
separating space between the prefix and exception text—e.g., change the format
used in printMsg inside logExceptionExceptInterrupt (for the Error and
std::exception handlers) to include a space between the two %s segments or
ensure the prefix string includes a trailing space so output becomes "error
(ignored): <msg>" instead of "error (ignored):<msg>".
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: af971748-81c1-4a75-a406-2c2533664339
📒 Files selected for processing (3)
src/libexpr/parallel-eval.ccsrc/libutil/include/nix/util/util.hhsrc/libutil/util.cc
edolstra
added
the
flake-regression-test
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/nix/flake.cc (1)
947-957:⚠️ Potential issue | 🟠 MajorStop keying legacy error handling off
legacyPackages.The new visitor path is generic, but the
EvalErrorfallback still only suppresses failures when the top-level attr is literallylegacyPackages. If another schema node is markedisLegacy,nix flake show --legacywill still abort there instead of surfacing"failed": true. Please drive this from the legacy state you already get fromflake_schemas::visit, not from the output name.Also applies to: 964-968
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nix/flake.cc` around lines 947 - 957, The EvalError catch is keyed to the literal output name "legacyPackages" instead of the schema's legacy flag; change the conditional to use the legacy state supplied by flake_schemas::visit (the info available on the AttrCursor/visit context) rather than comparing node->root->state.symbols[...] to "legacyPackages" so that any node marked isLegacy produces j.emplace("failed", true); apply the same change in the second occurrence (lines ~964-968) so both error paths consult the visit-provided legacy marker on the node/visit context.
🧹 Nitpick comments (2)
src/nix/flake.cc (1)
383-386: Resolve theflake checklegacy toggle before merge.Line 385 hard-codes
includeLegacy = false, sonix flake checkcan never validateisLegacyoutputs even though the visitor API now supports it. Either wire a flag through here or drop the FIXME if skipping is meant to be unconditional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nix/flake.cc` around lines 383 - 386, The call to flake_schemas::visit hard-codes the includeLegacy parameter to false, preventing `nix flake check` from validating legacy outputs; update the call site to pass a real includeLegacy boolean instead of false (e.g. add or propagate an existing CLI/config flag into this function call and use that variable here: flake_schemas::visit(..., includeLegacy, node)), or if skipping legacy checks is intended, remove the FIXME and leave a clear comment explaining the unconditional behavior; refer to symbols flake_schemas::visit, checkAllSystems, localSystem and the includeLegacy parameter when making the change.tests/functional/flakes/show.sh (1)
14-33: Assert omission, not just annotation.These checks only prove that the current-system node gets tagged
isLegacy; they no longer prove that the legacy subtree stayed hidden without--legacy. A regression that still exposes legacy children while also settingisLegacy = truewould now pass. Please add a negative assertion for the missing child set in both cases, and in the--all-systemscase consider covering at least one non-local branch too.Possible assertion to add
assert show_output.inventory.legacyPackages.output.children.${builtins.currentSystem}.isLegacy; +assert !(show_output.inventory.legacyPackages.output.children.${builtins.currentSystem} ? children);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/functional/flakes/show.sh` around lines 14 - 33, Add negative assertions to prove legacy children are absent when they should be: in the first (no --all-systems) block add an assertion that show_output.inventory.legacyPackages.output.children.someOtherSystem (or the equivalent non-local child) is missing/not present in the JSON in addition to the existing isLegacy check; in the second (--all-systems) block add an assertion that at least one non-local branch (e.g. show_output.inventory.packages.output.children.someOtherSystem.children.default or another non-local children node) is present to cover cross-system visibility while still asserting legacyPackages children reflect the expected state; reference the existing show_output variable and the inventory.packages / inventory.legacyPackages paths when adding these assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/nix/flake.cc`:
- Around line 947-957: The EvalError catch is keyed to the literal output name
"legacyPackages" instead of the schema's legacy flag; change the conditional to
use the legacy state supplied by flake_schemas::visit (the info available on the
AttrCursor/visit context) rather than comparing node->root->state.symbols[...]
to "legacyPackages" so that any node marked isLegacy produces
j.emplace("failed", true); apply the same change in the second occurrence (lines
~964-968) so both error paths consult the visit-provided legacy marker on the
node/visit context.
---
Nitpick comments:
In `@src/nix/flake.cc`:
- Around line 383-386: The call to flake_schemas::visit hard-codes the
includeLegacy parameter to false, preventing `nix flake check` from validating
legacy outputs; update the call site to pass a real includeLegacy boolean
instead of false (e.g. add or propagate an existing CLI/config flag into this
function call and use that variable here: flake_schemas::visit(...,
includeLegacy, node)), or if skipping legacy checks is intended, remove the
FIXME and leave a clear comment explaining the unconditional behavior; refer to
symbols flake_schemas::visit, checkAllSystems, localSystem and the includeLegacy
parameter when making the change.
In `@tests/functional/flakes/show.sh`:
- Around line 14-33: Add negative assertions to prove legacy children are absent
when they should be: in the first (no --all-systems) block add an assertion that
show_output.inventory.legacyPackages.output.children.someOtherSystem (or the
equivalent non-local child) is missing/not present in the JSON in addition to
the existing isLegacy check; in the second (--all-systems) block add an
assertion that at least one non-local branch (e.g.
show_output.inventory.packages.output.children.someOtherSystem.children.default
or another non-local children node) is present to cover cross-system visibility
while still asserting legacyPackages children reflect the expected state;
reference the existing show_output variable and the inventory.packages /
inventory.legacyPackages paths when adding these assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d5a75708-c19b-4d4d-9264-9e7dd204bd5b
📒 Files selected for processing (2)
src/nix/flake.cctests/functional/flakes/show.sh
2 participants
Motivation
This makes
nix flake checkskiplegacyPackagesagain.Context
Summary by CodeRabbit
Documentation
New Features
Bug Fixes
Tests
Chores