From 5dcfddf9972fadf3a188397757eb1727289ab854 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Mon, 13 Oct 2025 13:59:39 +0200 Subject: [PATCH 1/6] strings: Add optionalBracket helper --- src/libutil-tests/strings.cc | 59 +++++++++++++++++++++++++ src/libutil/include/nix/util/strings.hh | 39 ++++++++++++++++ src/libutil/strings.cc | 14 ++++++ 3 files changed, 112 insertions(+) diff --git a/src/libutil-tests/strings.cc b/src/libutil-tests/strings.cc index bd740ce0c..dbbecd514 100644 --- a/src/libutil-tests/strings.cc +++ b/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("foo"), ")"), " (foo)"); +} + +TEST(optionalBracket, optionalNullopt) +{ + ASSERT_EQ(optionalBracket(" (", std::optional(std::nullopt), ")"), ""); +} + +TEST(optionalBracket, optionalEmptyString) +{ + ASSERT_EQ(optionalBracket(" (", std::optional(""), ")"), ""); +} + +TEST(optionalBracket, optionalStringViewWithValue) +{ + ASSERT_EQ(optionalBracket(" (", std::optional("bar"), ")"), " (bar)"); +} + } // namespace nix diff --git a/src/libutil/include/nix/util/strings.hh b/src/libutil/include/nix/util/strings.hh index ba37ce79f..da6decc31 100644 --- a/src/libutil/include/nix/util/strings.hh +++ b/src/libutil/include/nix/util/strings.hh @@ -3,6 +3,7 @@ #include "nix/util/types.hh" #include +#include #include #include #include @@ -93,6 +94,44 @@ extern template std::string dropEmptyInitThenConcatStringsSep(std::string_view, */ std::list 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("foo"), ")") == " (foo)" + * optionalBracket(" (", std::nullopt, ")") == "" + * optionalBracket(" (", std::optional(""), ")") == "" + */ +template + requires std::convertible_to +std::string optionalBracket(std::string_view prefix, const std::optional & 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. diff --git a/src/libutil/strings.cc b/src/libutil/strings.cc index a87567cef..c0c3d6602 100644 --- a/src/libutil/strings.cc +++ b/src/libutil/strings.cc @@ -138,4 +138,18 @@ std::list 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 From 583f5e37fc508e2307fb790188791214fb646b05 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Mon, 13 Oct 2025 14:02:59 +0200 Subject: [PATCH 2/6] Refactor: use optionalBracket in nix search --- src/nix/search.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/nix/search.cc b/src/nix/search.cc index 910450e95..20bb4cd5d 100644 --- a/src/nix/search.cc +++ b/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)); From 0fd890a8d68b128ff4c1e8eefc063589d7910fe1 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Mon, 13 Oct 2025 14:09:49 +0200 Subject: [PATCH 3/6] Add reason string support to MissingExperimentalFeature --- src/libutil/configuration.cc | 4 ++-- src/libutil/experimental-features.cc | 8 +++++--- src/libutil/include/nix/util/configuration.hh | 2 +- src/libutil/include/nix/util/experimental-features.hh | 2 +- 4 files changed, 9 insertions(+), 7 deletions(-) diff --git a/src/libutil/configuration.cc b/src/libutil/configuration.cc index dc9d91f63..7a0ed22ea 100644 --- a/src/libutil/configuration.cc +++ b/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 & feature) const diff --git a/src/libutil/experimental-features.cc b/src/libutil/experimental-features.cc index 0edd5a585..11b8ceadf 100644 --- a/src/libutil/experimental-features.cc +++ b/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 @@ -376,10 +377,11 @@ std::set 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", - showExperimentalFeature(feature)) + "experimental Nix feature '%1%' is disabled%2%; add '--extra-experimental-features %1%' to enable it", + showExperimentalFeature(feature), + Uncolored(optionalBracket(" (", reason, ")"))) , missingFeature(feature) { } diff --git a/src/libutil/include/nix/util/configuration.hh b/src/libutil/include/nix/util/configuration.hh index 65391721c..c8d7b7f24 100644 --- a/src/libutil/include/nix/util/configuration.hh +++ b/src/libutil/include/nix/util/configuration.hh @@ -463,7 +463,7 @@ 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; /** * `std::nullopt` pointer means no feature, which means there is nothing that could be diff --git a/src/libutil/include/nix/util/experimental-features.hh b/src/libutil/include/nix/util/experimental-features.hh index 73c4eeca4..6ffc0e0c0 100644 --- a/src/libutil/include/nix/util/experimental-features.hh +++ b/src/libutil/include/nix/util/experimental-features.hh @@ -88,7 +88,7 @@ public: */ ExperimentalFeature missingFeature; - MissingExperimentalFeature(ExperimentalFeature missingFeature); + MissingExperimentalFeature(ExperimentalFeature missingFeature, std::string reason = ""); }; /** From 71aa9a479883cdf372ed49e717abd277e58f449e Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Mon, 13 Oct 2025 14:20:08 +0200 Subject: [PATCH 4/6] Add reasons to dyndrv xp messages --- src/libexpr/primops.cc | 3 ++- src/libstore/derivations.cc | 8 ++++---- src/libstore/derived-path.cc | 6 +++++- src/libstore/downstream-placeholder.cc | 2 +- 4 files changed, 12 insertions(+), 7 deletions(-) diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index 86cb00131..5f06bf009 100644 --- a/src/libexpr/primops.cc +++ b/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); }; diff --git a/src/libstore/derivations.cc b/src/libstore/derivations.cc index 6d7dbc99c..b5d8d1a1c 100644 --- a/src/libstore/derivations.cc +++ b/src/libstore/derivations.cc @@ -288,7 +288,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); @@ -426,7 +426,7 @@ 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, fmt("derivation '%s', ATerm format version 'xp-dyn-drv'", name)); } else { throw FormatError("Unknown derivation ATerm format version '%s'", *versionS); } @@ -1301,7 +1301,7 @@ DerivationOutput::fromJSON(const nlohmann::json & _json, const ExperimentalFeatu auto methodAlgo = [&]() -> std::pair { 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)}; @@ -1454,7 +1454,7 @@ 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, fmt("dynamic output '%s' in JSON", outputId)); node.childMap[outputId] = doInput(childNode); } return node; diff --git a/src/libstore/derived-path.cc b/src/libstore/derived-path.cc index 2cf720b82..34e591666 100644 --- a/src/libstore/derived-path.cc +++ b/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, + fmt("building output '%s' of '%s'", b.output, b.drvPath->getBaseStorePath().to_string())); + }, }, drv.raw()); } diff --git a/src/libstore/downstream-placeholder.cc b/src/libstore/downstream-placeholder.cc index b3ac1c8c4..30044501b 100644 --- a/src/libstore/downstream-placeholder.cc +++ b/src/libstore/downstream-placeholder.cc @@ -24,7 +24,7 @@ DownstreamPlaceholder DownstreamPlaceholder::unknownDerivation( OutputNameView outputName, const ExperimentalFeatureSettings & xpSettings) { - xpSettings.require(Xp::DynamicDerivations); + xpSettings.require(Xp::DynamicDerivations, 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}; From 39c46654880d66a1bdfe107f6726630ff831707e Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Mon, 13 Oct 2025 23:48:58 +0200 Subject: [PATCH 5/6] Store reason as a field in MissingExperimentalFeature Store the reason string as a field in the exception class rather than only embedding it in the error message. This supports better structured error handling and future JSON error reporting. Suggested by Ericson2314 in PR review. --- src/libutil/experimental-features.cc | 1 + src/libutil/include/nix/util/experimental-features.hh | 2 ++ 2 files changed, 3 insertions(+) diff --git a/src/libutil/experimental-features.cc b/src/libutil/experimental-features.cc index 11b8ceadf..198d021bb 100644 --- a/src/libutil/experimental-features.cc +++ b/src/libutil/experimental-features.cc @@ -383,6 +383,7 @@ MissingExperimentalFeature::MissingExperimentalFeature(ExperimentalFeature featu showExperimentalFeature(feature), Uncolored(optionalBracket(" (", reason, ")"))) , missingFeature(feature) + , reason{reason} { } diff --git a/src/libutil/include/nix/util/experimental-features.hh b/src/libutil/include/nix/util/experimental-features.hh index 6ffc0e0c0..aca14bfbb 100644 --- a/src/libutil/include/nix/util/experimental-features.hh +++ b/src/libutil/include/nix/util/experimental-features.hh @@ -88,6 +88,8 @@ public: */ ExperimentalFeature missingFeature; + std::string reason; + MissingExperimentalFeature(ExperimentalFeature missingFeature, std::string reason = ""); }; From 1b96a704d38b38804d317a7dac3663630ac599e7 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Tue, 14 Oct 2025 16:49:59 +0200 Subject: [PATCH 6/6] Add lazy evaluation for experimental feature reasons Wrap fmt() calls in lambdas to defer string formatting until the feature check fails. This avoids unnecessary string formatting in the common case where the feature is enabled. Addresses performance concern raised by xokdvium in PR review. --- src/libstore/derivations.cc | 7 +++++-- src/libstore/derived-path.cc | 6 +++--- src/libstore/downstream-placeholder.cc | 3 ++- src/libutil/include/nix/util/configuration.hh | 13 +++++++++++++ 4 files changed, 23 insertions(+), 6 deletions(-) diff --git a/src/libstore/derivations.cc b/src/libstore/derivations.cc index b5d8d1a1c..fa8bc58ac 100644 --- a/src/libstore/derivations.cc +++ b/src/libstore/derivations.cc @@ -426,7 +426,9 @@ Derivation parseDerivation( if (*versionS == "xp-dyn-drv"sv) { // Only version we have so far version = DerivationATermVersion::DynamicDerivations; - xpSettings.require(Xp::DynamicDerivations, fmt("derivation '%s', ATerm format version 'xp-dyn-drv'", name)); + 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); } @@ -1454,7 +1456,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, fmt("dynamic output '%s' in JSON", outputId)); + xpSettings.require( + Xp::DynamicDerivations, [&] { return fmt("dynamic output '%s' in JSON", outputId); }); node.childMap[outputId] = doInput(childNode); } return node; diff --git a/src/libstore/derived-path.cc b/src/libstore/derived-path.cc index 34e591666..8d606cb41 100644 --- a/src/libstore/derived-path.cc +++ b/src/libstore/derived-path.cc @@ -86,9 +86,9 @@ void drvRequireExperiment(const SingleDerivedPath & drv, const ExperimentalFeatu // plain drv path; no experimental features required. }, [&](const SingleDerivedPath::Built & b) { - xpSettings.require( - Xp::DynamicDerivations, - fmt("building output '%s' of '%s'", b.output, b.drvPath->getBaseStorePath().to_string())); + xpSettings.require(Xp::DynamicDerivations, [&] { + return fmt("building output '%s' of '%s'", b.output, b.drvPath->getBaseStorePath().to_string()); + }); }, }, drv.raw()); diff --git a/src/libstore/downstream-placeholder.cc b/src/libstore/downstream-placeholder.cc index 30044501b..780717a62 100644 --- a/src/libstore/downstream-placeholder.cc +++ b/src/libstore/downstream-placeholder.cc @@ -24,7 +24,8 @@ DownstreamPlaceholder DownstreamPlaceholder::unknownDerivation( OutputNameView outputName, const ExperimentalFeatureSettings & xpSettings) { - xpSettings.require(Xp::DynamicDerivations, fmt("placeholder for unknown derivation output '%s'", outputName)); + 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}; diff --git a/src/libutil/include/nix/util/configuration.hh b/src/libutil/include/nix/util/configuration.hh index c8d7b7f24..541febdb5 100644 --- a/src/libutil/include/nix/util/configuration.hh +++ b/src/libutil/include/nix/util/configuration.hh @@ -465,6 +465,19 @@ struct ExperimentalFeatureSettings : Config */ 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 + requires std::invocable && std::convertible_to, 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 * disabled, and so the function returns true in that case.