From ec46a7e4dea8c568677d3d98588810bcd178f048 Mon Sep 17 00:00:00 2001 From: Sergei Trofimovich Date: Wed, 25 Dec 2024 21:09:58 +0000 Subject: [PATCH 1/5] 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]) --- 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 06a2f85be84..b1a9aa64020 100644 --- a/src/libstore/unix/build/local-derivation-goal.cc +++ b/src/libstore/unix/build/local-derivation-goal.cc @@ -2927,8 +2927,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 583a852c8a6b7461dad3635337a7d2fe6f205fd3 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Sat, 18 Jan 2025 09:44:46 +0100 Subject: [PATCH 2/5] nix-util: Add concatMapStrings --- 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 206890bcf19..33a1fae9b23 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 c4fd3daa194..ae0f0070e94 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 8c37493932eb421d629dec61a45f660e7775b6e5 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Sat, 18 Jan 2025 09:49:25 +0100 Subject: [PATCH 3/5] nix-util: Use small_vector in concatMapStringsSep --- 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 402b7ae98a3..b94bca61184 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 ae0f0070e94..521e3425f4a 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 114e7d571adc7cf48a9e0bdaa11047ddac184f99 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Sat, 18 Jan 2025 09:58:17 +0100 Subject: [PATCH 4/5] checkRefs: use concatMapStringsSep --- 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 b1a9aa64020..9b19ab094cf 100644 --- a/src/libstore/unix/build/local-derivation-goal.cc +++ b/src/libstore/unix/build/local-derivation-goal.cc @@ -2928,15 +2928,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 39bd25c7e88171059c2181c62dfdd48a7f40e643 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Sat, 18 Jan 2025 10:21:08 +0100 Subject: [PATCH 5/5] test illegal reference specifier error message --- 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 89690e456c1..1a900830e47 100644 --- a/tests/functional/check-refs.nix +++ b/tests/functional/check-refs.nix @@ -74,4 +74,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 5c3ac915ecf..8eb93b48d3c 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"