From a8da0df671d93162c1691f38f791a9194401e068 Mon Sep 17 00:00:00 2001
From: Tom Hubrecht <github@mail.hubrecht.ovh>
Date: Tue, 4 Mar 2025 10:49:36 +0100
Subject: [PATCH] libstore: Show illegal path references in fixed-outputs
 derivations

This allows debugging the errors by pinpointing the offenders, instead
of trying to blindly guess what could have possibly gone wrong.

The wording has been improved to better explain the failure.

Closes #530

Co-authored-by: Ben Millwood <thebenmachine+git@gmail.com>
Change-Id: I84ba5d2d81e5d1867f53bd3bc80e615cab9fe274
---
 doc/manual/change-authors.yml                 |  5 +++
 doc/manual/rl-next/illegal-path-references.md | 43 +++++++++++++++++++
 lix/libstore/build/local-derivation-goal.cc   | 19 ++++++--
 tests/functional/fixed.nix                    | 12 ++++++
 tests/functional/fixed.sh                     |  7 +++
 5 files changed, 82 insertions(+), 4 deletions(-)
 create mode 100644 doc/manual/rl-next/illegal-path-references.md

diff --git a/doc/manual/change-authors.yml b/doc/manual/change-authors.yml
index d62684598..dbc3f9348 100644
--- a/doc/manual/change-authors.yml
+++ b/doc/manual/change-authors.yml
@@ -168,6 +168,11 @@ teofilc:
   forgejo: teofilc
   github: TeofilC
 
+thubrecht:
+  display_name: Tom Hubrecht
+  forgejo: tom-hubrecht
+  github: Tom-Hubrecht
+
 thufschmitt:
   display_name: Théophane Hufschmitt
   github: thufschmitt
diff --git a/doc/manual/rl-next/illegal-path-references.md b/doc/manual/rl-next/illegal-path-references.md
new file mode 100644
index 000000000..ff17b2da9
--- /dev/null
+++ b/doc/manual/rl-next/illegal-path-references.md
@@ -0,0 +1,43 @@
+---
+synopsis: Show illegal path references in fixed-outputs derivations
+issues: [fj#530]
+cls: [2726]
+category: Fixes
+credits: [thubrecht]
+---
+
+The error created when referencing a store path in a Fixed-Output Derivation is now more verbose, listing the offending paths.
+This allows for better pinpointing where the issue might be.
+
+An offender is the following derivation:
+
+```nix
+pkgs.stdenv.mkDerivation {
+  name = "illegal-fod";
+
+  dontUnpack = true;
+  dontBuild = true;
+
+  installPhase = ''
+    cp -R ${pkgs.hello} $out
+  '';
+
+  outputHashMode = "recursive";
+  outputHashAlgo = "sha256";
+  outputHash = pkgs.lib.fakeHash;
+}
+```
+
+The previous error shown would have been:
+
+```
+error: illegal path references in fixed-output derivation '/nix/store/rpq4m1y79s2nhs1hj7k47yiyykxykiqa-illegal-fod.drv'
+```
+
+and is now:
+
+```
+error: the fixed-output derivation '/nix/store/rpq4m1y79s2nhs1hj7k47yiyykxykiqa-illegal-fod.drv' must not reference store paths but 2 such references were found:
+         /nix/store/1q8w6gl1ll0mwfkqc3c2yx005s6wwfrl-hello-2.12.1
+         /nix/store/wn7v2vhyyyi6clcyn0s9ixvl7d4d87ic-glibc-2.40-36
+```
diff --git a/lix/libstore/build/local-derivation-goal.cc b/lix/libstore/build/local-derivation-goal.cc
index 1b7c5fc15..635f10234 100644
--- a/lix/libstore/build/local-derivation-goal.cc
+++ b/lix/libstore/build/local-derivation-goal.cc
@@ -2312,10 +2312,21 @@ try {
                             wanted.to_string(Base::SRI, true),
                             got.to_string(Base::SRI, true)));
                 }
-                if (!newInfo0.references.empty())
-                    delayedException = std::make_exception_ptr(
-                        BuildError("illegal path references in fixed-output derivation '%s'",
-                            worker.store.printStorePath(drvPath)));
+                if (!newInfo0.references.empty()) {
+                    std::string references;
+
+                    for (StorePath r : newInfo0.references) {
+                        references.append("\n  " + worker.store.printStorePath(r));
+                    }
+
+                    delayedException = std::make_exception_ptr(BuildError(
+                        "the fixed-output derivation '%s' must not reference store paths but "
+                        "%d such references were found:%s",
+                        worker.store.printStorePath(drvPath),
+                        newInfo0.references.size(),
+                        references
+                    ));
+                }
 
                 return newInfo0;
             },
diff --git a/tests/functional/fixed.nix b/tests/functional/fixed.nix
index 5bdf79333..139b5bc3d 100644
--- a/tests/functional/fixed.nix
+++ b/tests/functional/fixed.nix
@@ -57,6 +57,18 @@ rec {
     outputHashMode = "flat";
   };
 
+  illegalReferences = mkDerivation {
+    name = "illegal-reference";
+    ref = builtins.head good;
+
+    builder = builtins.toFile "builder.sh" ''
+      mkdir $out
+      cp -R $ref $out
+    '';
+    outputHashMode = "recursive";
+    outputHash = "sha256-AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=";
+  };
+
   # Test for building two derivations in parallel that produce the
   # same output path because they're fixed-output derivations.
   parallelSame = [
diff --git a/tests/functional/fixed.sh b/tests/functional/fixed.sh
index d98d4cd15..6dc3adfa8 100644
--- a/tests/functional/fixed.sh
+++ b/tests/functional/fixed.sh
@@ -31,6 +31,13 @@ if isDaemonNewer "2.20pre20240108"; then
     expectStderr 1 nix-build fixed.nix -A badReferences | grepQuiet "not allowed to refer to other store paths"
 fi
 
+echo 'testing illegal references...'
+# Fixed FOD hashes cannot be asserted because:
+# - the store directory varies between the "Lix build sandbox environment" and a user test run
+# - *-darwin has a different store location on the top of this in the sandbox (/private/tmp/...) causing further changes.
+# Regex matching is the best we can afford.
+expectStderr 102 nix-build fixed.nix -A illegalReferences | grep -z "the fixed-output derivation '$TEST_ROOT/store/[a-z0-9]*-illegal-reference.drv' must not reference store paths but 1 such references were found:.*$TEST_ROOT/store/[a-z0-9]*-fixed" > /dev/null
+
 # While we're at it, check attribute selection a bit more.
 echo 'testing attribute selection...'
 test $(nix-instantiate fixed.nix -A good.1 | wc -l) = 1