Way more RAII for DerivationBuildingGoal#14788
Conversation
| * Line buffering and log tracking for build output. | ||
| * | ||
| * This class handles: | ||
| * - Owning the build Activity for logging |
There was a problem hiding this comment.
I would expect the build Activity to be broader than just the builder child process which this class seems to focus on (see eg doc for operator()). We also have things like pre and post build hooks, or the built-in reference checks that are part of the broader process of building.
There was a problem hiding this comment.
I think at the moment it is just using the public fiels on this for those. I don't really love that, but I think it's still a step in the right direction.
Ericson2314
force-pushed
the
coroutine-child-output
branch
from
b99af4a to
adca799
Compare
Ericson2314
force-pushed
the
coroutine-child-output
branch
2 times, most recently
from
d324c7d to
071d153
Compare
That's easy to do now.
This will allow for making `hook` and `builder` local variables.
Also remove uneeded arguments from `processChildOutput`.
This gets some trickier pure logic out of `DerivationBuilderGoal`.
Separate state for each loop.
Now it does more, using the logging.hh functionality as needed.
It is weird that `LogFile` and `BuildLog` are basically unrelated, but the does currently reflect the logic that exists.
There was a bunch of logic in there which was, effectively, using the build hook, rather than deciding *whether* to use the build hook. We want it to only be the latter.
These are immutable parameters, not state, set in the constructor.
This makes the logic much easier to follow. Unlike before, the use of separate functions is not making us pass a gazillion arguments or use the crutch of class variables.
Ericson2314
force-pushed
the
coroutine-child-output
branch
from
071d153 to
1807dc7
Compare
| LogFile(Store & store, const StorePath & drvPath); | ||
| ~LogFile(); |
There was a problem hiding this comment.
Maybe explicit rule of 5 here would be nice?
| LogFile(Store & store, const StorePath & drvPath); | |
| ~LogFile(); | |
| LogFile(Store & store, const StorePath & drvPath); | |
| LogFile(const LogFile &) = delete; | |
| LogFile(LogFile &&) = default; | |
| LogFile &operator=(const LogFile &) = delete; | |
| LogFile &operator=(LogFile &&) = default; | |
| ~LogFile(); |
If it is even supposed to be movable. If not, move constructors can also be deleted.
| auto openLogFile = [&]() { logFile = std::make_unique<LogFile>(worker.store, drvPath); }; | ||
|
|
||
| auto closeLogFile = [&]() { logFile.reset(); }; |
There was a problem hiding this comment.
A bit apprehensive about capturing by reference here due to lifetime semantics with coroutines.
There was a problem hiding this comment.
But probably ok since those variable live on the outer coroutine frame.
There was a problem hiding this comment.
Yeah I think it is OK for that reason --- especially now that builder is a local variable too.
| * The derivation stored at drvPath. | ||
| */ | ||
| std::unique_ptr<Derivation> drv; | ||
| const std::unique_ptr<Derivation> drv; |
There was a problem hiding this comment.
Would this work?
| const std::unique_ptr<Derivation> drv; | |
| const std::unique_ptr<const Derivation> drv; |
Way more RAII for `DerivationBuildingGoal`
3 participants
Motivation
This is important for making this code possible to maintain and understand.
Context
Please review each commit individually.
Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.