Merge pull request #14208 from obsidiansystems/consolidate-builder-dispatch

Consolidate logic choosing where we can/should build a bit

src/libstore/build/derivation-builder.cc

@@ -0,0 +1,27 @@
+#include "nix/util/json-utils.hh"
+#include "nix/store/build/derivation-builder.hh"
+
+namespace nlohmann {
+
+using namespace nix;
+
+ExternalBuilder adl_serializer<ExternalBuilder>::from_json(const json & json)
+{
+    auto obj = getObject(json);
+    return {
+        .systems = valueAt(obj, "systems"),
+        .program = valueAt(obj, "program"),
+        .args = valueAt(obj, "args"),
+    };
+}
+
+void adl_serializer<ExternalBuilder>::to_json(json & json, const ExternalBuilder & eb)
+{
+    json = {
+        {"systems", eb.systems},
+        {"program", eb.program},
+        {"args", eb.args},
+    };
+}
+
+} // namespace nlohmann

src/libstore/build/derivation-building-goal.cc

@@ -491,6 +491,8 @@ Goal::Co DerivationBuildingGoal::tryToBuild()
 
     bool useHook;
 
+    const ExternalBuilder * externalBuilder = nullptr;
+
     while (true) {
         trace("trying to build");
 
@@ -584,7 +586,42 @@ Goal::Co DerivationBuildingGoal::tryToBuild()
                 co_await waitForAWhile();
                 continue;
             case rpDecline:
-                /* We should do it ourselves. */
+                /* We should do it ourselves.
+
+                   Now that we've decided we can't / won't do a remote build, check
+                   that we can in fact build locally. First see if there is an
+                   external builder for a "semi-local build". If there is, prefer to
+                   use that. If there is not, then check if we can do a "true" local
+                   build. */
+
+                externalBuilder = settings.findExternalDerivationBuilderIfSupported(*drv);
+
+                if (!externalBuilder && !drvOptions->canBuildLocally(worker.store, *drv)) {
+                    auto msg =
+                        fmt("Cannot build '%s'.\n"
+                            "Reason: " ANSI_RED "required system or feature not available" ANSI_NORMAL
+                            "\n"
+                            "Required system: '%s' with features {%s}\n"
+                            "Current system: '%s' with features {%s}",
+                            Magenta(worker.store.printStorePath(drvPath)),
+                            Magenta(drv->platform),
+                            concatStringsSep(", ", drvOptions->getRequiredSystemFeatures(*drv)),
+                            Magenta(settings.thisSystem),
+                            concatStringsSep<StringSet>(", ", worker.store.Store::config.systemFeatures));
+
+                    // since aarch64-darwin has Rosetta 2, this user can actually run x86_64-darwin on their hardware -
+                    // we should tell them to run the command to install Darwin 2
+                    if (drv->platform == "x86_64-darwin" && settings.thisSystem == "aarch64-darwin")
+                        msg += fmt(
+                            "\nNote: run `%s` to run programs for x86_64-darwin",
+                            Magenta(
+                                "/usr/sbin/softwareupdate --install-rosetta && launchctl stop org.nixos.nix-daemon"));
+
+                    builder.reset();
+                    outputLocks.unlock();
+                    worker.permanentFailure = true;
+                    co_return doneFailure({BuildResult::Failure::InputRejected, std::move(msg)});
+                }
                 useHook = false;
                 break;
             }
@@ -771,36 +808,35 @@ Goal::Co DerivationBuildingGoal::tryToBuild()
                 co_return doneFailure(std::move(e));
             }
 
+            DerivationBuilderParams params{
+                .drvPath = drvPath,
+                .buildResult = buildResult,
+                .drv = *drv,
+                .drvOptions = *drvOptions,
+                .inputPaths = inputPaths,
+                .initialOutputs = initialOutputs,
+                .buildMode = buildMode,
+                .defaultPathsInChroot = std::move(defaultPathsInChroot),
+                .systemFeatures = worker.store.config.systemFeatures.get(),
+                .desugaredEnv = std::move(desugaredEnv),
+            };
+
             /* If we have to wait and retry (see below), then `builder` will
                already be created, so we don't need to create it again. */
-            builder = makeDerivationBuilder(
+            builder = externalBuilder ? makeExternalDerivationBuilder(
-                *localStoreP,
+                                            *localStoreP,
-                std::make_unique<DerivationBuildingGoalCallbacks>(*this, builder),
+                                            std::make_unique<DerivationBuildingGoalCallbacks>(*this, builder),
-                DerivationBuilderParams{
+                                            std::move(params),
-                    .drvPath = drvPath,
+                                            *externalBuilder)
-                    .buildResult = buildResult,
+                                      : makeDerivationBuilder(
-                    .drv = *drv,
+                                            *localStoreP,
-                    .drvOptions = *drvOptions,
+                                            std::make_unique<DerivationBuildingGoalCallbacks>(*this, builder),
-                    .inputPaths = inputPaths,
+                                            std::move(params));
-                    .initialOutputs = initialOutputs,
-                    .buildMode = buildMode,
-                    .defaultPathsInChroot = std::move(defaultPathsInChroot),
-                    .systemFeatures = worker.store.config.systemFeatures.get(),
-                    .desugaredEnv = std::move(desugaredEnv),
-                });
         }
 
