Get rid of addWantedOutputs#13240
Conversation
github-actions
bot
added
with-tests
Ericson2314
force-pushed
the
dyn-drv-take-3
branch
from
15d4fb0 to
3969fcb
Compare
Ericson2314
changed the title
DerivationCreationAndRealisationGoal, get rid of addWantedOutputsaddWantedOutputs
Ericson2314
force-pushed
the
dyn-drv-take-3
branch
from
3969fcb to
8a9bde3
Compare
|
🎉 All dependencies have been resolved ! |
Ericson2314
force-pushed
the
dyn-drv-take-3
branch
from
8a9bde3 to
a60ad96
Compare
| static bool removeGoal(std::shared_ptr<G> goal, std::weak_ptr<G> & gp) | ||
| { | ||
| /* !!! inefficient */ | ||
| cullMap(goalMap, [&](const std::weak_ptr<G> & gp) -> bool { | ||
| return gp.lock() != goal; | ||
| }); | ||
| return gp.lock() != goal; |
There was a problem hiding this comment.
It's a bit strange that removeGoal does't actually remove anything now. Maybe something like shouldRemoveGoal is more fitting?
There was a problem hiding this comment.
Actually it has to have the same as the others for overloading/recursion reasons.
src/libstore/include/nix/store/build/derivation-creation-and-realisation-goal.hh
Outdated
Show resolved
Hide resolved
Ericson2314
force-pushed
the
dyn-drv-take-3
branch
5 times, most recently
from
a8965c5 to
2ff8cbc
Compare
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/2025-05-28-nix-team-meeting-minutes-229/65205/1 |
Ericson2314
force-pushed
the
dyn-drv-take-3
branch
from
2ff8cbc to
30ccdd8
Compare
|
🎉 All dependencies have been resolved ! |
| try { | ||
| drvPath = resolveDerivedPath(worker.store, *drvReq); | ||
| } catch (MissingRealisation &) { | ||
| return std::nullopt; | ||
| } |
There was a problem hiding this comment.
Not a big fan of using exceptions as control flow like this. But this is just moving code around, so not a big deal.
Ericson2314
force-pushed
the
dyn-drv-take-3
branch
from
30ccdd8 to
cdcbf52
Compare
Ericson2314
force-pushed
the
dyn-drv-take-3
branch
from
cdcbf52 to
ce93ef2
Compare
This is a minor oversight from 012453d.
This is just a code cleanup; it should not be behavior change. `addWantedOutputs` is removed by introducing `DerivationTrampolineGoal`. `DerivationGoal` now only tracks a single output, and is back to tracking a plain store path `drvPath`, not a deriving path one. Its `addWantedOutputs` method is gone. These changes will allow subsequent PRs to simplify it greatly. Because the purpose of each goal is back to being immutable, we can also once again make `Goal::buildResult` a public field, and get rid of the `getBuildResult` method. This simplifies things also. `DerivationTrampolineGoal` is, as the nane is supposed to indicate, a cheap "trampoline" goal. It takes immutable sets of wanted outputs, and just kicks of `DerivationGoal`s for them. Since now "actual work" is done in these goals, it is not wasteful to have separate ones for separate sets of outputs, even if those outputs (and the derivations they are from) overlap. This design is described in more detail in the doc comments on the goal types, which I've now greatly expanded. --- This separation of concerns will make it possible for future work on issues like NixOS#11928, and to continue the path of having more goal types, but each goal type does fewer things (issue NixOS#12628). --- This commit in some sense reverts f4f28cd, but that one kept around `addWantedOutputs`. I am quite sure it was having two layers of goals with `addWantedOutputs` that caused the issues --- restarting logic like `addWantedOutputs` has is very tempermental! In this version of the change, we have *zero* layers of `addWantedOutputs` --- no goal type needs it, or otherwise has a mutable objective --- and so I think this change is safe. Co-authored-by: Sergei Zimmerman <145775305+xokdvium@users.noreply.github.com>
Ericson2314
force-pushed
the
dyn-drv-take-3
branch
from
ce93ef2 to
a55806a
Compare
|
OK got your last two things, @xokdvium |
xokdvium
left a comment
xokdvium
left a comment
There was a problem hiding this comment.
High-level design SGTM, but I'm not very familiar with the code yet. This does seem like a good division of responsibilities and is better in terms of SRP.
Left one non-blocking question.
| auto & g = *concreteDrvGoals.begin(); | ||
| buildResult = g->buildResult; | ||
| for (auto & g2 : concreteDrvGoals) { | ||
| for (auto && [x, y] : g2->buildResult.builtOutputs) | ||
| buildResult.builtOutputs.insert_or_assign(x, y); | ||
| } | ||
|
|
||
| co_return amDone(g->exitCode, g->ex); |
There was a problem hiding this comment.
Non-blocking question: what is the semantics of buildResult now? Is the exitCode and ex the same for all concrete goals and only the builtOutputs are now different? I think you've had some low-hanging ideas for a follow-up simplication wrt to this.
There was a problem hiding this comment.
Yes, there are no longer different build result views based on which outputs one cares about. Just a single result for the goal, and that is fine for the downstream goal since the upstream goal's wanted outputs are not mutated --- it will refer to what the downstream goal originally requested.
roberth
added
the
backports rejected
4 participants
Motivation
Do this by introducing
DerivationTrampolineGoal.DerivationGoalnow only tracks a single output, and is back to tracking a plain store pathdrvPath, not a deriving path one. ItsaddWantedOutputsmethod is gone. These changes will allow subsequent PRs to simplify it greatly.Because the purpose of each goal is back to being immutable, we can also once again make
Goal::buildResulta public field, and get rid of thegetBuildResultmethod. This simplifies things also.DerivationTrampolineGoalis, as the nane is supposed to indicate, a cheap "trampoline" goal. It takes immutable sets of wanted outputs, and just kicks ofDerivationGoals for them. Since now "actual work" is done in these goals, it is not wasteful to have separate ones for separate sets of outputs, even if those outputs (and the derivations they are from) overlap.This design is described in more detail in the doc comments on the goal types, which I've now greatly expanded.
Context
This separation of concerns will make it possible for future work on issues like #11928, and to continue the path of having more goal types, but each goal type does fewer things (issue #12628).
This commit in some sense reverts f4f28cd, but that one kept around
addWantedOutputs. I am quite sure it was having two layers of goals withaddWantedOutputsthat caused the issues --- restarting logic likeaddWantedOutputshas is very tempermental! In this version of the change, we have zero layers ofaddWantedOutputs--- no goal type needs it, or otherwise has a mutable objective --- and so I think this change is safe.depends on #13186depends on #13294Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.