Clean up derivation goals a bit#12630
Merged
Ericson2314 merged 13 commits intoNixOS:masterfrom Mar 12, 2025
Merged
Conversation
Ericson2314
reviewed
Mar 11, 2025
ec02f08 to
bcd5fed
Compare
bcd5fed to
07c86b7
Compare
24879be to
b104b1d
Compare
b104b1d to
4d50386
Compare
4d50386 to
f1232c8
Compare
Ericson2314
approved these changes
Mar 12, 2025
Ericson2314
approved these changes
Mar 12, 2025
edolstra
approved these changes
Mar 12, 2025
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.
f1232c8 to
dc0bc7f
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
#12628
Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.