-        std::optional<Descriptor> builderOutOpt;
+        if (auto builderOutOpt = builder->startBuild()) {
-        try {
+            builderOut = *std::move(builderOutOpt);
-            /* Okay, we have to build. */
+        } else {
-            builderOutOpt = builder->startBuild();
-        } catch (BuildError & e) {
-            builder.reset();
-            outputLocks.unlock();
-            worker.permanentFailure = true;
-            co_return doneFailure(std::move(e)); // InputRejected
-        }
-        if (!builderOutOpt) {
             if (!actLock)
                 actLock = std::make_unique<Activity>(
                     *logger,
@@ -809,9 +845,7 @@ Goal::Co DerivationBuildingGoal::tryToBuild()
                     fmt("waiting for a free build user ID for '%s'", Magenta(worker.store.printStorePath(drvPath))));
             co_await waitForAWhile();
             continue;
-        } else {
+        }
-            builderOut = *std::move(builderOutOpt);
-        };
 
         break;
     }

src/libstore/globals.cc

@@ -258,6 +258,15 @@ Path Settings::getDefaultSSLCertFile()
     return "";
 }
 
+const ExternalBuilder * Settings::findExternalDerivationBuilderIfSupported(const Derivation & drv)
+{
+    if (auto it = std::ranges::find_if(
+            externalBuilders.get(), [&](const auto & handler) { return handler.systems.contains(drv.platform); });
+        it != externalBuilders.get().end())
+        return &*it;
+    return nullptr;
+}
+
 std::string nixVersion = PACKAGE_VERSION;
 
 NLOHMANN_JSON_SERIALIZE_ENUM(
@@ -379,8 +388,6 @@ unsigned int MaxBuildJobsSetting::parse(const std::string & str) const
     }
 }
 
-NLOHMANN_DEFINE_TYPE_NON_INTRUSIVE(Settings::ExternalBuilder, systems, program, args);
-
 template<>
 Settings::ExternalBuilders BaseSetting<Settings::ExternalBuilders>::parse(const std::string & str) const
 {

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

@@ -1,12 +1,15 @@
 #pragma once
 ///@file
 
+#include <nlohmann/json_fwd.hpp>
+
 #include "nix/store/build-result.hh"
 #include "nix/store/derivation-options.hh"
 #include "nix/store/build/derivation-building-misc.hh"
 #include "nix/store/derivations.hh"
 #include "nix/store/parsed-derivations.hh"
 #include "nix/util/processes.hh"
+#include "nix/util/json-impls.hh"
 #include "nix/store/restricted-store.hh"
 #include "nix/store/build/derivation-env-desugar.hh"
 
@@ -179,9 +182,28 @@ struct DerivationBuilder : RestrictionContext
     virtual bool killChild() = 0;
 };
 
+struct ExternalBuilder
+{
+    StringSet systems;
+    Path program;
+    std::vector<std::string> args;
+};
+
 #ifndef _WIN32 // TODO enable `DerivationBuilder` on Windows
 std::unique_ptr<DerivationBuilder> makeDerivationBuilder(
     LocalStore & store, std::unique_ptr<DerivationBuilderCallbacks> miscMethods, DerivationBuilderParams params);
+
+/**
+ * @param handler Must be chosen such that it supports the given
+ * derivation.
+ */
+std::unique_ptr<DerivationBuilder> makeExternalDerivationBuilder(
+    LocalStore & store,
+    std::unique_ptr<DerivationBuilderCallbacks> miscMethods,
+    DerivationBuilderParams params,
+    const ExternalBuilder & handler);
 #endif
 
 } // namespace nix
+
+JSON_IMPL(nix::ExternalBuilder)

src/libstore/include/nix/store/globals.hh

@@ -1373,13 +1373,6 @@ public:
           Set it to 1 to warn on all paths.
         )"};
 
-    struct ExternalBuilder
-    {
-        std::vector<std::string> systems;
-        Path program;
-        std::vector<std::string> args;
-    };
-
     using ExternalBuilders = std::vector<ExternalBuilder>;
 
     Setting<ExternalBuilders> externalBuilders{
@@ -1443,6 +1436,12 @@ public:
         //        Current system: 'aarch64-darwin' with features {apple-virt, benchmark, big-parallel, nixos-test}
         // Xp::ExternalBuilders
     };
+
+    /**
+     * Finds the first external derivation builder that supports this
+     * derivation, or else returns a null pointer.
+     */
+    const ExternalBuilder * findExternalDerivationBuilderIfSupported(const Derivation & drv);
 };
 
 // FIXME: don't use a global variable.

src/libstore/meson.build

@@ -298,6 +298,7 @@ sources = files(
   'aws-creds.cc',
   'binary-cache-store.cc',
   'build-result.cc',
+  'build/derivation-builder.cc',
   'build/derivation-building-goal.cc',
   'build/derivation-check.cc',
   'build/derivation-env-desugar.cc',

src/libstore/unix/build/derivation-builder.cc

@@ -229,12 +229,6 @@ protected:
         return acquireUserLock(1, false);
     }
 
