Merge pull request #10941 from hercules-ci/check-derivation-name

Improve error messages for invalid derivation names

src/libexpr/primops.cc

@@ -1162,12 +1162,34 @@ static void prim_derivationStrict(EvalState & state, const PosIdx pos, Value * *
     }
 }
 
+/**
+ * Early validation for the derivation name, for better error message.
+ * It is checked again when constructing store paths.
+ *
+ * @todo Check that the `.drv` suffix also fits.
+ */
+static void checkDerivationName(EvalState & state, std::string_view drvName)
+{
+    try {
+        checkName(drvName);
+    } catch (BadStorePathName & e) {
+        // "Please pass a different name": Users may not be aware that they can
+        //     pass a different one, in functions like `fetchurl` where the name
+        //     is optional.
+        // Note that Nixpkgs generally won't trigger this, because `mkDerivation`
+        // sanitizes the name.
+        state.error<EvalError>("invalid derivation name: %s. Please pass a different '%s'.", Uncolored(e.message()), "name").debugThrow();
+    }
+}
+
 static void derivationStrictInternal(
     EvalState & state,
     const std::string & drvName,
     const Bindings * attrs,
     Value & v)
 {
+    checkDerivationName(state, drvName);
+
     /* Check whether attributes should be passed as a JSON file. */
     using nlohmann::json;
     std::optional<json> jsonObject;

src/libexpr/primops/fetchTree.cc

@@ -431,7 +431,10 @@ static void fetch(EvalState & state, const PosIdx pos, Value * * args, Value & v
 
     state.forceValue(*args[0], pos);
 
-    if (args[0]->type() == nAttrs) {
+    bool isArgAttrs = args[0]->type() == nAttrs;
+    bool nameAttrPassed = false;
+
+    if (isArgAttrs) {
 
         for (auto & attr : *args[0]->attrs()) {
             std::string_view n(state.symbols[attr.name]);
@@ -439,8 +442,10 @@ static void fetch(EvalState & state, const PosIdx pos, Value * * args, Value & v
                 url = state.forceStringNoCtx(*attr.value, attr.pos, "while evaluating the url we should fetch");
             else if (n == "sha256")
                 expectedHash = newHashAllowEmpty(state.forceStringNoCtx(*attr.value, attr.pos, "while evaluating the sha256 of the content we should fetch"), HashAlgorithm::SHA256);
-            else if (n == "name")
+            else if (n == "name") {
+                nameAttrPassed = true;
                 name = state.forceStringNoCtx(*attr.value, attr.pos, "while evaluating the name of the content we should fetch");
+            }
             else
                 state.error<EvalError>("unsupported argument '%s' to '%s'", n, who)
                 .atPos(pos).debugThrow();
@@ -460,6 +465,19 @@ static void fetch(EvalState & state, const PosIdx pos, Value * * args, Value & v
     if (name == "")
         name = baseNameOf(*url);
 
+    try {
+        checkName(name);
+    } catch (BadStorePathName & e) {
+        auto resolution =
+            nameAttrPassed ? HintFmt("Please change the value for the 'name' attribute passed to '%s', so that it can create a valid store path.", who) :
+            isArgAttrs ? HintFmt("Please add a valid 'name' attribute to the argument for '%s', so that it can create a valid store path.", who) :
+            HintFmt("Please pass an attribute set with 'url' and 'name' attributes to '%s',  so that it can create a valid store path.", who);
+
+        state.error<EvalError>(
+            std::string("invalid store path name when fetching URL '%s': %s. %s"), *url, Uncolored(e.message()), Uncolored(resolution.str()))
+        .atPos(pos).debugThrow();
+    }
+
     if (state.settings.pureEval && !expectedHash)
         state.error<EvalError>("in pure evaluation mode, '%s' requires a 'sha256' argument", who).atPos(pos).debugThrow();
 

src/libstore/path.cc

@@ -2,25 +2,24 @@
 
 namespace nix {
 
-static void checkName(std::string_view path, std::string_view name)
+void checkName(std::string_view name)
 {
     if (name.empty())
-        throw BadStorePath("store path '%s' has an empty name", path);
+        throw BadStorePathName("name must not be empty");
     if (name.size() > StorePath::MaxPathLen)
-        throw BadStorePath("store path '%s' has a name longer than %d characters",
-            path, StorePath::MaxPathLen);
+        throw BadStorePathName("name '%s' must be no longer than %d characters", name, StorePath::MaxPathLen);
     // See nameRegexStr for the definition
     if (name[0] == '.') {
         // check against "." and "..", followed by end or dash
         if (name.size() == 1)
-            throw BadStorePath("store path '%s' has invalid name '%s'", path, name);
+            throw BadStorePathName("name '%s' is not valid", name);
         if (name[1] == '-')
-            throw BadStorePath("store path '%s' has invalid name '%s': first dash-separated component must not be '%s'", path, name, ".");
+            throw BadStorePathName("name '%s' is not valid: first dash-separated component must not be '%s'", name, ".");
         if (name[1] == '.') {
             if (name.size() == 2)
-                throw BadStorePath("store path '%s' has invalid name '%s'", path, name);
+                throw BadStorePathName("name '%s' is not valid", name);
             if (name[2] == '-')
-                throw BadStorePath("store path '%s' has invalid name '%s': first dash-separated component must not be '%s'", path, name, "..");
+                throw BadStorePathName("name '%s' is not valid: first dash-separated component must not be '%s'", name, "..");
         }
     }
     for (auto c : name)
@@ -28,7 +27,16 @@ static void checkName(std::string_view path, std::string_view name)
                 || (c >= 'a' && c <= 'z')
                 || (c >= 'A' && c <= 'Z')
                 || c == '+' || c == '-' || c == '.' || c == '_' || c == '?' || c == '='))
-            throw BadStorePath("store path '%s' contains illegal character '%s'", path, c);
+            throw BadStorePathName("name '%s' contains illegal character '%s'", name, c);
+}
+
+static void checkPathName(std::string_view path, std::string_view name)
+{
+    try {
+        checkName(name);
+    } catch (BadStorePathName & e) {
+        throw BadStorePath("path '%s' is not a valid store path: %s", path, Uncolored(e.message()));
+    }
 }
 
 StorePath::StorePath(std::string_view _baseName)
@@ -40,13 +48,13 @@ StorePath::StorePath(std::string_view _baseName)
         if (c == 'e' || c == 'o' || c == 'u' || c == 't'
             || !((c >= '0' && c <= '9') || (c >= 'a' && c <= 'z')))
             throw BadStorePath("store path '%s' contains illegal base-32 character '%s'", baseName, c);
-    checkName(baseName, name());
+    checkPathName(baseName, name());
 }
 
 StorePath::StorePath(const Hash & hash, std::string_view _name)
     : baseName((hash.to_string(HashFormat::Nix32, false) + "-").append(std::string(_name)))
 {
-    checkName(baseName, name());
+    checkPathName(baseName, name());
 }
 
 bool StorePath::isDerivation() const noexcept

src/libstore/path.hh

@@ -9,6 +9,13 @@ namespace nix {
 
 struct Hash;
 
+/**
+ * Check whether a name is a valid store path name.
+ *
+ * @throws BadStorePathName if the name is invalid. The message is of the format "name %s is not valid, for this specific reason".
+ */
+void checkName(std::string_view name);
+
 /**
  * \ref StorePath "Store path" is the fundamental reference type of Nix.
  * A store paths refers to a Store object.
@@ -31,8 +38,10 @@ public:
 
     StorePath() = delete;
 
+    /** @throws BadStorePath */
     StorePath(std::string_view baseName);
 
+    /** @throws BadStorePath */
     StorePath(const Hash & hash, std::string_view name);
 
     std::string_view to_string() const noexcept

src/libstore/store-dir-config.hh

@@ -16,6 +16,7 @@ namespace nix {
 struct SourcePath;
 
 MakeError(BadStorePath, Error);
+MakeError(BadStorePathName, BadStorePath);
 
 struct StoreDirConfig : public Config
 {

src/libutil/error.hh

@@ -155,6 +155,7 @@ public:
         : err(e)
     { }
 
+    /** The error message without "error: " prefixed to it. */
     std::string message() {
         return err.msg.str();
     }

src/libutil/fmt.hh

@@ -111,6 +111,8 @@ std::ostream & operator<<(std::ostream & out, const Magenta<T> & y)
 /**
  * Values wrapped in this class are printed without coloring.
  *
+ * Specifically, the color is reset to normal before printing the value.
+ *
  * By default, arguments to `HintFmt` are printed in magenta (see `Magenta`).
  */
 template <class T>

tests/functional/lang/eval-fail-derivation-name.err.exp

@@ -0,0 +1,26 @@
+error:
+       … while evaluating the attribute 'outPath'
+         at <nix/derivation-internal.nix>:19:9:
+           18|       value = commonAttrs // {
+           19|         outPath = builtins.getAttr outputName strict;
+             |         ^
+           20|         drvPath = strict.drvPath;
+
+       … while calling the 'getAttr' builtin
+         at <nix/derivation-internal.nix>:19:19:
+           18|       value = commonAttrs // {
+           19|         outPath = builtins.getAttr outputName strict;
+             |                   ^
+           20|         drvPath = strict.drvPath;
+
+       … while calling the 'derivationStrict' builtin
+         at <nix/derivation-internal.nix>:9:12:
+            8|
+            9|   strict = derivationStrict drvAttrs;
+             |            ^
+           10|
+
+       … while evaluating derivation '~jiggle~'
+         whose name attribute is located at /pwd/lang/eval-fail-derivation-name.nix:2:3
+
+       error: invalid derivation name: name '~jiggle~' contains illegal character '~'. Please pass a different 'name'.

tests/functional/lang/eval-fail-derivation-name.nix

@@ -0,0 +1,5 @@
+derivation {
+  name = "~jiggle~";
+  system = "some-system";
+  builder = "/dontcare";
+}

tests/functional/lang/eval-fail-fetchurl-baseName-attrs-name.err.exp

@@ -0,0 +1,8 @@
+error:
+       … while calling the 'fetchurl' builtin
+         at /pwd/lang/eval-fail-fetchurl-baseName-attrs-name.nix:1:1:
+            1| builtins.fetchurl { url = "https://example.com/foo.tar.gz"; name = "~wobble~"; }
+             | ^
+            2|
+
+       error: invalid store path name when fetching URL 'https://example.com/foo.tar.gz': name '~wobble~' contains illegal character '~'. Please change the value for the 'name' attribute passed to 'fetchurl', so that it can create a valid store path.

tests/functional/lang/eval-fail-fetchurl-baseName-attrs-name.nix

@@ -0,0 +1 @@
+builtins.fetchurl { url = "https://example.com/foo.tar.gz"; name = "~wobble~"; }

tests/functional/lang/eval-fail-fetchurl-baseName-attrs.err.exp

@@ -0,0 +1,8 @@
+error:
+       … while calling the 'fetchurl' builtin
+         at /pwd/lang/eval-fail-fetchurl-baseName-attrs.nix:1:1:
+            1| builtins.fetchurl { url = "https://example.com/~wiggle~"; }
+             | ^
+            2|
+
+       error: invalid store path name when fetching URL 'https://example.com/~wiggle~': name '~wiggle~' contains illegal character '~'. Please add a valid 'name' attribute to the argument for 'fetchurl', so that it can create a valid store path.

tests/functional/lang/eval-fail-fetchurl-baseName-attrs.nix

@@ -0,0 +1 @@
+builtins.fetchurl { url = "https://example.com/~wiggle~"; }

tests/functional/lang/eval-fail-fetchurl-baseName.err.exp

@@ -0,0 +1,8 @@
+error:
+       … while calling the 'fetchurl' builtin
+         at /pwd/lang/eval-fail-fetchurl-baseName.nix:1:1:
+            1| builtins.fetchurl "https://example.com/~wiggle~"
+             | ^
+            2|
+
+       error: invalid store path name when fetching URL 'https://example.com/~wiggle~': name '~wiggle~' contains illegal character '~'. Please pass an attribute set with 'url' and 'name' attributes to 'fetchurl',  so that it can create a valid store path.

tests/functional/lang/eval-fail-fetchurl-baseName.nix

@@ -0,0 +1 @@
+builtins.fetchurl "https://example.com/~wiggle~"

tests/unit/libstore/path.cc

@@ -26,10 +26,19 @@ static std::regex nameRegex { std::string { nameRegexStr } };
     TEST_F(StorePathTest, bad_ ## NAME) {                    \
         std::string_view str =                               \
             STORE_DIR HASH_PART "-" STR;                     \
-        ASSERT_THROW(                                        \
-            store->parseStorePath(str),                      \
-            BadStorePath);                                   \
+        /* ASSERT_THROW generates a duplicate goto label */  \
+        /* A lambda isolates those labels. */                \
+        [&](){                                               \
+            ASSERT_THROW(                                    \
+                store->parseStorePath(str),                  \
+                BadStorePath);                               \
+        }();                                                 \
         std::string name { STR };                            \
+        [&](){                                               \
+            ASSERT_THROW(                                    \
+                nix::checkName(name),                        \
+                BadStorePathName);                           \
+        }();                                                 \
         EXPECT_FALSE(std::regex_match(name, nameRegex));     \
     }
 
@@ -54,6 +63,7 @@ TEST_DONT_PARSE(dot_dash_a, ".-a")
             STORE_DIR HASH_PART "-" STR;                     \
         auto p = store->parseStorePath(str);                 \
         std::string name { p.name() };                       \
+        EXPECT_EQ(p.name(), STR);                            \
         EXPECT_TRUE(std::regex_match(name, nameRegex));      \
     }