Merge pull request #12753 from NixOS/mergify/bp/2.25-maintenance/pr-12105

local-derivation-goal: improve "illegal reference" error (backport #12105)

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

@@ -2942,8 +2942,12 @@ void LocalDerivationGoal::checkOutputs(const std::map<std::string, ValidPathInfo
                         spec.insert(worker.store.parseStorePath(i));
                     else if (auto output = get(outputs, i))
                         spec.insert(output->path);
-                    else
-                        throw BuildError("derivation contains an illegal reference specifier '%s'", i);
+                    else {
+                        std::string outputsListing = concatMapStringsSep(", ", outputs, [](auto & o) { return o.first; });
+                        throw BuildError("derivation '%s' output check for '%s' contains an illegal reference specifier '%s',"
+                            " expected store path or output name (one of [%s])",
+                            worker.store.printStorePath(drvPath), outputName, i, outputsListing);
+                    }
                 }
 
                 auto used = recursive

src/libutil-tests/strings.cc

@@ -81,6 +81,42 @@ TEST(concatStringsSep, buildSingleString)
     ASSERT_EQ(concatStringsSep(",", strings), "this");
 }
 
+TEST(concatMapStringsSep, empty)
+{
+    Strings strings;
+
+    ASSERT_EQ(concatMapStringsSep(",", strings, [](const std::string & s) { return s; }), "");
+}
+
+TEST(concatMapStringsSep, justOne)
+{
+    Strings strings;
+    strings.push_back("this");
+
+    ASSERT_EQ(concatMapStringsSep(",", strings, [](const std::string & s) { return s; }), "this");
+}
+
+TEST(concatMapStringsSep, two)
+{
+    Strings strings;
+    strings.push_back("this");
+    strings.push_back("that");
+
+    ASSERT_EQ(concatMapStringsSep(",", strings, [](const std::string & s) { return s; }), "this,that");
+}
+
+TEST(concatMapStringsSep, map)
+{
+    std::map<std::string, std::string> strings;
+    strings["this"] = "that";
+    strings["1"] = "one";
+
+    ASSERT_EQ(
+        concatMapStringsSep(
+            ", ", strings, [](const std::pair<std::string, std::string> & s) { return s.first + " -> " + s.second; }),
+        "1 -> one, this -> that");
+}
+
 /* ----------------------------------------------------------------------------
  * dropEmptyInitThenConcatStringsSep
  * --------------------------------------------------------------------------*/

src/libutil/strings.cc

@@ -36,6 +36,7 @@ basicSplitString(std::basic_string_view<OsChar> s, std::basic_string_view<OsChar
 template std::string concatStringsSep(std::string_view, const std::list<std::string> &);
 template std::string concatStringsSep(std::string_view, const std::set<std::string> &);
 template std::string concatStringsSep(std::string_view, const std::vector<std::string> &);
+template std::string concatStringsSep(std::string_view, const boost::container::small_vector<std::string, 64> &);
 
 typedef std::string_view strings_2[2];
 template std::string concatStringsSep(std::string_view, const strings_2 &);

src/libutil/strings.hh

@@ -6,6 +6,8 @@
 #include <string>
 #include <vector>
 
+#include <boost/container/small_vector.hpp>
+
 namespace nix {
 
 /*
@@ -54,6 +56,21 @@ std::string concatStringsSep(const std::string_view sep, const C & ss);
 extern template std::string concatStringsSep(std::string_view, const std::list<std::string> &);
 extern template std::string concatStringsSep(std::string_view, const std::set<std::string> &);
 extern template std::string concatStringsSep(std::string_view, const std::vector<std::string> &);
+extern template std::string concatStringsSep(std::string_view, const boost::container::small_vector<std::string, 64> &);
+
+/**
+ * Apply a function to the `iterable`'s items and concat them with `separator`.
+ */
+template<class C, class F>
+std::string concatMapStringsSep(std::string_view separator, const C & iterable, F fn)
+{
+    boost::container::small_vector<std::string, 64> strings;
+    strings.reserve(iterable.size());
+    for (const auto & elem : iterable) {
+        strings.push_back(fn(elem));
+    }
+    return concatStringsSep(separator, strings);
+}
 
 /**
  * Ignore any empty strings at the start of the list, and then concatenate the

tests/functional/check-refs.nix

@@ -79,4 +79,13 @@ rec {
     buildCommand = ''echo ${dep} > "''${outputs[out]}"'';
   };
 
+  test12 = makeTest 12 {
+    builder = builtins.toFile "builder.sh" "mkdir $out $lib";
+    outputs = [
+      "out"
+      "lib"
+    ];
+    disallowedReferences = [ "dev" ];
+  };
+
 }

tests/functional/check-refs.sh

@@ -60,3 +60,9 @@ if ! isTestOnNixOS; then
     fi
 
 fi
+
+if isDaemonNewer "2.28pre20241225"; then
+    # test12 should fail (syntactically invalid).
+    expectStderr 1 nix-build -vvv -o "$RESULT" check-refs.nix -A test12 >"$TEST_ROOT/test12.stderr"
+    grepQuiet -F "output check for 'lib' contains an illegal reference specifier 'dev', expected store path or output name (one of [lib, out])" < "$TEST_ROOT/test12.stderr"
+fi