Merge pull request #14232 from roberth/dyndrv-messages

Better dyndrv messages

src/libexpr/primops.cc

@@ -1420,7 +1420,8 @@ static void derivationStrictInternal(EvalState & state, std::string_view drvName
                         .debugThrow();
                 }
             if (ingestionMethod == ContentAddressMethod::Raw::Text)
-                experimentalFeatureSettings.require(Xp::DynamicDerivations);
+                experimentalFeatureSettings.require(
+                    Xp::DynamicDerivations, fmt("text-hashed derivation '%s', outputHashMode = \"text\"", drvName));
             if (ingestionMethod == ContentAddressMethod::Raw::Git)
                 experimentalFeatureSettings.require(Xp::GitHashing);
         };

src/libstore/derivations.cc

@@ -290,7 +290,7 @@ static DerivationOutput parseDerivationOutput(
     if (!hashAlgoStr.empty()) {
         ContentAddressMethod method = ContentAddressMethod::parsePrefix(hashAlgoStr);
         if (method == ContentAddressMethod::Raw::Text)
-            xpSettings.require(Xp::DynamicDerivations);
+            xpSettings.require(Xp::DynamicDerivations, "text-hashed derivation output");
         const auto hashAlgo = parseHashAlgo(hashAlgoStr);
         if (hashS == "impure"sv) {
             xpSettings.require(Xp::ImpureDerivations);
@@ -428,7 +428,9 @@ Derivation parseDerivation(
         if (*versionS == "xp-dyn-drv"sv) {
             // Only version we have so far
             version = DerivationATermVersion::DynamicDerivations;
-            xpSettings.require(Xp::DynamicDerivations);
+            xpSettings.require(Xp::DynamicDerivations, [&] {
+                return fmt("derivation '%s', ATerm format version 'xp-dyn-drv'", name);
+            });
         } else {
             throw FormatError("Unknown derivation ATerm format version '%s'", *versionS);
         }
@@ -1303,7 +1305,7 @@ DerivationOutput::fromJSON(const nlohmann::json & _json, const ExperimentalFeatu
     auto methodAlgo = [&]() -> std::pair<ContentAddressMethod, HashAlgorithm> {
         ContentAddressMethod method = ContentAddressMethod::parse(getString(valueAt(json, "method")));
         if (method == ContentAddressMethod::Raw::Text)
-            xpSettings.require(Xp::DynamicDerivations);
+            xpSettings.require(Xp::DynamicDerivations, "text-hashed derivation output in JSON");
 
         auto hashAlgo = parseHashAlgo(getString(valueAt(json, "hashAlgo")));
         return {std::move(method), std::move(hashAlgo)};
@@ -1456,7 +1458,8 @@ Derivation Derivation::fromJSON(const nlohmann::json & _json, const Experimental
             node.value = getStringSet(valueAt(json, "outputs"));
             auto drvs = getObject(valueAt(json, "dynamicOutputs"));
             for (auto & [outputId, childNode] : drvs) {
-                xpSettings.require(Xp::DynamicDerivations);
+                xpSettings.require(
+                    Xp::DynamicDerivations, [&] { return fmt("dynamic output '%s' in JSON", outputId); });
                 node.childMap[outputId] = doInput(childNode);
             }
             return node;

src/libstore/derived-path.cc

@@ -85,7 +85,11 @@ void drvRequireExperiment(const SingleDerivedPath & drv, const ExperimentalFeatu
             [&](const SingleDerivedPath::Opaque &) {
                 // plain drv path; no experimental features required.
             },
-            [&](const SingleDerivedPath::Built &) { xpSettings.require(Xp::DynamicDerivations); },
+            [&](const SingleDerivedPath::Built & b) {
+                xpSettings.require(Xp::DynamicDerivations, [&] {
+                    return fmt("building output '%s' of '%s'", b.output, b.drvPath->getBaseStorePath().to_string());
+                });
+            },
         },
         drv.raw());
 }

src/libstore/downstream-placeholder.cc

@@ -24,7 +24,8 @@ DownstreamPlaceholder DownstreamPlaceholder::unknownDerivation(
     OutputNameView outputName,
     const ExperimentalFeatureSettings & xpSettings)
 {
-    xpSettings.require(Xp::DynamicDerivations);
+    xpSettings.require(
+        Xp::DynamicDerivations, [&] { return fmt("placeholder for unknown derivation output '%s'", outputName); });
     auto compressed = compressHash(placeholder.hash, 20);
     auto clearText =
         "nix-computed-output:" + compressed.to_string(HashFormat::Nix32, false) + ":" + std::string{outputName};

src/libutil-tests/strings.cc

@@ -494,4 +494,63 @@ TEST(shellSplitString, testUnbalancedQuotes)
     ASSERT_THROW(shellSplitString("foo\"bar\\\""), Error);
 }
 
+/* ----------------------------------------------------------------------------
+ * optionalBracket
+ * --------------------------------------------------------------------------*/
+
+TEST(optionalBracket, emptyContent)
+{
+    ASSERT_EQ(optionalBracket(" (", "", ")"), "");
+}
+
+TEST(optionalBracket, nonEmptyContent)
+{
+    ASSERT_EQ(optionalBracket(" (", "foo", ")"), " (foo)");
+}
+
+TEST(optionalBracket, emptyPrefixAndSuffix)
+{
+    ASSERT_EQ(optionalBracket("", "foo", ""), "foo");
+}
+
+TEST(optionalBracket, emptyContentEmptyBrackets)
+{
+    ASSERT_EQ(optionalBracket("", "", ""), "");
+}
+
+TEST(optionalBracket, complexBrackets)
+{
+    ASSERT_EQ(optionalBracket(" [[[", "content", "]]]"), " [[[content]]]");
+}
+
+TEST(optionalBracket, onlyPrefix)
+{
+    ASSERT_EQ(optionalBracket("prefix", "content", ""), "prefixcontent");
+}
+
+TEST(optionalBracket, onlySuffix)
+{
+    ASSERT_EQ(optionalBracket("", "content", "suffix"), "contentsuffix");
+}
+
+TEST(optionalBracket, optionalWithValue)
+{
+    ASSERT_EQ(optionalBracket(" (", std::optional<std::string>("foo"), ")"), " (foo)");
+}
+
+TEST(optionalBracket, optionalNullopt)
+{
+    ASSERT_EQ(optionalBracket(" (", std::optional<std::string>(std::nullopt), ")"), "");
+}
+
+TEST(optionalBracket, optionalEmptyString)
+{
+    ASSERT_EQ(optionalBracket(" (", std::optional<std::string>(""), ")"), "");
+}
+
+TEST(optionalBracket, optionalStringViewWithValue)
+{
+    ASSERT_EQ(optionalBracket(" (", std::optional<std::string_view>("bar"), ")"), " (bar)");
+}
+
 } // namespace nix

src/libutil/configuration.cc

@@ -500,10 +500,10 @@ bool ExperimentalFeatureSettings::isEnabled(const ExperimentalFeature & feature)
     return std::find(f.begin(), f.end(), feature) != f.end();
 }
 
-void ExperimentalFeatureSettings::require(const ExperimentalFeature & feature) const
+void ExperimentalFeatureSettings::require(const ExperimentalFeature & feature, std::string reason) const
 {
     if (!isEnabled(feature))
-        throw MissingExperimentalFeature(feature);
+        throw MissingExperimentalFeature(feature, std::move(reason));
 }
 
 bool ExperimentalFeatureSettings::isEnabled(const std::optional<ExperimentalFeature> & feature) const

src/libutil/experimental-features.cc

@@ -1,5 +1,6 @@
 #include "nix/util/experimental-features.hh"
 #include "nix/util/fmt.hh"
+#include "nix/util/strings.hh"
 #include "nix/util/util.hh"
 
 #include <nlohmann/json.hpp>
@@ -376,11 +377,13 @@ std::set<ExperimentalFeature> parseFeatures(const StringSet & rawFeatures)
     return res;
 }
 
-MissingExperimentalFeature::MissingExperimentalFeature(ExperimentalFeature feature)
+MissingExperimentalFeature::MissingExperimentalFeature(ExperimentalFeature feature, std::string reason)
     : Error(
-          "experimental Nix feature '%1%' is disabled; add '--extra-experimental-features %1%' to enable it",
+          "experimental Nix feature '%1%' is disabled%2%; add '--extra-experimental-features %1%' to enable it",
-          showExperimentalFeature(feature))
+          showExperimentalFeature(feature),
+          Uncolored(optionalBracket(" (", reason, ")")))
     , missingFeature(feature)
+    , reason{reason}
 {
 }
 

src/libutil/include/nix/util/configuration.hh

@@ -463,7 +463,20 @@ struct ExperimentalFeatureSettings : Config
      * Require an experimental feature be enabled, throwing an error if it is
      * not.
      */
-    void require(const ExperimentalFeature &) const;
+    void require(const ExperimentalFeature &, std::string reason = "") const;
+
+    /**
+     * Require an experimental feature be enabled, throwing an error if it is
+     * not. The reason is lazily evaluated only if the feature is disabled.
+     */
+    template<typename GetReason>
+        requires std::invocable<GetReason> && std::convertible_to<std::invoke_result_t<GetReason>, std::string>
+    void require(const ExperimentalFeature & feature, GetReason && getReason) const
+    {
+        if (isEnabled(feature))
+            return;
+        require(feature, getReason());
+    }
 
     /**
      * `std::nullopt` pointer means no feature, which means there is nothing that could be

src/libutil/include/nix/util/experimental-features.hh

@@ -88,7 +88,9 @@ public:
      */
     ExperimentalFeature missingFeature;
 
-    MissingExperimentalFeature(ExperimentalFeature missingFeature);
+    std::string reason;
+
+    MissingExperimentalFeature(ExperimentalFeature missingFeature, std::string reason = "");
 };
 
 /**

src/libutil/include/nix/util/strings.hh

@@ -3,6 +3,7 @@
 #include "nix/util/types.hh"
 
 #include <list>
+#include <optional>
 #include <set>
 #include <string_view>
 #include <string>
@@ -93,6 +94,44 @@ extern template std::string dropEmptyInitThenConcatStringsSep(std::string_view,
  */
 std::list<std::string> shellSplitString(std::string_view s);
 
