From 41a31db7c5a536cf969706f2f8f2fea8cb8aee10 Mon Sep 17 00:00:00 2001 From: Sergei Trofimovich Date: Wed, 25 Dec 2024 21:09:58 +0000 Subject: [PATCH 1/7] local-derivation-goal: improve "illegal reference" error Before the change "illegal reference" was hard to interpret as it did not mention what derivation actually hits it. Today's `nixpkgs` example: Before the change: $ nix build --no-link -f. postgresql_14 ... error: derivation contains an illegal reference specifier 'man' After the change: $ nix build --no-link -f. postgresql_14 ... error: derivation '/nix/store/bxp6g57limvwiga61vdlyvhy7i8rp6wd-postgresql-14.15.drv' output check for 'lib' contains an illegal reference specifier 'man', expected store path or output name (one of [debug, dev, doc, lib, out]) (cherry picked from commit bbdc3197a925b56bdec1220089de7622832bd2a3) --- src/libstore/unix/build/local-derivation-goal.cc | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/src/libstore/unix/build/local-derivation-goal.cc b/src/libstore/unix/build/local-derivation-goal.cc index ecbc8acb8..915392078 100644 --- a/src/libstore/unix/build/local-derivation-goal.cc +++ b/src/libstore/unix/build/local-derivation-goal.cc @@ -2942,8 +2942,17 @@ void LocalDerivationGoal::checkOutputs(const std::mappath); - else - throw BuildError("derivation contains an illegal reference specifier '%s'", i); + else { + std::string allOutputs; + for (auto & o : outputs) { + if (! allOutputs.empty()) + allOutputs.append(", "); + allOutputs.append(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, allOutputs); + } } auto used = recursive From beac7720a30052e726371e8cc865bb3d347e9415 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Sat, 18 Jan 2025 09:44:46 +0100 Subject: [PATCH 2/7] nix-util: Add concatMapStrings (cherry picked from commit f3dbaa3f54c54b0a71e230ab097c9a72d17c3ed9) --- src/libutil-tests/strings.cc | 36 ++++++++++++++++++++++++++++++++++++ src/libutil/strings.hh | 14 ++++++++++++++ 2 files changed, 50 insertions(+) diff --git a/src/libutil-tests/strings.cc b/src/libutil-tests/strings.cc index 206890bcf..33a1fae9b 100644 --- a/src/libutil-tests/strings.cc +++ b/src/libutil-tests/strings.cc @@ -80,6 +80,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 strings; + strings["this"] = "that"; + strings["1"] = "one"; + + ASSERT_EQ( + concatMapStringsSep( + ", ", strings, [](const std::pair & s) { return s.first + " -> " + s.second; }), + "1 -> one, this -> that"); +} + /* ---------------------------------------------------------------------------- * dropEmptyInitThenConcatStringsSep * --------------------------------------------------------------------------*/ diff --git a/src/libutil/strings.hh b/src/libutil/strings.hh index c4fd3daa1..ae0f0070e 100644 --- a/src/libutil/strings.hh +++ b/src/libutil/strings.hh @@ -55,6 +55,20 @@ extern template std::string concatStringsSep(std::string_view, const std::list &); extern template std::string concatStringsSep(std::string_view, const std::vector &); +/** + * Apply a function to the `iterable`'s items and concat them with `separator`. + */ +template +std::string concatMapStringsSep(std::string_view separator, const C & iterable, F fn) +{ + std::vector 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 * given strings with a separator between the elements. From 0ddb8e21fe986073a04e85625def7abc9c343cbd Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Sat, 18 Jan 2025 09:49:25 +0100 Subject: [PATCH 3/7] nix-util: Use small_vector in concatMapStringsSep (cherry picked from commit 32898dc46a21c628d3ae275310307c56cbe8ab03) --- src/libutil/strings.cc | 1 + src/libutil/strings.hh | 5 ++++- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/src/libutil/strings.cc b/src/libutil/strings.cc index 402b7ae98..b94bca611 100644 --- a/src/libutil/strings.cc +++ b/src/libutil/strings.cc @@ -37,6 +37,7 @@ basicSplitString(std::basic_string_view s, std::basic_string_view &); template std::string concatStringsSep(std::string_view, const std::set &); template std::string concatStringsSep(std::string_view, const std::vector &); +template std::string concatStringsSep(std::string_view, const boost::container::small_vector &); typedef std::string_view strings_2[2]; template std::string concatStringsSep(std::string_view, const strings_2 &); diff --git a/src/libutil/strings.hh b/src/libutil/strings.hh index ae0f0070e..521e3425f 100644 --- a/src/libutil/strings.hh +++ b/src/libutil/strings.hh @@ -6,6 +6,8 @@ #include #include +#include + namespace nix { /* @@ -54,6 +56,7 @@ std::string concatStringsSep(const std::string_view sep, const C & ss); extern template std::string concatStringsSep(std::string_view, const std::list &); extern template std::string concatStringsSep(std::string_view, const std::set &); extern template std::string concatStringsSep(std::string_view, const std::vector &); +extern template std::string concatStringsSep(std::string_view, const boost::container::small_vector &); /** * Apply a function to the `iterable`'s items and concat them with `separator`. @@ -61,7 +64,7 @@ extern template std::string concatStringsSep(std::string_view, const std::vector template std::string concatMapStringsSep(std::string_view separator, const C & iterable, F fn) { - std::vector strings; + boost::container::small_vector strings; strings.reserve(iterable.size()); for (const auto & elem : iterable) { strings.push_back(fn(elem)); From 7b3a78dbabb4f4bac81cd31134358d75c34c8008 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Sat, 18 Jan 2025 09:58:17 +0100 Subject: [PATCH 4/7] checkRefs: use concatMapStringsSep (cherry picked from commit 2b4d461c14e01eb86f5b253e7df93c595f45f52e) --- src/libstore/unix/build/local-derivation-goal.cc | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/src/libstore/unix/build/local-derivation-goal.cc b/src/libstore/unix/build/local-derivation-goal.cc index 915392078..b058aa5ad 100644 --- a/src/libstore/unix/build/local-derivation-goal.cc +++ b/src/libstore/unix/build/local-derivation-goal.cc @@ -2943,15 +2943,10 @@ void LocalDerivationGoal::checkOutputs(const std::mappath); else { - std::string allOutputs; - for (auto & o : outputs) { - if (! allOutputs.empty()) - allOutputs.append(", "); - allOutputs.append(o.first); - } + 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, allOutputs); + worker.store.printStorePath(drvPath), outputName, i, outputsListing); } } From 6e9331142649c81d4eca611858d2b15058b1c68b Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Sat, 18 Jan 2025 10:21:08 +0100 Subject: [PATCH 5/7] test illegal reference specifier error message (cherry picked from commit f4def47c899a8f637449a3d3670c843a706218ca) --- tests/functional/check-refs.nix | 6 ++++++ tests/functional/check-refs.sh | 4 ++++ 2 files changed, 10 insertions(+) diff --git a/tests/functional/check-refs.nix b/tests/functional/check-refs.nix index 471d95753..9512c73c1 100644 --- a/tests/functional/check-refs.nix +++ b/tests/functional/check-refs.nix @@ -79,4 +79,10 @@ rec { buildCommand = ''echo ${dep} > "''${outputs[out]}"''; }; + test12 = makeTest 12 { + builder = builtins.toFile "builder.sh" "mkdir $out $lib"; + outputs = ["out" "lib"]; + disallowedReferences = ["dev"]; + }; + } diff --git a/tests/functional/check-refs.sh b/tests/functional/check-refs.sh index 5c3ac915e..8eb93b48d 100755 --- a/tests/functional/check-refs.sh +++ b/tests/functional/check-refs.sh @@ -60,3 +60,7 @@ if ! isTestOnNixOS; then fi fi + +# 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" From 2fbebb6574d9cbff482f6a3f451291c7ad437996 Mon Sep 17 00:00:00 2001 From: Sergei Trofimovich Date: Mon, 24 Mar 2025 22:34:09 +0000 Subject: [PATCH 6/7] tests/functional/check-refs.sh: guard test12 against too old nix daemon Otherwise without the change the test fails on nix-2.26 as: error: derivation contains an illegal reference specifier 'dev' Note: the error message does not match intended change. (cherry picked from commit 1e7c7244cf6e7f0fba83764153a31a9ff780cb7e) --- tests/functional/check-refs.sh | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/tests/functional/check-refs.sh b/tests/functional/check-refs.sh index 8eb93b48d..590c3fb53 100755 --- a/tests/functional/check-refs.sh +++ b/tests/functional/check-refs.sh @@ -61,6 +61,8 @@ if ! isTestOnNixOS; then fi -# 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" +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 From fa33df1e76ba58f0aab538908ef171403d5ec862 Mon Sep 17 00:00:00 2001 From: Sergei Trofimovich Date: Mon, 24 Mar 2025 22:45:28 +0000 Subject: [PATCH 7/7] tests/functional/check-refs.nix: format newly added test (cherry picked from commit 4d72e0f73bc31ac200d57caba65f6355760df032) --- tests/functional/check-refs.nix | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/tests/functional/check-refs.nix b/tests/functional/check-refs.nix index 9512c73c1..bdd5c4f8d 100644 --- a/tests/functional/check-refs.nix +++ b/tests/functional/check-refs.nix @@ -81,8 +81,11 @@ rec { test12 = makeTest 12 { builder = builtins.toFile "builder.sh" "mkdir $out $lib"; - outputs = ["out" "lib"]; - disallowedReferences = ["dev"]; + outputs = [ + "out" + "lib" + ]; + disallowedReferences = [ "dev" ]; }; }