fix: good errors for failures caused by allowSubstitutes

This caused an absolute saga which I would not like anyone else to have
to experience. Let's put in a laser targeted error message that
diagnoses this exact problem.

Fixes: lix-project/lix#484
Change-Id: I2a79f04aeb4a1b67c10115e5e39501d958836298

doc/manual/rl-next/allowsubstitutes-errors.md

@@ -0,0 +1,21 @@
+---
+synopsis: "Build failures caused by `allowSubstitutes = false` while being the wrong system now produce a decent error"
+issues: [fj#484]
+cls: [1841]
+category: Fixes
+credits: jade
+---
+
+Nix allows derivations to set `allowSubstitutes = false` in order to force them to be built locally without querying substituters for them.
+This is useful for derivations that are very fast to build (especially if they produce large output).
+However, this can shoot you in the foot if the derivation *has* to be substituted such as if the derivation is for another architecture, which is what `--always-allow-substitutes` is for.
+
+Perhaps such derivations that are known to be impossible to build locally should ignore `allowSubstitutes` (irrespective of remote builders) in the future, but this at least reports the failure and solution directly.
+
+```
+$ nix build -f fail.nix
+error: a 'unicornsandrainbows-linux' with features {} is required to build '/nix/store/...-meow.drv', but I am a 'x86_64-linux' with features {...}
+
+       Hint: the failing derivation has allowSubstitutes set to false, forcing it to be built rather than substituted.
+       Passing --always-allow-substitutes to force substitution may resolve this failure if the path is available in a substituter.
+```

src/libstore/build/derivation-goal.cc

@@ -261,24 +261,29 @@ Goal::WorkResult DerivationGoal::haveDerivation(bool inBuildSlot)
        through substitutes.  If that doesn't work, we'll build
        them. */
     WaitForGoals result;
-    if (settings.useSubstitutes && parsedDrv->substitutesAllowed())
+    if (settings.useSubstitutes) {
-        for (auto & [outputName, status] : initialOutputs) {
+        if (parsedDrv->substitutesAllowed()) {
-            if (!status.wanted) continue;
+            for (auto & [outputName, status] : initialOutputs) {
-            if (!status.known)
+                if (!status.wanted) continue;
-                result.goals.insert(
+                if (!status.known)
-                    worker.makeDrvOutputSubstitutionGoal(
+                    result.goals.insert(
-                        DrvOutput{status.outputHash, outputName},
+                        worker.makeDrvOutputSubstitutionGoal(
-                        buildMode == bmRepair ? Repair : NoRepair
+                            DrvOutput{status.outputHash, outputName},
-                    )
+                            buildMode == bmRepair ? Repair : NoRepair
-                );
+                        )
-            else {
+                    );
-                auto * cap = getDerivationCA(*drv);
+                else {
-                result.goals.insert(worker.makePathSubstitutionGoal(
+                    auto * cap = getDerivationCA(*drv);
-                    status.known->path,
+                    result.goals.insert(worker.makePathSubstitutionGoal(
-                    buildMode == bmRepair ? Repair : NoRepair,
+                        status.known->path,
-                    cap ? std::optional { *cap } : std::nullopt));
+                        buildMode == bmRepair ? Repair : NoRepair,
+                        cap ? std::optional { *cap } : std::nullopt));
+                }
             }
+        } else {
+            trace("skipping substitute because allowSubstitutes is false");
         }
+    }
 
     if (result.goals.empty()) { /* to prevent hang (no wake-up event) */
         return outputsSubstitutionTried(inBuildSlot);

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

@@ -436,13 +436,23 @@ std::set<int> LocalDerivationGoal::startBuilder()
     killSandbox(false);
 
     /* Right platform? */
-    if (!parsedDrv->canBuildLocally(worker.store))
+    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}",
+        HintFmt addendum{""};
-            drv->platform,
+        if (settings.useSubstitutes && !parsedDrv->substitutesAllowed()) {
-            concatStringsSep(", ", parsedDrv->getRequiredSystemFeatures()),
+            addendum = HintFmt("\n\nHint: the failing derivation has %s set to %s, forcing it to be built rather than substituted.\n"
-            worker.store.printStorePath(drvPath),
+                "Passing %s to force substitution may resolve this failure if the path is available in a substituter.",
-            settings.thisSystem,
+                "allowSubstitutes", "false", "--always-allow-substitutes");
-            concatStringsSep<StringSet>(", ", worker.store.systemFeatures));
+        }
+        throw Error({
+            .msg = HintFmt("a '%s' with features {%s} is required to build '%s', but I am a '%s' with features {%s}%s",
+                drv->platform,
+                concatStringsSep(", ", parsedDrv->getRequiredSystemFeatures()),
+                worker.store.printStorePath(drvPath),
+                settings.thisSystem,
+                concatStringsSep<StringSet>(", ", worker.store.systemFeatures),
+                Uncolored(addendum))
+        });
+    }
 
     /* Create a temporary directory where the build will take
        place. */

tests/functional/meson.build

@@ -189,6 +189,7 @@ functional_tests_scripts = [
   'test-libstoreconsumer.sh',
   'extra-sandbox-profile.sh',
   'substitute-truncated-nar.sh',
+  'regression-484.sh',
 ]
 
 # Plugin tests require shared libraries support.

tests/functional/regression-484.sh

@@ -0,0 +1,41 @@
+source common.sh
+
+# Ensures that nix build will deliver a usable error message when it encounters
+# a build failure potentially caused by allowSubstitutes.
+
+clearStore
+
+cd $TEST_HOME
+
+putDrv() {
+    cat > "$1" <<EOF
+builtins.derivation {
+  name = "meow";
+  builder = "/bin/sh";
+  args = [];
+  system = "unicornsandrainbows-linux";
+  allowSubstitutes = ${2};
+  preferLocalBuild = true;
+}
+EOF
+}
+
+putDrv drv-disallow-substitutes.nix false
+putDrv drv-allow-substitutes.nix true
+
+expect 1 nix build --substitute --substituters '' --offline -f drv-disallow-substitutes.nix 2> output.txt
+
+# error: a 'unicornsandrainbows-linux' with features {} is required to build '$TMPDIR/regression-484/store/...-meow.drv', but I am a 'x86_64-linux' with features {benchmark, big-parallel, kvm, nixos-test, uid-range}
+#
+#        Hint: the failing derivation has allowSubstitutes set to false, forcing it to be built rather than substituted.
+#        Passing --always-allow-substitutes to force substitution may resolve this failure if the path is available in a substituter.
+cat output.txt
+grepQuiet unicornsandrainbows-linux output.txt
+grepQuiet always-allow-substitutes output.txt
+grepQuiet allowSubstitutes output.txt
+
+expect 1 nix build --substitute --substituters '' --offline -f drv-allow-substitutes.nix 2> output.txt
+
+cat output.txt
+grepQuiet unicornsandrainbows-linux output.txt
+grepQuiet -v allowSubstitutes output.txt