+/**
+ * Conditionally wrap a string with prefix and suffix brackets.
+ *
+ * If `content` is empty, returns an empty string.
+ * Otherwise, returns `prefix + content + suffix`.
+ *
+ * Example:
+ *   optionalBracket(" (", "foo", ")") == " (foo)"
+ *   optionalBracket(" (", "", ")") == ""
+ *
+ * Design note: this would have been called `optionalParentheses`, except this
+ * function is more general and more explicit. Parentheses typically *also* need
+ * to be prefixed with a space in order to fit nicely in a piece of natural
+ * language.
+ */
+std::string optionalBracket(std::string_view prefix, std::string_view content, std::string_view suffix);
+
+/**
+ * Overload for optional content.
+ *
+ * If `content` is nullopt or contains an empty string, returns an empty string.
+ * Otherwise, returns `prefix + *content + suffix`.
+ *
+ * Example:
+ *   optionalBracket(" (", std::optional<std::string>("foo"), ")") == " (foo)"
+ *   optionalBracket(" (", std::nullopt, ")") == ""
+ *   optionalBracket(" (", std::optional<std::string>(""), ")") == ""
+ */
+template<typename T>
+    requires std::convertible_to<T, std::string_view>
+std::string optionalBracket(std::string_view prefix, const std::optional<T> & content, std::string_view suffix)
+{
+    if (!content || std::string_view(*content).empty()) {
+        return "";
+    }
+    return optionalBracket(prefix, std::string_view(*content), suffix);
+}
+
 /**
  * Hash implementation that can be used for zero-copy heterogenous lookup from
  * P1690R1[1] in unordered containers.

src/libutil/strings.cc

@@ -138,4 +138,18 @@ std::list<std::string> shellSplitString(std::string_view s)
 
     return result;
 }
+
+std::string optionalBracket(std::string_view prefix, std::string_view content, std::string_view suffix)
+{
+    if (content.empty()) {
+        return "";
+    }
+    std::string result;
+    result.reserve(prefix.size() + content.size() + suffix.size());
+    result.append(prefix);
+    result.append(content);
+    result.append(suffix);
+    return result;
+}
+
 } // namespace nix

src/nix/search.cc

@@ -159,7 +159,7 @@ struct CmdSearch : InstallableValueCommand, MixJSON
                             logger->cout(
                                 "* %s%s",
                                 wrap("\e[0;1m", hiliteMatches(attrPath2, attrPathMatches, ANSI_GREEN, "\e[0;1m")),
-                                name.version != "" ? " (" + name.version + ")" : "");
+                                optionalBracket(" (", name.version, ")"));
                             if (description != "")
                                 logger->cout(
                                     "  %s", hiliteMatches(description, descriptionMatches, ANSI_GREEN, ANSI_NORMAL));