Get rid of addWantedOutputs
#13240
Open
Get rid of addWantedOutputs
#13240
Conversation
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
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 ! |
xokdvium
reviewed
May 23, 2025
Ericson2314
force-pushed
the
dyn-drv-take-3
branch
from
8a9bde3 to
a60ad96
Compare
xokdvium
reviewed
May 23, 2025
src/libstore/build/worker.cc
Outdated
Comment on lines
130
to
135
| 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 ! |
xokdvium
reviewed
Jun 18, 2025
xokdvium
reviewed
Jun 18, 2025
Comment on lines
+86
to
+90
| 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.
xokdvium
reviewed
Jun 18, 2025
xokdvium
reviewed
Jun 18, 2025
Ericson2314
force-pushed
the
dyn-drv-take-3
branch
from
30ccdd8 to
cdcbf52
Compare
xokdvium
reviewed
Jun 18, 2025
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
approved these changes
Jun 20, 2025
Comment on lines
+165
to
+172
| 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.
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
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.