Merge pull request #13177 from obsidiansystems/less-useDerivation

Remove `useDerivation`

Author: Ericson2314

src/libstore/build/derivation-goal.cc

@@ -25,8 +25,7 @@ namespace nix {
 
 DerivationGoal::DerivationGoal(const StorePath & drvPath,
     const OutputsSpec & wantedOutputs, Worker & worker, BuildMode buildMode)
-    : Goal(worker)
-    , useDerivation(true)
+    : Goal(worker, loadDerivation())
     , drvPath(drvPath)
     , wantedOutputs(wantedOutputs)
     , buildMode(buildMode)
@@ -43,8 +42,7 @@ DerivationGoal::DerivationGoal(const StorePath & drvPath,
 
 DerivationGoal::DerivationGoal(const StorePath & drvPath, const BasicDerivation & drv,
     const OutputsSpec & wantedOutputs, Worker & worker, BuildMode buildMode)
-    : Goal(worker)
-    , useDerivation(false)
+    : Goal(worker, haveDerivation())
     , drvPath(drvPath)
     , wantedOutputs(wantedOutputs)
     , buildMode(buildMode)
@@ -143,10 +141,10 @@ void DerivationGoal::addWantedOutputs(const OutputsSpec & outputs)
 }
 
 
-Goal::Co DerivationGoal::init() {
-    trace("init");
+Goal::Co DerivationGoal::loadDerivation() {
+    trace("local derivation");
 
-    if (useDerivation) {
+    {
         /* The first thing to do is to make sure that the derivation
            exists.  If it doesn't, it may be created through a
            substitute. */
@@ -355,7 +353,7 @@ Goal::Co DerivationGoal::gaveUpOnSubstitution()
 
     std::map<ref<const SingleDerivedPath>, GoalPtr, value_comparison> inputGoals;
 
-    if (useDerivation) {
+    {
         std::function<void(ref<const SingleDerivedPath>, const DerivedPathMap<StringSet>::ChildNode &)> addWaiteeDerivedPath;
 
         addWaiteeDerivedPath = [&](ref<const SingleDerivedPath> inputDrv, const DerivedPathMap<StringSet>::ChildNode & inputNode) {
@@ -375,7 +373,7 @@ Goal::Co DerivationGoal::gaveUpOnSubstitution()
                     childNode);
         };
 
-        for (const auto & [inputDrvPath, inputNode] : dynamic_cast<Derivation *>(drv.get())->inputDrvs.map) {
+        for (const auto & [inputDrvPath, inputNode] : drv->inputDrvs.map) {
             /* Ensure that pure, non-fixed-output derivations don't
                depend on impure derivations. */
             if (experimentalFeatureSettings.isEnabled(Xp::ImpureDerivations) && !drv->type().isImpure() && !drv->type().isFixed()) {
@@ -417,8 +415,6 @@ Goal::Co DerivationGoal::gaveUpOnSubstitution()
     trace("all inputs realised");
 
     if (nrFailed != 0) {
-        if (!useDerivation)
-            throw Error("some dependencies of '%s' are missing", worker.store.printStorePath(drvPath));
         auto msg = fmt(
             "Cannot build '%s'.\n"
             "Reason: " ANSI_RED "%d %s failed" ANSI_NORMAL ".",
@@ -435,8 +431,8 @@ Goal::Co DerivationGoal::gaveUpOnSubstitution()
     /* Determine the full set of input paths. */
 
     /* First, the input derivations. */
-    if (useDerivation) {
-        auto & fullDrv = *dynamic_cast<Derivation *>(drv.get());
+    {
+        auto & fullDrv = *drv;
 
         auto drvType = fullDrv.type();
         bool resolveDrv = std::visit(overloaded {
@@ -918,7 +914,12 @@ Goal::Co DerivationGoal::repairClosure()
        derivation is responsible for which path in the output
        closure. */
     StorePathSet inputClosure;
-    if (useDerivation) worker.store.computeFSClosure(drvPath, inputClosure);
+
+    /* 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))
+        worker.store.computeFSClosure(drvPath, inputClosure);
+
     std::map<StorePath, StorePath> outputsToDrv;
     for (auto & i : inputClosure)
         if (i.isDerivation()) {
@@ -1133,7 +1134,9 @@ HookReply DerivationGoal::tryBuildHook()
 #ifdef _WIN32 // TODO enable build hook on Windows
     return rpDecline;
 #else
-    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;
 
     if (!worker.hook)
         worker.hook = std::make_unique<HookInstance>();
@@ -1399,33 +1402,32 @@ void DerivationGoal::flushLine()
 std::map<std::string, std::optional<StorePath>> DerivationGoal::queryPartialDerivationOutputMap()
 {
     assert(!drv->type().isImpure());
-    if (!useDerivation || drv->type().hasKnownOutputPaths()) {
-        std::map<std::string, std::optional<StorePath>> res;
-        for (auto & [name, output] : drv->outputs)
-            res.insert_or_assign(name, output.path(worker.store, drv->name, name));
-        return res;
-    } else {
-        for (auto * drvStore : { &worker.evalStore, &worker.store })
-            if (drvStore->isValidPath(drvPath))
-                return worker.store.queryPartialDerivationOutputMap(drvPath, drvStore);
-        assert(false);
-    }
+
+    for (auto * drvStore : { &worker.evalStore, &worker.store })
+        if (drvStore->isValidPath(drvPath))
+            return worker.store.queryPartialDerivationOutputMap(drvPath, drvStore);
+
+    /* In-memory derivation will naturally fall back on this case, where
+       we do best-effort with static information. */
+    std::map<std::string, std::optional<StorePath>> res;
+    for (auto & [name, output] : drv->outputs)
+        res.insert_or_assign(name, output.path(worker.store, drv->name, name));
+    return res;
 }
 
 OutputPathMap DerivationGoal::queryDerivationOutputMap()
 {
     assert(!drv->type().isImpure());
-    if (!useDerivation || drv->type().hasKnownOutputPaths()) {
-        OutputPathMap res;
-        for (auto & [name, output] : drv->outputsAndOptPaths(worker.store))
-            res.insert_or_assign(name, *output.second);
-        return res;
-    } else {
-        for (auto * drvStore : { &worker.evalStore, &worker.store })
-            if (drvStore->isValidPath(drvPath))
-                return worker.store.queryDerivationOutputMap(drvPath, drvStore);
-        assert(false);
-    }
+
+    for (auto * drvStore : { &worker.evalStore, &worker.store })
+        if (drvStore->isValidPath(drvPath))
+            return worker.store.queryDerivationOutputMap(drvPath, drvStore);
+
+    // See comment in `DerivationGoal::queryPartialDerivationOutputMap`.
+    OutputPathMap res;
+    for (auto & [name, output] : drv->outputsAndOptPaths(worker.store))
+        res.insert_or_assign(name, *output.second);
+    return res;
 }
 
 

src/libstore/build/drv-output-substitution-goal.cc

@@ -12,7 +12,7 @@ DrvOutputSubstitutionGoal::DrvOutputSubstitutionGoal(
     Worker & worker,
     RepairFlag repair,
     std::optional<ContentAddress> ca)
-    : Goal(worker)
+    : Goal(worker, init())
     , id(id)
 {
     name = fmt("substitution of '%s'", id.to_string());

src/libstore/build/substitution-goal.cc

@@ -9,7 +9,7 @@
 namespace nix {
 
 PathSubstitutionGoal::PathSubstitutionGoal(const StorePath & storePath, Worker & worker, RepairFlag repair, std::optional<ContentAddress> ca)
-    : Goal(worker)
+    : Goal(worker, init())
     , storePath(storePath)
     , repair(repair)
     , ca(ca)

src/libstore/include/nix/store/build/derivation-goal.hh

@@ -33,11 +33,6 @@ void runPostBuildHook(
  */
 struct DerivationGoal : public Goal
 {
-    /**
-     * Whether to use an on-disk .drv file.
-     */
-    bool useDerivation;
-
     /** The path of the derivation. */
     StorePath drvPath;
 
@@ -166,7 +161,7 @@ struct DerivationGoal : public Goal
     /**
      * The states.
      */
-    Co init() override;
+    Co loadDerivation();
     Co haveDerivation();
     Co gaveUpOnSubstitution();
     Co tryToBuild();

src/libstore/include/nix/store/build/drv-output-substitution-goal.hh

@@ -33,7 +33,7 @@ public:
     typedef void (DrvOutputSubstitutionGoal::*GoalState)();
     GoalState state;
 
-    Co init() override;
+    Co init();
     Co realisationFetched(Goals waitees, std::shared_ptr<const Realisation> outputInfo, nix::ref<nix::Store> sub);
 
     void timedOut(Error && ex) override { unreachable(); };

src/libstore/include/nix/store/build/goal.hh

@@ -338,17 +338,6 @@ protected:
      */
     std::optional<Co> top_co;
 
-    /**
-     * The entry point for the goal
-     */
-    virtual Co init() = 0;
-
-    /**
-     * Wrapper around @ref init since virtual functions
-     * can't be used in constructors.
-     */
-    inline Co init_wrapper();
-
     /**
      * Signals that the goal is done.
      * `co_return` the result. If you're not inside a coroutine, you can ignore
@@ -376,8 +365,8 @@ public:
      */
     std::optional<Error> ex;
 
-    Goal(Worker & worker)
-        : worker(worker), top_co(init_wrapper())
+    Goal(Worker & worker, Co init)
+        : worker(worker), top_co(std::move(init))
     {
         // top_co shouldn't have a goal already, should be nullptr.
         assert(!top_co->handle.promise().goal);
@@ -440,7 +429,3 @@ template<typename... ArgTypes>
 struct std::coroutine_traits<nix::Goal::Co, ArgTypes...> {
     using promise_type = nix::Goal::promise_type;
 };
-
-nix::Goal::Co nix::Goal::init_wrapper() {
-    co_return init();
-}

src/libstore/include/nix/store/build/substitution-goal.hh

@@ -64,7 +64,7 @@ public:
     /**
      * The states.
      */
-    Co init() override;
+    Co init();
     Co gotInfo();
     Co tryToRun(StorePath subPath, nix::ref<Store> sub, std::shared_ptr<const ValidPathInfo> info, bool & substituterFailed);
     Co finished();