nix flake check: Show which outputs failed or succeeded

[edolstra]

Motivation

Similar to #281, this makes nix flake check print at the end which flake outputs failed or succeeded to build.

Cherry-picked from #217.

Also fixes a potential crash in nix flake check when parallel eval is enabled.

Context

Summary by CodeRabbit

• Refactor

  • Internal improvements to expression evaluation caching and attribute-path formatting for clearer diagnostics.

• Bug Fixes

  • Improved flake-check build reporting: more precise per-item success/failure notifications and more robust error handling with staged exit behavior.

• Breaking Change

  • A small change to derived-path equality behavior in the public API may affect consumers relying on the previous equality operator.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

The pull request refactors attribute path formatting and flake check logic. A new helper function toAttrPathStr() centralizes attribute path stringification across the eval cache. The flake check command is refactored to use thread-safe synchronized data structures for tracking derived paths and their attribute path mappings, with revised error handling and per-attribute-path build notifications.

Changes

Cohort / File(s)	Summary
Attribute path formatting helper src/libexpr/include/nix/expr/eval-cache.hh, src/libexpr/eval-cache.cc	Added toAttrPathStr() helper function to format attribute paths and type alias AttrPath = std::vector<Symbol>. Refactored existing methods to use the new helper.
Flake check refactoring src/nix/flake.cc	Replaced plain vector with synchronized data structures (drvPaths_, omittedSystems, derivedPathToAttrPaths_) for thread-safe tracking of derived paths and their attribute paths. Refactored missing/build steps to use locked access patterns. Changed error handling from throwing Error to staged error flow with Exit(1). Added per-attribute-path build result notifications.
DerivedPath equality operator removal src/libstore/include/nix/store/derived-path.hh	Removed the explicit defaulted equality operator operator== from DerivedPath, eliminating its public declaration and removing commented spaceship operator note.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

• flake.cc: Significant refactoring with synchronized data structures, modified error handling flow, and revised build/missing logic requires careful review of lock semantics and control flow changes
• eval-cache files: Helper function extraction and type alias are straightforward but require verification of correct usage patterns in dependent code
• derived-path.hh: Removal of equality operator requires checking for impact on any usages or unit tests that might depend on it

Possibly related PRs

• Parallel eval performance improvements #194: Touches concurrency/synchronization abstractions, overlapping with synchronized containers and locking introduced here.
• nix flake check: Remove incorrect assertion #252: Also modifies flake check logic in src/nix/flake.cc, specifically removing assertions around the missing handling that this PR refactors.

Suggested reviewers

• cole-h
• grahamc

Poem

🐰 A rabbit hops through sync'd-up paths so fine,
With attribute maps now safely in line,
From simple errors to graceful Exit calls,
The flake check dances through logic's halls,
Thread-safe and tidy, a hop-ful delight! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The pull request title accurately describes the main objective: adding functionality to show which outputs failed or succeeded in nix flake check.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch nix-flake-check-results

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 889daaf and 256efe2.

📒 Files selected for processing (1)

• src/libstore/include/nix/store/derived-path.hh (0 hunks)

💤 Files with no reviewable changes (1)

• src/libstore/include/nix/store/derived-path.hh

⏰ 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

---

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

[github-actions]

🚀 Deployed on https://6932a63ae9f719290f78afe1--determinate-nix.netlify.app

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

src/nix/flake.cc (1)

850-877: Build result reporting logic is sound, but map lookups could be safer.

The three-phase reporting (successes, cancellations, failures) provides clear user feedback. However, using operator[] on the map (lines 856, 862, 872) will silently insert empty vectors if buildResult.path isn't found, which could mask issues during debugging.

Consider using .find() or .at() for defensive lookups:

-                    for (auto & attrPath : (*derivedPathToAttrPaths)[buildResult.path])
+                    auto it = derivedPathToAttrPaths->find(buildResult.path);
+                    if (it != derivedPathToAttrPaths->end())
+                        for (auto & attrPath : it->second)

Alternatively, use .at() to get an exception if the path is unexpectedly missing, which would surface bugs earlier.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 055dfbc and 889daaf.

📒 Files selected for processing (3)

• src/libexpr/eval-cache.cc (1 hunks)
• src/libexpr/include/nix/expr/eval-cache.hh (1 hunks)
• src/nix/flake.cc (5 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

src/nix/flake.cc (3)

src/libstore/include/nix/store/store-api.hh (4)

• drvPath (684-684)
• drvPath (769-769)
• drvPath (774-774)
• drvPath (779-779)

src/libstore/include/nix/store/legacy-ssh-store.hh (1)

• drvPaths (142-143)

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

• toAttrPathStr (382-385)
• toAttrPathStr (382-382)

⏰ 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 (4)

src/libexpr/include/nix/expr/eval-cache.hh (1)

16-18: LGTM! Clean API addition for attribute path handling.

The new AttrPath type alias and toAttrPathStr function provide a reusable way to format attribute paths outside of AttrCursor context, which is needed for the flake check reporting feature.

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

382-395: LGTM! Good refactoring to extract common attribute path formatting.

The toAttrPathStr helper centralizes the attribute path formatting logic, and the AttrCursor methods now delegate to it appropriately. This enables reuse from flake.cc for build result reporting.

src/nix/flake.cc (2)

637-643: Attribute path tracking for checks is implemented correctly.

The code properly records the mapping before moving derivedPath, avoiding use-after-move. The attribute path uses Symbol values which are efficient indices into the symbol table.

---

886-889: Clean exit handling for error conditions.

Using throw Exit(1) provides a proper exit code without a stack trace, which is appropriate for user-facing build failures. The std::atomic_bool hasErrors correctly aggregates errors from parallel evaluation and build phases.