Merge pull request #3924 from obsidiansystems/features-per-store

Make `system-features` a store setting

src/build-remote/build-remote.cc

@@ -38,9 +38,9 @@ static AutoCloseFD openSlotLock(const Machine & m, uint64_t slot)
     return openLockFile(fmt("%s/%s-%d", currentLoad, escapeUri(m.storeUri), slot), true);
 }
 
-static bool allSupportedLocally(const std::set<std::string>& requiredFeatures) {
+static bool allSupportedLocally(Store & store, const std::set<std::string>& requiredFeatures) {
     for (auto & feature : requiredFeatures)
-        if (!settings.systemFeatures.get().count(feature)) return false;
+        if (!store.systemFeatures.get().count(feature)) return false;
     return true;
 }
 
@@ -106,7 +106,7 @@ static int _main(int argc, char * * argv)
             auto canBuildLocally = amWilling
                  &&  (  neededSystem == settings.thisSystem
                      || settings.extraPlatforms.get().count(neededSystem) > 0)
-                 &&  allSupportedLocally(requiredFeatures);
+                 &&  allSupportedLocally(*store, requiredFeatures);
 
             /* Error ignored here, will be caught later */
             mkdir(currentLoad.c_str(), 0777);
@@ -224,15 +224,7 @@ static int _main(int argc, char * * argv)
 
                     Activity act(*logger, lvlTalkative, actUnknown, fmt("connecting to '%s'", bestMachine->storeUri));
 
-                    Store::Params storeParams;
-                    if (hasPrefix(bestMachine->storeUri, "ssh://")) {
-                        storeParams["max-connections"] = "1";
-                        storeParams["log-fd"] = "4";
-                        if (bestMachine->sshKey != "")
-                            storeParams["ssh-key"] = bestMachine->sshKey;
-                    }
-
-                    sshStore = openStore(bestMachine->storeUri, storeParams);
+                    sshStore = bestMachine->openStore();
                     sshStore->connect();
                     storeUri = bestMachine->storeUri;
 

src/libstore/build.cc

@@ -1475,7 +1475,7 @@ void DerivationGoal::tryToBuild()
     /* Don't do a remote build if the derivation has the attribute
        `preferLocalBuild' set.  Also, check and repair modes are only
        supported for local builds. */
-    bool buildLocally = buildMode != bmNormal || parsedDrv->willBuildLocally();
+    bool buildLocally = buildMode != bmNormal || parsedDrv->willBuildLocally(worker.store);
 
     /* Is the build hook willing to accept this job? */
     if (!buildLocally) {
@@ -1964,13 +1964,13 @@ void linkOrCopy(const Path & from, const Path & to)
 void DerivationGoal::startBuilder()
 {
     /* Right platform? */
-    if (!parsedDrv->canBuildLocally())
+    if (!parsedDrv->canBuildLocally(worker.store))
         throw Error("a '%s' with features {%s} is required to build '%s', but I am a '%s' with features {%s}",
             drv->platform,
             concatStringsSep(", ", parsedDrv->getRequiredSystemFeatures()),
             worker.store.printStorePath(drvPath),
             settings.thisSystem,
-            concatStringsSep<StringSet>(", ", settings.systemFeatures));
+            concatStringsSep<StringSet>(", ", worker.store.systemFeatures));
 
     if (drv->isBuiltin())
         preloadNSS();
@@ -3180,7 +3180,7 @@ void DerivationGoal::runChild()
                 createDirs(chrootRootDir + "/dev/shm");
                 createDirs(chrootRootDir + "/dev/pts");
                 ss.push_back("/dev/full");
-                if (settings.systemFeatures.get().count("kvm") && pathExists("/dev/kvm"))
+                if (worker.store.systemFeatures.get().count("kvm") && pathExists("/dev/kvm"))
                     ss.push_back("/dev/kvm");
                 ss.push_back("/dev/null");
                 ss.push_back("/dev/random");

src/libstore/machines.cc

@@ -1,6 +1,7 @@
 #include "machines.hh"
 #include "util.hh"
 #include "globals.hh"
+#include "store-api.hh"
 
 #include <algorithm>
 
@@ -48,6 +49,29 @@ bool Machine::mandatoryMet(const std::set<string> & features) const {
         });
 }
 
+ref<Store> Machine::openStore() const {
+    Store::Params storeParams;
+    if (hasPrefix(storeUri, "ssh://")) {
+        storeParams["max-connections"] = "1";
+        storeParams["log-fd"] = "4";
+        if (sshKey != "")
+            storeParams["ssh-key"] = sshKey;
+    }
+    {
+        auto & fs = storeParams["system-features"];
+        auto append = [&](auto feats) {
+            for (auto & f : feats) {
+                if (fs.size() > 0) fs += ' ';
+                fs += f;
+            }
+        };
+        append(supportedFeatures);
+        append(mandatoryFeatures);
+    }
+
+    return nix::openStore(storeUri, storeParams);
+}
+
 void parseMachines(const std::string & s, Machines & machines)
 {
     for (auto line : tokenizeString<std::vector<string>>(s, "\n;")) {

src/libstore/machines.hh

@@ -4,6 +4,8 @@
 
 namespace nix {
 
+class Store;
+
 struct Machine {
 
     const string storeUri;
@@ -28,6 +30,8 @@ struct Machine {
         decltype(supportedFeatures) supportedFeatures,
         decltype(mandatoryFeatures) mandatoryFeatures,
         decltype(sshPublicHostKey) sshPublicHostKey);
+
+    ref<Store> openStore() const;
 };
 
 typedef std::vector<Machine> Machines;

src/libstore/parsed-derivations.cc

@@ -94,7 +94,7 @@ StringSet ParsedDerivation::getRequiredSystemFeatures() const
     return res;
 }
 
-bool ParsedDerivation::canBuildLocally() const
+bool ParsedDerivation::canBuildLocally(Store & localStore) const
 {
     if (drv.platform != settings.thisSystem.get()
         && !settings.extraPlatforms.get().count(drv.platform)
@@ -102,14 +102,14 @@ bool ParsedDerivation::canBuildLocally() const
         return false;
 
     for (auto & feature : getRequiredSystemFeatures())
-        if (!settings.systemFeatures.get().count(feature)) return false;
+        if (!localStore.systemFeatures.get().count(feature)) return false;
 
     return true;
 }
 
-bool ParsedDerivation::willBuildLocally() const
+bool ParsedDerivation::willBuildLocally(Store & localStore) const
 {
-    return getBoolAttr("preferLocalBuild") && canBuildLocally();
+    return getBoolAttr("preferLocalBuild") && canBuildLocally(localStore);
 }
 
 bool ParsedDerivation::substitutesAllowed() const

src/libstore/parsed-derivations.hh

@@ -29,9 +29,9 @@ public:
 
     StringSet getRequiredSystemFeatures() const;
 
-    bool canBuildLocally() const;
+    bool canBuildLocally(Store & localStore) const;
 
-    bool willBuildLocally() const;
+    bool willBuildLocally(Store & localStore) const;
 
     bool substitutesAllowed() const;
 };

src/libstore/store-api.hh

@@ -163,6 +163,10 @@ public:
 
     Setting<bool> wantMassQuery{this, false, "want-mass-query", "whether this substituter can be queried efficiently for path validity"};
 
+    Setting<StringSet> systemFeatures{this, settings.systemFeatures,
+        "system-features",
+        "Optional features that the system this store builds on implements (like \"kvm\")."};
+
 protected:
 
     struct PathInfoCacheValue {

tests/build-hook.nix

@@ -23,6 +23,7 @@ let
     shell = busybox;
     name = "build-remote-input-2";
     buildCommand = "echo BAR > $out";
+    requiredSystemFeatures = ["bar"];
   };
 
 in
@@ -34,6 +35,6 @@ in
       ''
         read x < ${input1}
         read y < ${input2}
-        echo $x$y > $out
+        echo "$x $y" > $out
       '';
   }

tests/build-remote.sh

@@ -1,31 +1,36 @@
 source common.sh
 
-clearStore
-
 if ! canUseSandbox; then exit; fi
 if ! [[ $busybox =~ busybox ]]; then exit; fi
 
-chmod -R u+w $TEST_ROOT/machine0 || true
-chmod -R u+w $TEST_ROOT/machine1 || true
-chmod -R u+w $TEST_ROOT/machine2 || true
-rm -rf $TEST_ROOT/machine0 $TEST_ROOT/machine1 $TEST_ROOT/machine2
-rm -f $TEST_ROOT/result
-
 unset NIX_STORE_DIR
 unset NIX_STATE_DIR
 
+function join_by { local d=$1; shift; echo -n "$1"; shift; printf "%s" "${@/#/$d}"; }
+
+builders=(
+  # system-features will automatically be added to the outer URL, but not inner
+  # remote-store URL.
+  "ssh://localhost?remote-store=$TEST_ROOT/machine1?system-features=foo - - 1 1 foo"
+  "$TEST_ROOT/machine2 - - 1 1 bar"
+)
+
 # Note: ssh://localhost bypasses ssh, directly invoking nix-store as a
 # child process. This allows us to test LegacySSHStore::buildDerivation().
+# ssh-ng://... likewise allows us to test RemoteStore::buildDerivation().
 nix build -L -v -f build-hook.nix -o $TEST_ROOT/result --max-jobs 0 \
   --arg busybox $busybox \
   --store $TEST_ROOT/machine0 \
-  --builders "ssh://localhost?remote-store=$TEST_ROOT/machine1; $TEST_ROOT/machine2 - - 1 1 foo" \
-  --system-features foo
+  --builders "$(join_by '; ' "${builders[@]}")"
 
 outPath=$(readlink -f $TEST_ROOT/result)
 
-cat $TEST_ROOT/machine0/$outPath | grep FOOBAR
+grep 'FOO BAR' $TEST_ROOT/machine0/$outPath
 
-# Ensure that input1 was built on store2 due to the required feature.
-(! nix path-info --store $TEST_ROOT/machine1 --all | grep builder-build-remote-input-1.sh)
-nix path-info --store $TEST_ROOT/machine2 --all | grep builder-build-remote-input-1.sh
+# Ensure that input1 was built on store1 due to the required feature.
+(! nix path-info --store $TEST_ROOT/machine2 --all | grep builder-build-remote-input-1.sh)
+nix path-info --store $TEST_ROOT/machine1 --all | grep builder-build-remote-input-1.sh
+
+# Ensure that input2 was built on store2 due to the required feature.
+(! nix path-info --store $TEST_ROOT/machine1 --all | grep builder-build-remote-input-2.sh)
+nix path-info --store $TEST_ROOT/machine2 --all | grep builder-build-remote-input-2.sh

tests/local.mk

@@ -1,5 +1,5 @@
 nix_tests = \
-  init.sh hash.sh lang.sh add.sh simple.sh dependencies.sh \
+  hash.sh lang.sh add.sh simple.sh dependencies.sh \
   config.sh \
   gc.sh \
   gc-concurrent.sh \