From 4720853129b6866775edd9f90ad6f10701f98a3c Mon Sep 17 00:00:00 2001 From: John Ericson Date: Wed, 12 Aug 2020 16:32:36 +0000 Subject: [PATCH] Make `system-features` a store setting This seems more correct. It also means one can specify the features a store should support with --store and remote-store=..., which is useful. I use this to clean up the build remotes test. --- src/build-remote/build-remote.cc | 16 ++++----------- src/libstore/build.cc | 8 ++++---- src/libstore/machines.cc | 24 ++++++++++++++++++++++ src/libstore/machines.hh | 4 ++++ src/libstore/parsed-derivations.cc | 8 ++++---- src/libstore/parsed-derivations.hh | 4 ++-- src/libstore/store-api.hh | 4 ++++ tests/build-hook.nix | 3 ++- tests/build-remote.sh | 33 +++++++++++++++++------------- tests/local.mk | 2 +- 10 files changed, 68 insertions(+), 38 deletions(-) diff --git a/src/build-remote/build-remote.cc b/src/build-remote/build-remote.cc index cec7b369b..ce5127113 100644 --- a/src/build-remote/build-remote.cc +++ b/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& requiredFeatures) { +static bool allSupportedLocally(Store & store, const std::set& 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; diff --git a/src/libstore/build.cc b/src/libstore/build.cc index 76baa1a6e..d6e6ad6a9 100644 --- a/src/libstore/build.cc +++ b/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(", ", settings.systemFeatures)); + concatStringsSep(", ", worker.store.systemFeatures)); if (drv->isBuiltin()) preloadNSS(); @@ -3179,7 +3179,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"); diff --git a/src/libstore/machines.cc b/src/libstore/machines.cc index f848582da..7db2556f4 100644 --- a/src/libstore/machines.cc +++ b/src/libstore/machines.cc @@ -1,6 +1,7 @@ #include "machines.hh" #include "util.hh" #include "globals.hh" +#include "store-api.hh" #include @@ -48,6 +49,29 @@ bool Machine::mandatoryMet(const std::set & features) const { }); } +ref 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>(s, "\n;")) { diff --git a/src/libstore/machines.hh b/src/libstore/machines.hh index de92eb924..341d9bd97 100644 --- a/src/libstore/machines.hh +++ b/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 openStore() const; }; typedef std::vector Machines; diff --git a/src/libstore/parsed-derivations.cc b/src/libstore/parsed-derivations.cc index 24f848e46..e7b7202d4 100644 --- a/src/libstore/parsed-derivations.cc +++ b/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 diff --git a/src/libstore/parsed-derivations.hh b/src/libstore/parsed-derivations.hh index 6ee172d81..3fa09f34f 100644 --- a/src/libstore/parsed-derivations.hh +++ b/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; }; diff --git a/src/libstore/store-api.hh b/src/libstore/store-api.hh index e94d975c5..1e940e6a8 100644 --- a/src/libstore/store-api.hh +++ b/src/libstore/store-api.hh @@ -164,6 +164,10 @@ public: Setting wantMassQuery{this, false, "want-mass-query", "whether this substituter can be queried efficiently for path validity"}; + Setting systemFeatures{this, settings.systemFeatures, + "system-features", + "Optional features that the system this store builds on implements (like \"kvm\")."}; + protected: struct PathInfoCacheValue { diff --git a/tests/build-hook.nix b/tests/build-hook.nix index a19c10dde..1bd0b759f 100644 --- a/tests/build-hook.nix +++ b/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 ''; } diff --git a/tests/build-remote.sh b/tests/build-remote.sh index 4dfb753e1..7638f536f 100644 --- a/tests/build-remote.sh +++ b/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 diff --git a/tests/local.mk b/tests/local.mk index 0f3bfe606..5c77b9bb7 100644 --- a/tests/local.mk +++ b/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 \