nix build, nix profile: Report failing/succeeding installables#281
nix build, nix profile: Report failing/succeeding installables#281
Conversation
This denotes the result of a build that didn't succeed or fail, but was cancelled because some other goal failed and --keep-going was not enabled.
WalkthroughReplaces pair-based build results with a variant-wrapped InstallableWithBuildResult, changes Installable::build2() to return those, adds Installable::throwBuildErrors(), moves statusToString helpers into BuildResult nested types, adds Failure::Cancelled, initializes Goal.buildResult to Cancelled, and updates profile code and tests to the new flow. Changes
Sequence Diagram(s)sequenceDiagram
participant Cmd as Command (profile/add/upgrade)
participant Build2 as Installable::build2()
participant Store as Store
participant Throw as Installable::throwBuildErrors()
participant Extract as InstallableWithBuildResult::getSuccess()
participant Caller as Caller finalizing results
Cmd->>Build2: invoke build2(...)
Build2-->>Cmd: returns [InstallableWithBuildResult{Success|Failure}, ...]
Cmd->>Throw: throwBuildErrors(buildResults, Store)
alt any Failure present
Throw->>Cmd: logs successes, reports cancelled as notices, throws on non-cancelled failures
Cmd-->>Caller: propagation of thrown Exit/error
else all Success
Throw-->>Cmd: returns normally
Cmd->>Extract: for each InstallableWithBuildResult call getSuccess()
alt variant == Success
Extract-->>Cmd: returns BuiltPathWithResult
else variant == Failure
Extract->>Cmd: rethrows stored Failure
end
Cmd-->>Caller: vector<BuiltPathWithResult>
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/libcmd/include/nix/cmd/installables.hh (1)
99-113: Clean wrapper type for build results per installable.The
InstallableWithBuildResultstruct provides a well-structured way to associate build outcomes with their installables. The type aliases improve readability.One minor observation: the comment on line 105 states that
Failure"must be aBuildResult::Failure", but the type alias is justBuildResult(which contains a variant). Consider whether the type could be tightened toBuildResult::Failuredirectly, or if the fullBuildResultis needed for additional metadata (like timing info). The current approach works but relies on the caller to uphold the invariant.src/libcmd/installables.cc (1)
594-602: Consider usingstd::getfor clarity.Since the variant can only hold
SuccessorFailure, usingstd::get<Success>(result)would be clearer and provide a runtime check, avoiding the nullable pointer dereference pattern.const BuiltPathWithResult & InstallableWithBuildResult::getSuccess() const { if (auto * failure = std::get_if<Failure>(&result)) { auto failure2 = failure->tryGetFailure(); assert(failure2); failure2->rethrow(); - } else - return *std::get_if<Success>(&result); + } + return std::get<Success>(result); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/libcmd/include/nix/cmd/installables.hh(2 hunks)src/libcmd/installables.cc(6 hunks)src/libstore/build-result.cc(4 hunks)src/libstore/include/nix/store/build-result.hh(3 hunks)src/libstore/include/nix/store/build/goal.hh(1 hunks)src/nix/profile.cc(4 hunks)tests/functional/build.sh(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/nix/profile.cc (2)
src/libcmd/include/nix/cmd/installables.hh (2)
buildResults(186-186)store(198-198)src/libcmd/installables.cc (4)
build2(652-763)build2(652-653)throwBuildErrors(604-639)throwBuildErrors(604-604)
src/libstore/build-result.cc (1)
src/libstore/include/nix/store/build-result.hh (4)
status(34-34)status(45-49)status(45-45)status(83-83)
src/libstore/include/nix/store/build-result.hh (1)
src/libstore/build-result.cc (4)
statusToString(16-30)statusToString(16-16)statusToString(32-64)statusToString(32-32)
⏰ 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_x86_64-linux / build
- GitHub Check: build_aarch64-darwin / build
🔇 Additional comments (18)
src/libstore/include/nix/store/build/goal.hh (1)
112-112: LGTM! Sensible default for build result.Initializing
buildResulttoCancelledensures that goals that never complete (or are interrupted) have a well-defined failed state. This aligns with the new per-installable reporting where cancelled builds show ❓.src/libstore/include/nix/store/build-result.hh (4)
34-35: LGTM! Clean static helper declaration.The
statusToStringhelper provides a well-typed way to convert status codes to strings, improving API clarity over a free function.
83-84: LGTM! Symmetric withSuccess::statusToString.Provides consistent API for status-to-string conversion across both result types.
106-106: Good fallback for empty error messages.Using
statusToString(status)whenerrorMsgis empty ensuresrethrow()always provides a meaningful error message rather than throwing an empty one.
80-80: Based on my verification, I've found important evidence about the review comment:Key Findings:
HashMismatch (value 15) in
src/libstore/include/nix/store/build-result.hhhas an explicit 3-line comment explaining:
- "The protocols do not yet know about this one"
- "change it back to
OutputRejectedbefore serialization"Serialization Fallback in
src/libstore/build/derivation-building-goal.cc(lines 843-850):
- When
BuildResult::Failure::HashMismatchis encountered, it's unconditionally converted toOutputRejected- The comment states: "the protocols don't know about
HashMismatchyet"Cancelled (value 16) has:
- NO explanatory comment like HashMismatch
- NO corresponding conversion/fallback in the switch statement
- NOT handled in the serialization code
Pattern Observed:
- The switch statement at lines 843-857 handles specific statuses that need adjustment
- Cancelled is conspicuously absent from this handling
- The default case says "Other statuses need no adjusting"
The review comment's concern appears valid and substantiated. The question of whether
Cancelledneeds similar protocol compatibility handling toHashMismatchcannot be definitively answered without understanding: (1) whetherCancelledis sent to older protocol versions, and (2) whether those older versions recognize it. However, the evidence suggests this warrants investigation or clarification.
Verify protocol compatibility for
Cancelledstatus.
Cancelled = 16lacks the protocol compatibility documentation and serialization fallback thatHashMismatch = 15explicitly requires.HashMismatch(lines 76-78) is documented as needing conversion toOutputRejectedbefore serialization because older protocols don't recognize it.Cancelledhas no such comment or fallback in the switch statement atsrc/libstore/build/derivation-building-goal.cc:843-857. IfCancelledcan be sent to older protocol versions, it should receive equivalent treatment.tests/functional/build.sh (2)
170-174: LGTM! Correct error count for keep-going with 3 failing FODs.With
--keep-going, all three FODs (x1, x2, x3) are attempted and fail independently, so expecting 3 errors is correct. The individual hash mismatch assertions properly validate each failure is reported.
164-166: Based on my analysis of the code:The test expectation of 3 cancellations is correct.
The
fod-failing.nixfile defines 4 derivations:
- x1, x2, x3: Fixed-output derivations (FODs) with identical invalid hashes
- x4: A regular derivation that depends on x2 and x3
When
nix build -f fod-failing.nixis run without explicit targets (line 161), all top-level attributes are attempted:
- One FOD (typically x1) fails first with a hash mismatch error → generates 1 error message
- The remaining three derivations (x2, x3, x4) are cancelled as a result → 3 cancellations
This aligns with the modern error reporting behavior where:
- Each installable build attempt generates one error entry
- Dependent builds are cancelled and counted separately
- The test correctly validates this by checking for exactly 1 error and 3 cancellations
The test structure is sound and the assertion is accurate.
src/libstore/build-result.cc (3)
16-30: LGTM! Clean implementation with exhaustive switch.The
statusToStringimplementation correctly covers allSuccess::Statusenum values withunreachable()as a defensive default.
32-64: LGTM! Exhaustive handling of all failure statuses including newCancelled.All
Failure::Statusenum values are handled, with the newCancelledcase properly returning"Cancelled".
71-73: LGTM! JSON serialization updated to use the new member functions.Using the nested
statusToStringhelpers is cleaner than free functions and provides better API discoverability.src/libcmd/include/nix/cmd/installables.hh (2)
179-184: LGTM! Improved return type forbuild2.Returning
std::vector<InstallableWithBuildResult>instead ofstd::vector<std::pair<ref<Installable>, BuiltPathWithResult>>is a cleaner API that can now represent both successes and failures per installable.
186-187: LGTM! NewthrowBuildErrorsdeclaration.This provides a clean separation between collecting build results and reporting errors. Taking a non-const reference suggests it may consume or modify the results, which should be documented in the implementation.
src/nix/profile.cc (3)
315-335: LGTM!The function correctly adapts to the new
InstallableWithBuildResulttype. The call tob.getSuccess()is safe here becausethrowBuildErrorsis always invoked before this function at both call sites, ensuring only successful results remain when this code executes.
367-370: LGTM!The two-phase flow (build → throw errors → extract paths) is clean and makes error handling explicit. This ensures users see proper success/failure reporting before the profile is updated.
772-775: LGTM!Consistent with the
CmdProfileAddchanges, applying the same two-phase build result handling pattern.src/libcmd/installables.cc (3)
24-24: LGTM!Required include for the
Exitexception used inthrowBuildErrors.
641-650: LGTM!Clean wrapper that maintains the simpler
build()API while delegating tobuild2()internally. The error throwing before extraction ensuresgetSuccess()is safe.
652-763: LGTM!The refactored
build2()cleanly separates success and failure handling using the newInstallableWithBuildResultwrapper. Failures are detected early and stored directly (lines 718-723), while successes are properly wrapped with path information.
If any installable fails to build, it will print something like
✅ flake:nixpkgs#hello
❌ git+file:///home/eelco/Dev/patchelf#packages.x86_64-linux.default
error: Cannot build '/nix/store/nh2dx9cqcy9lw4d4rvd0dbsflwdsbzdy-patchelf-0.18.0.drv'.
Reason: builder failed with exit code 2.
...
❓ git+file:///home/eelco/Dev/patchelf#hydraJobs.coverage (cancelled)
It doesn't print anything if all installables succeed.
edolstra
force-pushed
the
show-build-result
branch
from
aef7257 to
ee75a66
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/libcmd/installables.cc (2)
604-639: Well-structured error reporting with clear status differentiation.The method properly reports successes first, then failures, and distinguishes between actual failures (❌) and cancelled builds (❓). The triple iteration through
buildResultsis clear and maintainable, though it could be consolidated into a single pass if performance becomes a concern.Note: The method only prints per-installable status when at least one failure exists, which appears intentional to avoid noise when all builds succeed.
If you'd like to optimize the multiple passes through
buildResults, consider collecting the messages in one pass:void Installable::throwBuildErrors(std::vector<InstallableWithBuildResult> & buildResults, const Store & store) { + std::vector<std::string> successMsgs, failureMsgs; + bool hasFailures = false; + for (auto & buildResult : buildResults) { - if (std::get_if<InstallableWithBuildResult::Failure>(&buildResult.result)) { - // Report success first. - for (auto & buildResult : buildResults) { - if (std::get_if<InstallableWithBuildResult::Success>(&buildResult.result)) - notice("✅ " ANSI_BOLD "%s" ANSI_NORMAL, buildResult.installable->what()); - } - - // Then failures. - for (auto & buildResult : buildResults) { - if (auto failure = std::get_if<InstallableWithBuildResult::Failure>(&buildResult.result)) { - auto failure2 = failure->tryGetFailure(); - assert(failure2); - if (failure2->status == BuildResult::Failure::Cancelled - // FIXME: remove MiscFailure eventually - || failure2->status == BuildResult::Failure::MiscFailure) - notice( - "❓ " ANSI_BOLD "%s" ANSI_NORMAL ANSI_FAINT " (cancelled)", - buildResult.installable->what()); - else { - printError("❌ " ANSI_RED "%s" ANSI_NORMAL, buildResult.installable->what()); - try { - failure2->rethrow(); - } catch (Error & e) { - logError(e.info()); - } - } - } - } - - throw Exit(1); + if (std::get_if<InstallableWithBuildResult::Success>(&buildResult.result)) { + successMsgs.push_back(buildResult.installable->what()); + } else if (auto failure = std::get_if<InstallableWithBuildResult::Failure>(&buildResult.result)) { + hasFailures = true; + // Store failure details... } } + + if (hasFailures) { + // Print all collected messages + // ... + throw Exit(1); + } }
652-763: Solid implementation of variant-based build result flow.The method correctly constructs
InstallableWithBuildResultentries for all build paths, properly handling both success and failure cases across different realization modes. The distinction between modes that include actual build results (Outputs) versus those that just resolve paths (Nothing/Derivation) is clear.There's some code duplication in constructing Success results for
DerivedPath::BuiltvsDerivedPath::Opaquecases across different modes (lines 686-697 vs 700-703, and lines 728-742 vs 744-748). Consider extracting a helper function to reduce duplication:auto makeSuccessResult = [&](const BuiltPath & path, const Aux & aux, std::optional<BuiltPathWithResult> buildResult = std::nullopt) { return InstallableWithBuildResult{ .installable = aux.installable, .result = InstallableWithBuildResult::Success{ .path = path, .info = aux.info, .result = buildResult } }; };This would make the code more maintainable if the Success structure changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/libcmd/include/nix/cmd/installables.hh(2 hunks)src/libcmd/installables.cc(6 hunks)src/nix/profile.cc(4 hunks)tests/functional/build.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/nix/profile.cc
- src/libcmd/include/nix/cmd/installables.hh
- tests/functional/build.sh
🧰 Additional context used
🧬 Code graph analysis (1)
src/libcmd/installables.cc (1)
src/libcmd/include/nix/cmd/installables.hh (7)
buildResults(186-186)evalStore(172-177)evalStore(179-184)evalStore(188-189)evalStore(191-192)evalStore(194-195)evalStore(200-201)
⏰ 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 (3)
src/libcmd/installables.cc (3)
24-24: LGTM - Required for new error handling.The new include is necessary for the
Exitexception thrown inthrowBuildErrors.
594-602: LGTM - Proper variant handling and error propagation.The method correctly extracts the success result or rethrows the stored failure. The assert ensures the failure state is valid before rethrowing.
641-650: LGTM - Clean delegation and error handling.The method properly delegates to
build2(), centralizes error handling viathrowBuildErrors(), and converts results by extracting successful paths. The pattern ensures that callers either get all successful results or an exception.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/functional/build.sh (1)
163-166: Comment is inconsistent with the test assertion.The comment on line 163 says "one cancelled build" but line 165 asserts there are 3 cancelled builds. The comment should be updated to match the actual expected behavior.
-# one "hash mismatch" error, one cancelled build +# one "hash mismatch" error, three cancelled builds
🧹 Nitpick comments (1)
src/libstore/include/nix/store/build-result.hh (1)
150-156: Consider tracking the FIXME for future cleanup.The
isCancelled()method correctly handles both the newCancelledstatus and the legacyMiscFailurestatus during the transition period. The FIXME comment appropriately documents thatMiscFailureshould be removed once migration is complete.Would you like me to open an issue to track the removal of the
MiscFailurefallback once all code paths consistently useCancelled?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/libcmd/installables.cc(6 hunks)src/libstore/include/nix/store/build-result.hh(4 hunks)tests/functional/build.sh(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/libstore/include/nix/store/build-result.hh (1)
src/libstore/build-result.cc (4)
statusToString(16-30)statusToString(16-16)statusToString(32-64)statusToString(32-32)
src/libcmd/installables.cc (2)
src/libstore/include/nix/store/build-result.hh (1)
failure(151-156)src/libcmd/include/nix/cmd/installables.hh (7)
buildResults(186-186)evalStore(172-177)evalStore(179-184)evalStore(188-189)evalStore(191-192)evalStore(194-195)evalStore(200-201)
⏰ 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 (8)
tests/functional/build.sh (1)
168-174: LGTM!The test expectations for
--keep-goingwith multiple FODs are correctly updated. Each failing FOD now reports its own hash mismatch error, and the assertions properly verify all three derivations.src/libstore/include/nix/store/build-result.hh (2)
80-84: LGTM!The new
Cancelledstatus follows the protocol compatibility requirements by being added at the end of the enum. ThestatusToStringhelper is properly declared and implemented in the corresponding.ccfile.
104-107: LGTM!Good improvement to
rethrow()- falling back tostatusToString(status)ensures a meaningful error message is always provided, even whenerrorMsgis empty.src/libcmd/installables.cc (5)
594-602: LGTM!The
getSuccess()method correctly handles the variant - rethrowing failures and returning the success path. The logic is sound since the variant can only beSuccessorFailure.
604-643: LGTM!The reporting order now follows the suggested pattern: successes first (✅), then cancelled builds (❓), then failures (❌). This makes it easier to find failures at the bottom of the output rather than having them interleaved with cancelled builds. The format strings are now correct with matching arguments.
645-654: LGTM!Clean wrapper that delegates to
build2()for the actual build, usesthrowBuildErrors()for consistent error reporting, and extracts successful results for the public API. Good separation of concerns.
736-752: LGTM!The success path correctly constructs
InstallableWithBuildResult::Successwith all necessary fields: the built path, extra path info, and the build result (which includes timing and other metadata).
722-727: No type compatibility issue exists. ThebuildResultvariable (typeKeyedBuildResult, which publicly inherits fromBuildResult) is correctly assigned to the.resultfield (typestd::variant<BuiltPathWithResult, BuildResult>). The implicit conversion from derived to base class is standard C++ and safe—there is no slicing of critical data. TheBuildResultbase contains all necessary failure information, and the extra metadata inKeyedBuildResultis not needed in the stored result.
2 participants
Motivation
If any installable fails to build in
nix buildornix profile install/upgrade, Nix will print something likeIt doesn't print anything if all installables succeed.
Context
Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.
Summary by CodeRabbit
Bug Fixes
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.