Skip to content

Clean up derivation goals a bit#12630

Merged
Ericson2314 merged 13 commits intoNixOS:masterfrom
L-as:me/clean-up-drv-goal
Mar 12, 2025
Merged

Clean up derivation goals a bit#12630
Ericson2314 merged 13 commits intoNixOS:masterfrom
L-as:me/clean-up-drv-goal

Conversation

@L-as
Copy link
Copy Markdown
Member

@L-as L-as commented Mar 11, 2025

Motivation

#12628


Add 👍 to pull requests you find important.

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

@L-as L-as requested a review from Ericson2314 as a code owner March 11, 2025 01:16
@github-actions github-actions bot added the store Issues and pull requests concerning the Nix store label Mar 11, 2025
@L-as L-as force-pushed the me/clean-up-drv-goal branch from ec02f08 to bcd5fed Compare March 11, 2025 01:28
@L-as L-as requested review from edolstra and roberth as code owners March 11, 2025 01:28
@github-actions github-actions bot added documentation repl The Read Eval Print Loop, "nix repl" command and debugger fetching Networking with the outside (non-Nix) world, input locking labels Mar 11, 2025
@L-as L-as force-pushed the me/clean-up-drv-goal branch from bcd5fed to 07c86b7 Compare March 11, 2025 01:33
@Ericson2314 Ericson2314 force-pushed the me/clean-up-drv-goal branch from 24879be to b104b1d Compare March 11, 2025 01:46
@L-as L-as force-pushed the me/clean-up-drv-goal branch from b104b1d to 4d50386 Compare March 11, 2025 02:03
@Ericson2314 Ericson2314 force-pushed the me/clean-up-drv-goal branch from 4d50386 to f1232c8 Compare March 12, 2025 21:25
Ericson2314 and others added 13 commits March 12, 2025 18:00
This will help avoid duplication later. In particular, the next commit
will not need to duplicate as much.
The basic idea is that while we have duplicated this function, we now
have one call-site in the local build case, and one call site in the
build hook case. This unlocks big opportunities to specialize each copy,
since they really shouldn't be doing the same things. By the time we are
are done, there should not be much duplication left.

See NixOS#12628 for further info.
Thanks to the previous commit, we can inline all these small callbacks.
In the build-hook case, they were empty, and now they disappear
entirely.

While `LocalDerivationGoal` can be used in the hook case (we use it
based on whether we have a local store, not based on whether we are
using the build hook, a decision which comes later), the previous
commit's inline moved the code into a spot where we know we are cleaning
up after local building, *not* after running the build hook. This allows
for much more simplification.
The `assert` above proves that `hook` is not set.
The code that was in between is now gone. We can just set `st` correctly
the first time.
Can just inline its definition, it was immutable.
- `chrootParentDir` can be a local variable instead of a class variable.

- `getChildStatus` can be inlined. Again, we have the `assert(!hook);`
  in the local building case, which makes for a simpler thing inlined.
Easy to inline in one spot, and assert in the other.
We can move this method from `LocalStore` to `Store` --- even if we only
want the actual builder to sign things in many cases, there is no reason
to try to enforce this policy by spurious moving the method to a
subclass.

Now, we might technically sign class, but CA derivations is
experimental, and @Ericson2314 is going to revisit all this stuff with
issue NixOS#11896 anyways.
The variables are only set by CGroup mechanisms in `killSandbox` in the
local build. In the build hook case, these variables will not be set, so
there is nothing to do.
In the local building case, there is many things which can through
`BuildError`, but in the hook case there is just this one. We can
therefore simplify the code by "cinching" down the logic just to the
spot the error is thrown.

There is other code outside `libstore/build` which also uses
`BuildError`, but I believe those cases are mistakes. The point of
`BuildError` is the narrow technical use-cases of "errors which should
not be fatal with `--keep-going`". Using it outside the
building/scheduling code doesn't really make sense in that regard. It
seems likely that those usages were instead merely because "oh, this
error has something to do with building, so I guess `BuildError` is
better than `Error`".

It is quite likely that I myself used `BuildError` incorrectly as
described above :).
The simplification here is due to a long-standing bug, but it is not
worth fixing the bug at this time. Instead we've finally written up an
issue for the bug, and referenced the issue number in the code.
@Ericson2314 Ericson2314 force-pushed the me/clean-up-drv-goal branch from f1232c8 to dc0bc7f Compare March 12, 2025 22:15
@Ericson2314 Ericson2314 enabled auto-merge March 12, 2025 22:18
@Ericson2314 Ericson2314 merged commit 1055c9f into NixOS:master Mar 12, 2025
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation fetching Networking with the outside (non-Nix) world, input locking repl The Read Eval Print Loop, "nix repl" command and debugger store Issues and pull requests concerning the Nix store

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants