From 80d7994f52ccefca2f2fbe4ee64741a6f49884ff Mon Sep 17 00:00:00 2001
From: John Ericson <John.Ericson@Obsidian.Systems>
Date: Wed, 6 Sep 2023 22:57:37 -0400
Subject: [PATCH] Special-case error message to add extra information

The Derivation parser and old ATerm unfortunately leaves few ways to get
nice errors when an old version of Nix encounters a new version of the
format. The most likely scenario for this to occur is with a new client
making a derivation that the old daemon it is communicating with cannot
understand.

The extensions we just created for dynamic derivation deps will add a
version field, solving the problem going forward, but there is still the
issue of what to do about old versions of Nix up to now.

The solution here is to carefully catch the bad error from the daemon
that is likely to indicate this problem, and add some extra context to
it.

There is another "Ugly backwards compatibility hack" in
`remote-store.cc` that also works by transforming an error.

Co-authored-by: Robert Hensing <roberth@users.noreply.github.com>
---
 src/libstore/remote-store.cc            | 19 ++++++++++++++++++-
 tests/dyn-drv/local.mk                  |  3 ++-
 tests/dyn-drv/old-daemon-error-hack.nix | 20 ++++++++++++++++++++
 tests/dyn-drv/old-daemon-error-hack.sh  | 11 +++++++++++
 4 files changed, 51 insertions(+), 2 deletions(-)
 create mode 100644 tests/dyn-drv/old-daemon-error-hack.nix
 create mode 100644 tests/dyn-drv/old-daemon-error-hack.sh

diff --git a/src/libstore/remote-store.cc b/src/libstore/remote-store.cc
index 58f72beb9..a639346d1 100644
--- a/src/libstore/remote-store.cc
+++ b/src/libstore/remote-store.cc
@@ -172,7 +172,24 @@ void RemoteStore::ConnectionHandle::processStderr(Sink * sink, Source * source,
     auto ex = handle->processStderr(sink, source, flush);
     if (ex) {
         daemonException = true;
-        std::rethrow_exception(ex);
+        try {
+            std::rethrow_exception(ex);
+        } catch (const Error & e) {
+            // Nix versions before #4628 did not have an adequate behavior for reporting that the derivation format was upgraded.
+            // To avoid having to add compatibility logic in many places, we expect to catch almost all occurrences of the
+            // old incomprehensible error here, so that we can explain to users what's going on when their daemon is
+            // older than #4628 (2023).
+            if (experimentalFeatureSettings.isEnabled(Xp::DynamicDerivations) &&
+                GET_PROTOCOL_MINOR(handle->daemonVersion) <= 35)
+            {
+                auto m = e.msg();
+                if (m.find("parsing derivation") != std::string::npos &&
+                    m.find("expected string") != std::string::npos &&
+                    m.find("Derive([") != std::string::npos)
+                    throw Error("%s, this might be because the daemon is too old to understand dependencies on dynamic derivations. Check to see if the raw dervation is in the form '%s'", std::move(m), "DrvWithVersion(..)");
+            }
+            throw;
+        }
     }
 }
 
diff --git a/tests/dyn-drv/local.mk b/tests/dyn-drv/local.mk
index 0ce7cd37d..6b435499b 100644
--- a/tests/dyn-drv/local.mk
+++ b/tests/dyn-drv/local.mk
@@ -3,7 +3,8 @@ dyn-drv-tests := \
   $(d)/recursive-mod-json.sh \
   $(d)/build-built-drv.sh \
   $(d)/eval-outputOf.sh \
-  $(d)/dep-built-drv.sh
+  $(d)/dep-built-drv.sh \
+  $(d)/old-daemon-error-hack.sh
 
 install-tests-groups += dyn-drv
 
diff --git a/tests/dyn-drv/old-daemon-error-hack.nix b/tests/dyn-drv/old-daemon-error-hack.nix
new file mode 100644
index 000000000..c9d4a62d4
--- /dev/null
+++ b/tests/dyn-drv/old-daemon-error-hack.nix
@@ -0,0 +1,20 @@
+with import ./config.nix;
+
+# A simple content-addressed derivation.
+# The derivation can be arbitrarily modified by passing a different `seed`,
+# but the output will always be the same
+rec {
+  stub = mkDerivation {
+    name = "stub";
+    buildCommand = ''
+      echo stub > $out
+    '';
+  };
+  wrapper = mkDerivation {
+    name = "has-dynamic-drv-dep";
+    buildCommand = ''
+      exit 1 # we're not building this derivation
+      ${builtins.outputOf stub.outPath "out"}
+    '';
+  };
+}
diff --git a/tests/dyn-drv/old-daemon-error-hack.sh b/tests/dyn-drv/old-daemon-error-hack.sh
new file mode 100644
index 000000000..43b049973
--- /dev/null
+++ b/tests/dyn-drv/old-daemon-error-hack.sh
@@ -0,0 +1,11 @@
+# Purposely bypassing our usual common for this subgroup
+source ../common.sh
+
+# Need backend to support text-hashing too
+isDaemonNewer "2.18.0pre20230906" && skipTest "Daemon is too new"
+
+enableFeatures "ca-derivations dynamic-derivations"
+
+restartDaemon
+
+expectStderr 1 nix-instantiate --read-write-mode ./old-daemon-error-hack.nix | grepQuiet "the daemon is too old to understand dependencies on dynamic derivations"