-    /**
-     * Throw an exception if we can't do this derivation because of
-     * missing system features.
-     */
-    virtual void checkSystem();
-
     /**
      * Return the paths that should be made available in the sandbox.
      * This includes:
@@ -672,33 +666,6 @@ static bool checkNotWorldWritable(std::filesystem::path path)
     return true;
 }
 
-void DerivationBuilderImpl::checkSystem()
-{
-    /* Right platform? */
-    if (!drvOptions.canBuildLocally(store, drv)) {
-        auto msg =
-            fmt("Cannot build '%s'.\n"
-                "Reason: " ANSI_RED "required system or feature not available" ANSI_NORMAL
-                "\n"
-                "Required system: '%s' with features {%s}\n"
-                "Current system: '%s' with features {%s}",
-                Magenta(store.printStorePath(drvPath)),
-                Magenta(drv.platform),
-                concatStringsSep(", ", drvOptions.getRequiredSystemFeatures(drv)),
-                Magenta(settings.thisSystem),
-                concatStringsSep<StringSet>(", ", store.Store::config.systemFeatures));
-
-        // since aarch64-darwin has Rosetta 2, this user can actually run x86_64-darwin on their hardware - we should
-        // tell them to run the command to install Darwin 2
-        if (drv.platform == "x86_64-darwin" && settings.thisSystem == "aarch64-darwin")
-            msg +=
-                fmt("\nNote: run `%s` to run programs for x86_64-darwin",
-                    Magenta("/usr/sbin/softwareupdate --install-rosetta && launchctl stop org.nixos.nix-daemon"));
-
-        throw BuildError(BuildResult::Failure::InputRejected, msg);
-    }
-}
-
 std::optional<Descriptor> DerivationBuilderImpl::startBuild()
 {
     if (useBuildUsers()) {
@@ -709,8 +676,6 @@ std::optional<Descriptor> DerivationBuilderImpl::startBuild()
             return std::nullopt;
     }
 
-    checkSystem();
-
     /* Make sure that no other processes are executing under the
        sandbox uids. This must be done before any chownToBuilder()
        calls. */
@@ -1922,9 +1887,6 @@ namespace nix {
 std::unique_ptr<DerivationBuilder> makeDerivationBuilder(
     LocalStore & store, std::unique_ptr<DerivationBuilderCallbacks> miscMethods, DerivationBuilderParams params)
 {
-    if (auto builder = ExternalDerivationBuilder::newIfSupported(store, miscMethods, params))
-        return builder;
-
     bool useSandbox = false;
 
     /* Are we doing a sandboxed build? */

src/libstore/unix/build/external-derivation-builder.cc

@@ -2,31 +2,19 @@ namespace nix {
 
 struct ExternalDerivationBuilder : DerivationBuilderImpl
 {
-    Settings::ExternalBuilder externalBuilder;
+    ExternalBuilder externalBuilder;
 
     ExternalDerivationBuilder(
         LocalStore & store,
         std::unique_ptr<DerivationBuilderCallbacks> miscMethods,
         DerivationBuilderParams params,
-        Settings::ExternalBuilder externalBuilder)
+        ExternalBuilder externalBuilder)
         : DerivationBuilderImpl(store, std::move(miscMethods), std::move(params))
         , externalBuilder(std::move(externalBuilder))
     {
         experimentalFeatureSettings.require(Xp::ExternalBuilders);
     }
 
-    static std::unique_ptr<ExternalDerivationBuilder> newIfSupported(
-        LocalStore & store, std::unique_ptr<DerivationBuilderCallbacks> & miscMethods, DerivationBuilderParams & params)
-    {
-        for (auto & handler : settings.externalBuilders.get()) {
-            for (auto & system : handler.systems)
-                if (params.drv.platform == system)
-                    return std::make_unique<ExternalDerivationBuilder>(
-                        store, std::move(miscMethods), std::move(params), handler);
-        }
-        return {};
-    }
-
     Path tmpDirInSandbox() override
     {
         /* In a sandbox, for determinism, always use the same temporary
@@ -40,8 +28,6 @@ struct ExternalDerivationBuilder : DerivationBuilderImpl
         createDir(tmpDir, 0700);
     }
 
-    void checkSystem() override {}
-
     void startChild() override
     {
         if (drvOptions.getRequiredSystemFeatures(drv).count("recursive-nix"))
@@ -120,4 +106,13 @@ struct ExternalDerivationBuilder : DerivationBuilderImpl
     }
 };
 
+std::unique_ptr<DerivationBuilder> makeExternalDerivationBuilder(
+    LocalStore & store,
+    std::unique_ptr<DerivationBuilderCallbacks> miscMethods,
+    DerivationBuilderParams params,
+    const ExternalBuilder & handler)
+{
+    return std::make_unique<ExternalDerivationBuilder>(store, std::move(miscMethods), std::move(params), handler);
+}
+
 } // namespace nix