Conversation
52ba749 to
3c070d9
Compare
| if (!useDerivation) | ||
| throw Error("some dependencies of '%s' are missing", worker.store.printStorePath(drvPath)); |
There was a problem hiding this comment.
I think the idea was that in the 0-dep case, this is more of an internal error. I think that's a small detail and it's fine to just use the other error code-path regardless?
See change originating in 1511aa9
| }; | ||
|
|
||
| for (const auto & [inputDrvPath, inputNode] : dynamic_cast<Derivation *>(drv.get())->inputDrvs.map) { | ||
| for (const auto & [inputDrvPath, inputNode] : drv->inputDrvs.map) { |
There was a problem hiding this comment.
Cast has been obsolete for a while now.
3c070d9 to
c2141e7
Compare
| if (useDerivation) { | ||
| auto & fullDrv = *dynamic_cast<Derivation *>(drv.get()); | ||
| { | ||
| auto & fullDrv = *drv; |
There was a problem hiding this comment.
I guess we can replace all uses of fullDrv by drv-> ?
There was a problem hiding this comment.
Yes we can, but I wanted to save such churn for later :)
There was a problem hiding this comment.
Reading more, this looks like a "free bonus feature" to me also?
If you have the build closure, the "outputs to drv" map provides strictly more ways to repair the path.
|
|
||
| /* If we're working from an in-memory derivation with no in-store | ||
| `*.drv` file, we cannot do this part. */ | ||
| if (worker.store.isValidPath(drvPath)) |
There was a problem hiding this comment.
This seems fishy. It makes the behaviour dependent on whether drvPath happens to exist, rather than whether we're building from an in-memory drv or not.
There was a problem hiding this comment.
That's true, but I think it's fine. Repairing is a best-effort operation where we through the kitchen sink at the problem. If we have the drv on hand, that's another tool in our toolbox.
I think you added the original conditional in 1511aa9 simply because this part wouldn't work without the file on hand, there wasn't some deeper reason for it.
Finally, I kinda doubt anyone is doing remote building (current buildDerivation code path) with bmRepair anyways :)
| if (settings.buildHook.get().empty() || !worker.tryBuildHook || !useDerivation) return rpDecline; | ||
| /* This should use `worker.evalStore`, but per #13179 the build hook | ||
| doesn't work with eval store anyways. */ | ||
| if (settings.buildHook.get().empty() || !worker.tryBuildHook || !worker.store.isValidPath(drvPath)) return rpDecline; |
There was a problem hiding this comment.
If you have the drv on hand, it's a "free bonus feature" that now the build hook works. No change in the semantics of what's trying to be accomplished.
At the same time, the only reason the build hook requires an in-store drv file is because the build hook protocol sucks (as we all agree). Once it's inlined per #5025 (Fixing #13179) in the process, this conditional goes away.
There was a problem hiding this comment.
If you have the drv on hand, it's a "free bonus feature" that now the build hook works.
I mean, that's the problem: something mysteriously succeeding or failing depending on whether the .drv happens to be present.
There was a problem hiding this comment.
Yeah it will be better with the build hook mechanism removed, and then no more mysterious failure/success for this one.
c2141e7 to
b82e21f
Compare
Now, each class provides the initial coroutine by value. This avoids some sketchy virtual function stuff, and will also be further put to good use in the next commit.
Try to make `DerivationGoal` care less whether we're working from an in-memory derivation or not. It's a clean-up in its own right, but it will also help with other cleanups under the umbrella of NixOS#12628.
b82e21f to
01207fd
Compare
Motivation
Try to make
DerivationGoalcare less whether we're working from an in-memory derivation or not.Context
It's a clean-up in its own right, but it will also help with other cleanups under the umbrella of #12628
Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.