From 1e7b8deea7e052ed9ebf47d1411bcaf542054b41 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Fri, 28 Sep 2018 15:57:27 +0200 Subject: [PATCH] Check requiredSystemFeatures for local builds For example, this prevents a "kvm" build on machines that don't have KVM. Fixes #2012. --- doc/manual/command-ref/conf-file.xml | 27 ++++++++++++++++++++++ doc/manual/release-notes/release-notes.xml | 1 + src/libstore/build.cc | 21 ++++++++--------- src/libstore/globals.cc | 15 ++++++++++++ src/libstore/globals.hh | 6 +++++ src/libstore/parsed-derivations.cc | 20 +++++++++++++--- src/libstore/parsed-derivations.hh | 2 ++ tests/build-remote.sh | 3 ++- 8 files changed, 79 insertions(+), 16 deletions(-) diff --git a/doc/manual/command-ref/conf-file.xml b/doc/manual/command-ref/conf-file.xml index fd09883be..e9947ebc6 100644 --- a/doc/manual/command-ref/conf-file.xml +++ b/doc/manual/command-ref/conf-file.xml @@ -757,6 +757,33 @@ password my-password + system-features + + A set of system “features” supported by this + machine, e.g. kvm. Derivations can express a + dependency on such features through the derivation attribute + requiredSystemFeatures. For example, the + attribute + + +requiredSystemFeatures = [ "kvm" ]; + + + ensures that the derivation can only be built on a machine with + the kvm feature. + + This setting by default includes kvm if + /dev/kvm is accessible, and the + pseudo-features nixos-test, + benchmark and big-parallel + that are used in Nixpkgs to route builds to specific + machines. + + + + + + timeout diff --git a/doc/manual/release-notes/release-notes.xml b/doc/manual/release-notes/release-notes.xml index ff4085cb7..e8ff586fa 100644 --- a/doc/manual/release-notes/release-notes.xml +++ b/doc/manual/release-notes/release-notes.xml @@ -12,6 +12,7 @@ --> + diff --git a/src/libstore/build.cc b/src/libstore/build.cc index eb7f106a2..0073b9b72 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -1646,18 +1646,13 @@ HookReply DerivationGoal::tryBuildHook() try { - /* Tell the hook about system features (beyond the system type) - required from the build machine. (The hook could parse the - drv file itself, but this is easier.) */ - auto features = parsedDrv->getStringsAttr("requiredSystemFeatures").value_or(Strings()); - /* Send the request to the hook. */ worker.hook->sink << "try" << (worker.getNrLocalBuilds() < settings.maxBuildJobs ? 1 : 0) << drv->platform << drvPath - << features; + << parsedDrv->getRequiredSystemFeatures(); worker.hook->sink.flush(); /* Read the first line of input, which should be a word indicating @@ -1797,11 +1792,13 @@ static void preloadNSS() { void DerivationGoal::startBuilder() { /* Right platform? */ - if (!parsedDrv->canBuildLocally()) { - throw Error( - format("a '%1%' is required to build '%3%', but I am a '%2%'") - % drv->platform % settings.thisSystem % drvPath); - } + if (!parsedDrv->canBuildLocally()) + 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()), + drvPath, + settings.thisSystem, + concatStringsSep(", ", settings.systemFeatures)); if (drv->isBuiltin()) preloadNSS(); @@ -2625,7 +2622,7 @@ void DerivationGoal::runChild() createDirs(chrootRootDir + "/dev/shm"); createDirs(chrootRootDir + "/dev/pts"); ss.push_back("/dev/full"); - if (pathExists("/dev/kvm")) + if (settings.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/globals.cc b/src/libstore/globals.cc index d95db5672..a9c07b23a 100644 --- a/src/libstore/globals.cc +++ b/src/libstore/globals.cc @@ -86,6 +86,21 @@ unsigned int Settings::getDefaultCores() return std::max(1U, std::thread::hardware_concurrency()); } +StringSet Settings::getDefaultSystemFeatures() +{ + /* For backwards compatibility, accept some "features" that are + used in Nixpkgs to route builds to certain machines but don't + actually require anything special on the machines. */ + StringSet features{"nixos-test", "benchmark", "big-parallel"}; + + #if __linux__ + if (access("/dev/kvm", R_OK | W_OK) == 0) + features.insert("kvm"); + #endif + + return features; +} + const string nixVersion = PACKAGE_VERSION; template<> void BaseSetting::set(const std::string & str) diff --git a/src/libstore/globals.hh b/src/libstore/globals.hh index f589078db..cf4ae63cd 100644 --- a/src/libstore/globals.hh +++ b/src/libstore/globals.hh @@ -32,6 +32,8 @@ class Settings : public Config { unsigned int getDefaultCores(); + StringSet getDefaultSystemFeatures(); + public: Settings(); @@ -261,6 +263,10 @@ public: "These may be supported natively (e.g. armv7 on some aarch64 CPUs " "or using hacks like qemu-user."}; + Setting systemFeatures{this, getDefaultSystemFeatures(), + "system-features", + "Optional features that this system implements (like \"kvm\")."}; + Setting substituters{this, nixStore == "/nix/store" ? Strings{"https://cache.nixos.org/"} : Strings(), "substituters", diff --git a/src/libstore/parsed-derivations.cc b/src/libstore/parsed-derivations.cc index 0d7acf046..dc3286482 100644 --- a/src/libstore/parsed-derivations.cc +++ b/src/libstore/parsed-derivations.cc @@ -82,11 +82,25 @@ std::experimental::optional ParsedDerivation::getStringsAttr(const std: } } +StringSet ParsedDerivation::getRequiredSystemFeatures() const +{ + StringSet res; + for (auto & i : getStringsAttr("requiredSystemFeatures").value_or(Strings())) + res.insert(i); + return res; +} + bool ParsedDerivation::canBuildLocally() const { - return drv.platform == settings.thisSystem - || settings.extraPlatforms.get().count(drv.platform) > 0 - || drv.isBuiltin(); + if (drv.platform != settings.thisSystem.get() + && !settings.extraPlatforms.get().count(drv.platform) + && !drv.isBuiltin()) + return false; + + for (auto & feature : getRequiredSystemFeatures()) + if (!settings.systemFeatures.get().count(feature)) return false; + + return true; } bool ParsedDerivation::willBuildLocally() const diff --git a/src/libstore/parsed-derivations.hh b/src/libstore/parsed-derivations.hh index 0c7dc32e1..0a82c1461 100644 --- a/src/libstore/parsed-derivations.hh +++ b/src/libstore/parsed-derivations.hh @@ -25,6 +25,8 @@ public: std::experimental::optional getStringsAttr(const std::string & name) const; + StringSet getRequiredSystemFeatures() const; + bool canBuildLocally() const; bool willBuildLocally() const; diff --git a/tests/build-remote.sh b/tests/build-remote.sh index 9bca0f4a3..ddd68f327 100644 --- a/tests/build-remote.sh +++ b/tests/build-remote.sh @@ -11,7 +11,8 @@ rm -rf $TEST_ROOT/store0 $TEST_ROOT/store1 nix build -f build-hook.nix -o $TEST_ROOT/result --max-jobs 0 \ --sandbox-paths /nix/store --sandbox-build-dir /build-tmp \ - --builders "$TEST_ROOT/store0; $TEST_ROOT/store1 - - 1 1 foo" + --builders "$TEST_ROOT/store0; $TEST_ROOT/store1 - - 1 1 foo" \ + --system-features foo outPath=$TEST_ROOT/result