Skip to content

Use std::variant to enforce BuildResult invariants #14060

Merged
Ericson2314 merged 2 commits intoNixOS:masterfrom
obsidiansystems:build-result-variant
Sep 30, 2025
Merged

Use std::variant to enforce BuildResult invariants #14060
Ericson2314 merged 2 commits intoNixOS:masterfrom
obsidiansystems:build-result-variant

Conversation

@Ericson2314
Copy link
Member

Motivation

There is now a clean separation between successful and failing build results.

Context

I thought about making a JSON format for it after seeing what @RossComputerGuy got from detsys in #14031, and I wanted to do this first to better understand the data structure.

(I think the JSON format should also cleanly separate the success and failure cases. Among other benefits, such a format that means that even if there is an unknown status code, success and failure could still be told apart.)


Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

@github-actions github-actions bot added new-cli Relating to the "nix" command with-tests Issues related to testing. PRs with tests have some priority store Issues and pull requests concerning the Nix store c api Nix as a C library with a stable interface labels Sep 23, 2025
@edolstra
Copy link
Member

edolstra commented Sep 24, 2025

This is a fairly large amount of code churn (a 1807-line diff) while it's not entirely clear to me what "a clean separation between successful and failing build results" gains us... Can you expand on that a bit?

@Ericson2314
Copy link
Member Author

Ericson2314 commented Sep 24, 2025

Motivations include:

  • Make the various doneSuccess and doneFailure functions more correct in terms of the statuses they accept
  • Allow for a JSON format which distinguishes success/failure separate from the status code, so new status codes that are not understood by the client don't mess up telling apart successes and failures. That makes things a bit more extensible.

@Ericson2314 Ericson2314 marked this pull request as draft September 24, 2025 21:25
This allows refactoring without changing wire protocol by mistake.
There is now a clean separation between successful and failing build
results.
@Ericson2314 Ericson2314 marked this pull request as ready for review September 27, 2025 20:53
@Ericson2314
Copy link
Member Author

Addressed the review items above, and created the tryGet* helper functions as discussed in the meeting.

@Ericson2314 Ericson2314 merged commit d76dc24 into NixOS:master Sep 30, 2025
15 checks passed
@Ericson2314 Ericson2314 deleted the build-result-variant branch September 30, 2025 15:02
@github-project-automation github-project-automation bot moved this from Triage to Done in Nix team Sep 30, 2025
@Ericson2314
Copy link
Member Author

Thank you @edolstra. I promise that some day I will have finished churning the code :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c api Nix as a C library with a stable interface new-cli Relating to the "nix" command store Issues and pull requests concerning the Nix store with-tests Issues related to testing. PRs with tests have some priority

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

2 participants