From 1a8defd06f6b9fb1867680a053dd23dfccd5df50 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Fri, 12 Jul 2024 22:09:27 +0200 Subject: [PATCH 01/28] Refactor: rename C++ concatStringsSep -> dropEmptyInitThenConcatStringsSep --- src/build-remote/build-remote.cc | 8 ++++---- src/libcmd/command.cc | 2 +- src/libcmd/installables.cc | 4 ++-- src/libcmd/repl.cc | 2 +- src/libexpr/eval-cache.cc | 6 +++--- src/libflake/flake/config.cc | 2 +- src/libflake/flake/lockfile.cc | 4 ++-- src/libmain/shared.cc | 6 +++--- src/libstore/build/derivation-goal.cc | 4 ++-- src/libstore/build/entry-points.cc | 2 +- src/libstore/globals.cc | 2 +- src/libstore/local-store.cc | 8 ++++---- src/libstore/misc.cc | 2 +- src/libstore/nar-info-disk-cache.cc | 4 ++-- src/libstore/nar-info.cc | 2 +- src/libstore/outputs-spec.cc | 2 +- src/libstore/path-info.cc | 2 +- src/libstore/path-with-outputs.cc | 2 +- src/libstore/store-api.cc | 2 +- src/libstore/unix/build/hook-instance.cc | 2 +- src/libstore/unix/build/local-derivation-goal.cc | 8 ++++---- src/libutil/config.cc | 8 ++++---- src/libutil/util.hh | 4 ++-- src/nix/develop.cc | 2 +- src/nix/diff-closures.cc | 4 ++-- src/nix/env.cc | 2 +- src/nix/flake.cc | 10 +++++----- src/nix/main.cc | 6 +++--- src/nix/path-info.cc | 2 +- src/nix/profile.cc | 6 +++--- src/nix/search.cc | 4 ++-- tests/unit/libutil/references.cc | 2 +- tests/unit/libutil/tests.cc | 14 +++++++------- 33 files changed, 70 insertions(+), 70 deletions(-) diff --git a/src/build-remote/build-remote.cc b/src/build-remote/build-remote.cc index 582e6d623..1c3ce930a 100644 --- a/src/build-remote/build-remote.cc +++ b/src/build-remote/build-remote.cc @@ -206,15 +206,15 @@ static int main_build_remote(int argc, char * * argv) error % drvstr % neededSystem - % concatStringsSep(", ", requiredFeatures) + % dropEmptyInitThenConcatStringsSep(", ", requiredFeatures) % machines.size(); for (auto & m : machines) error - % concatStringsSep(", ", m.systemTypes) + % dropEmptyInitThenConcatStringsSep(", ", m.systemTypes) % m.maxJobs - % concatStringsSep(", ", m.supportedFeatures) - % concatStringsSep(", ", m.mandatoryFeatures); + % dropEmptyInitThenConcatStringsSep(", ", m.supportedFeatures) + % dropEmptyInitThenConcatStringsSep(", ", m.mandatoryFeatures); printMsg(couldBuildLocally ? lvlChatty : lvlWarn, error.str()); diff --git a/src/libcmd/command.cc b/src/libcmd/command.cc index e0e5f0890..891a01f91 100644 --- a/src/libcmd/command.cc +++ b/src/libcmd/command.cc @@ -42,7 +42,7 @@ void NixMultiCommand::run() for (auto & [name, _] : commands) subCommandTextLines.insert(fmt("- `%s`", name)); std::string markdownError = fmt("`nix %s` requires a sub-command. Available sub-commands:\n\n%s\n", - commandName, concatStringsSep("\n", subCommandTextLines)); + commandName, dropEmptyInitThenConcatStringsSep("\n", subCommandTextLines)); throw UsageError(renderMarkdownToTerminal(markdownError)); } command->second->run(); diff --git a/src/libcmd/installables.cc b/src/libcmd/installables.cc index 417e15094..1f6ee1e23 100644 --- a/src/libcmd/installables.cc +++ b/src/libcmd/installables.cc @@ -374,7 +374,7 @@ void completeFlakeRefWithFragment( auto attrPath2 = (*attr)->getAttrPath(attr2); /* Strip the attrpath prefix. */ attrPath2.erase(attrPath2.begin(), attrPath2.begin() + attrPathPrefix.size()); - completions.add(flakeRefS + "#" + prefixRoot + concatStringsSep(".", evalState->symbols.resolve(attrPath2))); + completions.add(flakeRefS + "#" + prefixRoot + dropEmptyInitThenConcatStringsSep(".", evalState->symbols.resolve(attrPath2))); } } } @@ -630,7 +630,7 @@ static void throwBuildErrors( } failedPaths.insert(failedResult->path.to_string(store)); } - throw Error("build of %s failed", concatStringsSep(", ", quoteStrings(failedPaths))); + throw Error("build of %s failed", dropEmptyInitThenConcatStringsSep(", ", quoteStrings(failedPaths))); } } } diff --git a/src/libcmd/repl.cc b/src/libcmd/repl.cc index 3dd19ce39..1d39ef167 100644 --- a/src/libcmd/repl.cc +++ b/src/libcmd/repl.cc @@ -625,7 +625,7 @@ ProcessLineResult NixRepl::processLine(std::string line) markdown += "**Synopsis:** `builtins." + (std::string) (*doc->name) + "` " - + concatStringsSep(" ", args) + "\n\n"; + + dropEmptyInitThenConcatStringsSep(" ", args) + "\n\n"; } markdown += stripIndentation(doc->doc); diff --git a/src/libexpr/eval-cache.cc b/src/libexpr/eval-cache.cc index 46dd3691c..e573fe884 100644 --- a/src/libexpr/eval-cache.cc +++ b/src/libexpr/eval-cache.cc @@ -225,7 +225,7 @@ struct AttrDb (key.first) (symbols[key.second]) (AttrType::ListOfStrings) - (concatStringsSep("\t", l)).exec(); + (dropEmptyInitThenConcatStringsSep("\t", l)).exec(); return state->db.getLastInsertedRowId(); }); @@ -435,12 +435,12 @@ std::vector AttrCursor::getAttrPath(Symbol name) const std::string AttrCursor::getAttrPathStr() const { - return concatStringsSep(".", root->state.symbols.resolve(getAttrPath())); + return dropEmptyInitThenConcatStringsSep(".", root->state.symbols.resolve(getAttrPath())); } std::string AttrCursor::getAttrPathStr(Symbol name) const { - return concatStringsSep(".", root->state.symbols.resolve(getAttrPath(name))); + return dropEmptyInitThenConcatStringsSep(".", root->state.symbols.resolve(getAttrPath(name))); } Value & AttrCursor::forceValue() diff --git a/src/libflake/flake/config.cc b/src/libflake/flake/config.cc index 4e00d5c93..e526cdddf 100644 --- a/src/libflake/flake/config.cc +++ b/src/libflake/flake/config.cc @@ -47,7 +47,7 @@ void ConfigFile::apply(const Settings & flakeSettings) else if (auto* b = std::get_if>(&value)) valueS = b->t ? "true" : "false"; else if (auto ss = std::get_if>(&value)) - valueS = concatStringsSep(" ", *ss); // FIXME: evil + valueS = dropEmptyInitThenConcatStringsSep(" ", *ss); // FIXME: evil else assert(false); diff --git a/src/libflake/flake/lockfile.cc b/src/libflake/flake/lockfile.cc index 792dda740..f0e22a75a 100644 --- a/src/libflake/flake/lockfile.cc +++ b/src/libflake/flake/lockfile.cc @@ -61,7 +61,7 @@ static std::shared_ptr doFind(const ref& root, const InputPath & pat std::vector cycle; std::transform(found, visited.cend(), std::back_inserter(cycle), printInputPath); cycle.push_back(printInputPath(path)); - throw Error("follow cycle detected: [%s]", concatStringsSep(" -> ", cycle)); + throw Error("follow cycle detected: [%s]", dropEmptyInitThenConcatStringsSep(" -> ", cycle)); } visited.push_back(path); @@ -367,7 +367,7 @@ void check(); std::string printInputPath(const InputPath & path) { - return concatStringsSep("/", path); + return dropEmptyInitThenConcatStringsSep("/", path); } } diff --git a/src/libmain/shared.cc b/src/libmain/shared.cc index fee4d0c1e..681c1039f 100644 --- a/src/libmain/shared.cc +++ b/src/libmain/shared.cc @@ -301,11 +301,11 @@ void printVersion(const std::string & programName) #endif cfg.push_back("signed-caches"); std::cout << "System type: " << settings.thisSystem << "\n"; - std::cout << "Additional system types: " << concatStringsSep(", ", settings.extraPlatforms.get()) << "\n"; - std::cout << "Features: " << concatStringsSep(", ", cfg) << "\n"; + std::cout << "Additional system types: " << dropEmptyInitThenConcatStringsSep(", ", settings.extraPlatforms.get()) << "\n"; + std::cout << "Features: " << dropEmptyInitThenConcatStringsSep(", ", cfg) << "\n"; std::cout << "System configuration file: " << settings.nixConfDir + "/nix.conf" << "\n"; std::cout << "User configuration files: " << - concatStringsSep(":", settings.nixUserConfFiles) + dropEmptyInitThenConcatStringsSep(":", settings.nixUserConfFiles) << "\n"; std::cout << "Store directory: " << settings.nixStore << "\n"; std::cout << "State directory: " << settings.nixStateDir << "\n"; diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc index 64b8495e1..c0a784349 100644 --- a/src/libstore/build/derivation-goal.cc +++ b/src/libstore/build/derivation-goal.cc @@ -895,7 +895,7 @@ void runPostBuildHook( std::map hookEnvironment = getEnv(); hookEnvironment.emplace("DRV_PATH", store.printStorePath(drvPath)); - hookEnvironment.emplace("OUT_PATHS", chomp(concatStringsSep(" ", store.printStorePathSet(outputPaths)))); + hookEnvironment.emplace("OUT_PATHS", chomp(dropEmptyInitThenConcatStringsSep(" ", store.printStorePathSet(outputPaths)))); hookEnvironment.emplace("NIX_CONFIG", globalConfig.toKeyValue()); struct LogSink : Sink { @@ -1505,7 +1505,7 @@ std::pair DerivationGoal::checkPathValidity() if (!wantedOutputsLeft.empty()) throw Error("derivation '%s' does not have wanted outputs %s", worker.store.printStorePath(drvPath), - concatStringsSep(", ", quoteStrings(wantedOutputsLeft))); + dropEmptyInitThenConcatStringsSep(", ", quoteStrings(wantedOutputsLeft))); bool allValid = true; for (auto & [_, status] : initialOutputs) { diff --git a/src/libstore/build/entry-points.cc b/src/libstore/build/entry-points.cc index 784f618c1..8bf7ad35d 100644 --- a/src/libstore/build/entry-points.cc +++ b/src/libstore/build/entry-points.cc @@ -42,7 +42,7 @@ void Store::buildPaths(const std::vector & reqs, BuildMode buildMod throw std::move(*ex); } else if (!failed.empty()) { if (ex) logError(ex->info()); - throw Error(worker.failingExitStatus(), "build of %s failed", concatStringsSep(", ", quoteStrings(failed))); + throw Error(worker.failingExitStatus(), "build of %s failed", dropEmptyInitThenConcatStringsSep(", ", quoteStrings(failed))); } } diff --git a/src/libstore/globals.cc b/src/libstore/globals.cc index 7e1d7ea6d..5f5b4f89b 100644 --- a/src/libstore/globals.cc +++ b/src/libstore/globals.cc @@ -82,7 +82,7 @@ Settings::Settings() Strings ss; for (auto & p : tokenizeString(*s, ":")) ss.push_back("@" + p); - builders = concatStringsSep(" ", ss); + builders = dropEmptyInitThenConcatStringsSep(" ", ss); } #if defined(__linux__) && defined(SANDBOX_SHELL) diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 2b4e01eb3..8764b88b7 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -656,7 +656,7 @@ void LocalStore::registerDrvOutput(const Realisation & info) combinedSignatures.insert(info.signatures.begin(), info.signatures.end()); state->stmts->UpdateRealisedOutput.use() - (concatStringsSep(" ", combinedSignatures)) + (dropEmptyInitThenConcatStringsSep(" ", combinedSignatures)) (info.id.strHash()) (info.id.outputName) .exec(); @@ -675,7 +675,7 @@ void LocalStore::registerDrvOutput(const Realisation & info) (info.id.strHash()) (info.id.outputName) (printStorePath(info.outPath)) - (concatStringsSep(" ", info.signatures)) + (dropEmptyInitThenConcatStringsSep(" ", info.signatures)) .exec(); } for (auto & [outputId, depPath] : info.dependentRealisations) { @@ -729,7 +729,7 @@ uint64_t LocalStore::addValidPath(State & state, (info.deriver ? printStorePath(*info.deriver) : "", (bool) info.deriver) (info.narSize, info.narSize != 0) (info.ultimate ? 1 : 0, info.ultimate) - (concatStringsSep(" ", info.sigs), !info.sigs.empty()) + (dropEmptyInitThenConcatStringsSep(" ", info.sigs), !info.sigs.empty()) (renderContentAddress(info.ca), (bool) info.ca) .exec(); uint64_t id = state.db.getLastInsertedRowId(); @@ -833,7 +833,7 @@ void LocalStore::updatePathInfo(State & state, const ValidPathInfo & info) (info.narSize, info.narSize != 0) (info.narHash.to_string(HashFormat::Base16, true)) (info.ultimate ? 1 : 0, info.ultimate) - (concatStringsSep(" ", info.sigs), !info.sigs.empty()) + (dropEmptyInitThenConcatStringsSep(" ", info.sigs), !info.sigs.empty()) (renderContentAddress(info.ca), (bool) info.ca) (printStorePath(info.path)) .exec(); diff --git a/src/libstore/misc.cc b/src/libstore/misc.cc index dd0efbe19..179e5478c 100644 --- a/src/libstore/misc.cc +++ b/src/libstore/misc.cc @@ -464,7 +464,7 @@ OutputPathMap resolveDerivedPath(Store & store, const DerivedPath::Built & bfd) if (!outputsLeft.empty()) throw Error("derivation '%s' does not have an outputs %s", store.printStorePath(drvPath), - concatStringsSep(", ", quoteStrings(std::get(bfd.outputs.raw)))); + dropEmptyInitThenConcatStringsSep(", ", quoteStrings(std::get(bfd.outputs.raw)))); return outputMap; } diff --git a/src/libstore/nar-info-disk-cache.cc b/src/libstore/nar-info-disk-cache.cc index 07beb8acb..42d172237 100644 --- a/src/libstore/nar-info-disk-cache.cc +++ b/src/libstore/nar-info-disk-cache.cc @@ -337,9 +337,9 @@ public: (narInfo ? narInfo->fileSize : 0, narInfo != 0 && narInfo->fileSize) (info->narHash.to_string(HashFormat::Nix32, true)) (info->narSize) - (concatStringsSep(" ", info->shortRefs())) + (dropEmptyInitThenConcatStringsSep(" ", info->shortRefs())) (info->deriver ? std::string(info->deriver->to_string()) : "", (bool) info->deriver) - (concatStringsSep(" ", info->sigs)) + (dropEmptyInitThenConcatStringsSep(" ", info->sigs)) (renderContentAddress(info->ca)) (time(0)).exec(); diff --git a/src/libstore/nar-info.cc b/src/libstore/nar-info.cc index 3e0a754f9..577466f55 100644 --- a/src/libstore/nar-info.cc +++ b/src/libstore/nar-info.cc @@ -111,7 +111,7 @@ std::string NarInfo::to_string(const Store & store) const res += "NarHash: " + narHash.to_string(HashFormat::Nix32, true) + "\n"; res += "NarSize: " + std::to_string(narSize) + "\n"; - res += "References: " + concatStringsSep(" ", shortRefs()) + "\n"; + res += "References: " + dropEmptyInitThenConcatStringsSep(" ", shortRefs()) + "\n"; if (deriver) res += "Deriver: " + std::string(deriver->to_string()) + "\n"; diff --git a/src/libstore/outputs-spec.cc b/src/libstore/outputs-spec.cc index 21c069223..4ed8f95ae 100644 --- a/src/libstore/outputs-spec.cc +++ b/src/libstore/outputs-spec.cc @@ -83,7 +83,7 @@ std::string OutputsSpec::to_string() const return "*"; }, [&](const OutputsSpec::Names & outputNames) -> std::string { - return concatStringsSep(",", outputNames); + return dropEmptyInitThenConcatStringsSep(",", outputNames); }, }, raw); } diff --git a/src/libstore/path-info.cc b/src/libstore/path-info.cc index 51ed5fc62..a13bb8bef 100644 --- a/src/libstore/path-info.cc +++ b/src/libstore/path-info.cc @@ -30,7 +30,7 @@ std::string ValidPathInfo::fingerprint(const Store & store) const "1;" + store.printStorePath(path) + ";" + narHash.to_string(HashFormat::Nix32, true) + ";" + std::to_string(narSize) + ";" - + concatStringsSep(",", store.printStorePathSet(references)); + + dropEmptyInitThenConcatStringsSep(",", store.printStorePathSet(references)); } diff --git a/src/libstore/path-with-outputs.cc b/src/libstore/path-with-outputs.cc index 026e37647..5fa38d5d9 100644 --- a/src/libstore/path-with-outputs.cc +++ b/src/libstore/path-with-outputs.cc @@ -9,7 +9,7 @@ std::string StorePathWithOutputs::to_string(const StoreDirConfig & store) const { return outputs.empty() ? store.printStorePath(path) - : store.printStorePath(path) + "!" + concatStringsSep(",", outputs); + : store.printStorePath(path) + "!" + dropEmptyInitThenConcatStringsSep(",", outputs); } diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc index 05c4e1c5e..6904996b5 100644 --- a/src/libstore/store-api.cc +++ b/src/libstore/store-api.cc @@ -1208,7 +1208,7 @@ std::string StoreDirConfig::showPaths(const StorePathSet & paths) std::string showPaths(const PathSet & paths) { - return concatStringsSep(", ", quoteStrings(paths)); + return dropEmptyInitThenConcatStringsSep(", ", quoteStrings(paths)); } diff --git a/src/libstore/unix/build/hook-instance.cc b/src/libstore/unix/build/hook-instance.cc index dfc208798..ba6c3a912 100644 --- a/src/libstore/unix/build/hook-instance.cc +++ b/src/libstore/unix/build/hook-instance.cc @@ -8,7 +8,7 @@ namespace nix { HookInstance::HookInstance() { - debug("starting build hook '%s'", concatStringsSep(" ", settings.buildHook.get())); + debug("starting build hook '%s'", dropEmptyInitThenConcatStringsSep(" ", settings.buildHook.get())); auto buildHookArgs = settings.buildHook.get(); diff --git a/src/libstore/unix/build/local-derivation-goal.cc b/src/libstore/unix/build/local-derivation-goal.cc index d5a3e0034..2ab4334e9 100644 --- a/src/libstore/unix/build/local-derivation-goal.cc +++ b/src/libstore/unix/build/local-derivation-goal.cc @@ -496,10 +496,10 @@ void LocalDerivationGoal::startBuilder() if (!parsedDrv->canBuildLocally(worker.store)) throw Error("a '%s' with features {%s} is required to build '%s', but I am a '%s' with features {%s}", drv->platform, - concatStringsSep(", ", parsedDrv->getRequiredSystemFeatures()), + dropEmptyInitThenConcatStringsSep(", ", parsedDrv->getRequiredSystemFeatures()), worker.store.printStorePath(drvPath), settings.thisSystem, - concatStringsSep(", ", worker.store.systemFeatures)); + dropEmptyInitThenConcatStringsSep(", ", worker.store.systemFeatures)); /* Create a temporary directory where the build will take place. */ @@ -840,7 +840,7 @@ void LocalDerivationGoal::startBuilder() /* Run the builder. */ printMsg(lvlChatty, "executing builder '%1%'", drv->builder); - printMsg(lvlChatty, "using builder args '%1%'", concatStringsSep(" ", drv->args)); + printMsg(lvlChatty, "using builder args '%1%'", dropEmptyInitThenConcatStringsSep(" ", drv->args)); for (auto & i : drv->env) printMsg(lvlVomit, "setting builder env variable '%1%'='%2%'", i.first, i.second); @@ -1063,7 +1063,7 @@ void LocalDerivationGoal::startBuilder() e.addTrace({}, "while waiting for the build environment for '%s' to initialize (%s, previous messages: %s)", worker.store.printStorePath(drvPath), statusToString(status), - concatStringsSep("|", msgs)); + dropEmptyInitThenConcatStringsSep("|", msgs)); throw; } }(); diff --git a/src/libutil/config.cc b/src/libutil/config.cc index 907ca7fc1..81ec9a4c3 100644 --- a/src/libutil/config.cc +++ b/src/libutil/config.cc @@ -152,7 +152,7 @@ static void parseConfigFiles(const std::string & contents, const std::string & p parsedContents.push_back({ std::move(name), - concatStringsSep(" ", Strings(i, tokens.end())), + dropEmptyInitThenConcatStringsSep(" ", Strings(i, tokens.end())), }); }; } @@ -318,7 +318,7 @@ template<> void BaseSetting::appendOrSet(Strings newValue, bool append) template<> std::string BaseSetting::to_string() const { - return concatStringsSep(" ", value); + return dropEmptyInitThenConcatStringsSep(" ", value); } template<> StringSet BaseSetting::parse(const std::string & str) const @@ -334,7 +334,7 @@ template<> void BaseSetting::appendOrSet(StringSet newValue, bool app template<> std::string BaseSetting::to_string() const { - return concatStringsSep(" ", value); + return dropEmptyInitThenConcatStringsSep(" ", value); } template<> std::set BaseSetting>::parse(const std::string & str) const @@ -362,7 +362,7 @@ template<> std::string BaseSetting>::to_string() c StringSet stringifiedXpFeatures; for (const auto & feature : value) stringifiedXpFeatures.insert(std::string(showExperimentalFeature(feature))); - return concatStringsSep(" ", stringifiedXpFeatures); + return dropEmptyInitThenConcatStringsSep(" ", stringifiedXpFeatures); } template<> StringMap BaseSetting::parse(const std::string & str) const diff --git a/src/libutil/util.hh b/src/libutil/util.hh index 23682ff7c..66cef62ed 100644 --- a/src/libutil/util.hh +++ b/src/libutil/util.hh @@ -37,7 +37,7 @@ template C tokenizeString(std::string_view s, std::string_view separato * elements. */ template -std::string concatStringsSep(const std::string_view sep, const C & ss) +std::string dropEmptyInitThenConcatStringsSep(const std::string_view sep, const C & ss) { size_t size = 0; // need a cast to string_view since this is also called with Symbols @@ -56,7 +56,7 @@ auto concatStrings(Parts && ... parts) -> std::enable_if_t<(... && std::is_convertible_v), std::string> { std::string_view views[sizeof...(parts)] = { parts... }; - return concatStringsSep({}, views); + return dropEmptyInitThenConcatStringsSep({}, views); } diff --git a/src/nix/develop.cc b/src/nix/develop.cc index 6bd3dc9ef..807c75d4a 100644 --- a/src/nix/develop.cc +++ b/src/nix/develop.cc @@ -608,7 +608,7 @@ struct CmdDevelop : Common, MixEnvironment std::vector args; for (auto s : command) args.push_back(shellEscape(s)); - script += fmt("exec %s\n", concatStringsSep(" ", args)); + script += fmt("exec %s\n", dropEmptyInitThenConcatStringsSep(" ", args)); } else { diff --git a/src/nix/diff-closures.cc b/src/nix/diff-closures.cc index c7c37b66f..204213396 100644 --- a/src/nix/diff-closures.cc +++ b/src/nix/diff-closures.cc @@ -49,7 +49,7 @@ std::string showVersions(const std::set & versions) std::set versions2; for (auto & version : versions) versions2.insert(version.empty() ? "ε" : version); - return concatStringsSep(", ", versions2); + return dropEmptyInitThenConcatStringsSep(", ", versions2); } void printClosureDiff( @@ -97,7 +97,7 @@ void printClosureDiff( items.push_back(fmt("%s → %s", showVersions(removed), showVersions(added))); if (showDelta) items.push_back(fmt("%s%+.1f KiB" ANSI_NORMAL, sizeDelta > 0 ? ANSI_RED : ANSI_GREEN, sizeDelta / 1024.0)); - logger->cout("%s%s: %s", indent, name, concatStringsSep(", ", items)); + logger->cout("%s%s: %s", indent, name, dropEmptyInitThenConcatStringsSep(", ", items)); } } } diff --git a/src/nix/env.cc b/src/nix/env.cc index bc9cd91ad..7cc019c1d 100644 --- a/src/nix/env.cc +++ b/src/nix/env.cc @@ -94,7 +94,7 @@ struct CmdShell : InstallablesCommand, MixEnvironment auto unixPath = tokenizeString(getEnv("PATH").value_or(""), ":"); unixPath.insert(unixPath.begin(), pathAdditions.begin(), pathAdditions.end()); - auto unixPathString = concatStringsSep(":", unixPath); + auto unixPathString = dropEmptyInitThenConcatStringsSep(":", unixPath); setEnv("PATH", unixPathString.c_str()); Strings args; diff --git a/src/nix/flake.cc b/src/nix/flake.cc index cb73778b3..3ee2b1838 100644 --- a/src/nix/flake.cc +++ b/src/nix/flake.cc @@ -805,7 +805,7 @@ struct CmdFlakeCheck : FlakeCommand warn( "The check omitted these incompatible systems: %s\n" "Use '--all-systems' to check all.", - concatStringsSep(", ", omittedSystems) + dropEmptyInitThenConcatStringsSep(", ", omittedSystems) ); }; }; @@ -1211,7 +1211,7 @@ struct CmdFlakeShow : FlakeCommand, MixJSON auto attrPathS = state->symbols.resolve(attrPath); Activity act(*logger, lvlInfo, actUnknown, - fmt("evaluating '%s'", concatStringsSep(".", attrPathS))); + fmt("evaluating '%s'", dropEmptyInitThenConcatStringsSep(".", attrPathS))); try { auto recurse = [&]() @@ -1291,7 +1291,7 @@ struct CmdFlakeShow : FlakeCommand, MixJSON if (!json) logger->cout(fmt("%s " ANSI_WARNING "omitted" ANSI_NORMAL " (use '--all-systems' to show)", headerPrefix)); else { - logger->warn(fmt("%s omitted (use '--all-systems' to show)", concatStringsSep(".", attrPathS))); + logger->warn(fmt("%s omitted (use '--all-systems' to show)", dropEmptyInitThenConcatStringsSep(".", attrPathS))); } } else { if (visitor.isDerivation()) @@ -1315,13 +1315,13 @@ struct CmdFlakeShow : FlakeCommand, MixJSON if (!json) logger->cout(fmt("%s " ANSI_WARNING "omitted" ANSI_NORMAL " (use '--legacy' to show)", headerPrefix)); else { - logger->warn(fmt("%s omitted (use '--legacy' to show)", concatStringsSep(".", attrPathS))); + logger->warn(fmt("%s omitted (use '--legacy' to show)", dropEmptyInitThenConcatStringsSep(".", attrPathS))); } } else if (!showAllSystems && std::string(attrPathS[1]) != localSystem) { if (!json) logger->cout(fmt("%s " ANSI_WARNING "omitted" ANSI_NORMAL " (use '--all-systems' to show)", headerPrefix)); else { - logger->warn(fmt("%s omitted (use '--all-systems' to show)", concatStringsSep(".", attrPathS))); + logger->warn(fmt("%s omitted (use '--all-systems' to show)", dropEmptyInitThenConcatStringsSep(".", attrPathS))); } } else { if (visitor.isDerivation()) diff --git a/src/nix/main.cc b/src/nix/main.cc index e39f79f1f..aa4ced623 100644 --- a/src/nix/main.cc +++ b/src/nix/main.cc @@ -185,7 +185,7 @@ struct NixArgs : virtual MultiCommand, virtual MixCommonArgs, virtual RootArgs auto & info = i->second; if (info.status == AliasStatus::Deprecated) { warn("'%s' is a deprecated alias for '%s'", - arg, concatStringsSep(" ", info.replacement)); + arg, dropEmptyInitThenConcatStringsSep(" ", info.replacement)); } pos = args.erase(pos); for (auto j = info.replacement.rbegin(); j != info.replacement.rend(); ++j) @@ -238,7 +238,7 @@ struct NixArgs : virtual MultiCommand, virtual MixCommonArgs, virtual RootArgs lowdown. */ static void showHelp(std::vector subcommand, NixArgs & toplevel) { - auto mdName = subcommand.empty() ? "nix" : fmt("nix3-%s", concatStringsSep("-", subcommand)); + auto mdName = subcommand.empty() ? "nix" : fmt("nix3-%s", dropEmptyInitThenConcatStringsSep("-", subcommand)); evalSettings.restrictEval = false; evalSettings.pureEval = false; @@ -273,7 +273,7 @@ static void showHelp(std::vector subcommand, NixArgs & toplevel) auto attr = vRes->attrs()->get(state.symbols.create(mdName + ".md")); if (!attr) - throw UsageError("Nix has no subcommand '%s'", concatStringsSep("", subcommand)); + throw UsageError("Nix has no subcommand '%s'", dropEmptyInitThenConcatStringsSep("", subcommand)); auto markdown = state.forceString(*attr->value, noPos, "while evaluating the lowdown help text"); diff --git a/src/nix/path-info.cc b/src/nix/path-info.cc index 47f9baee5..2383fbe08 100644 --- a/src/nix/path-info.cc +++ b/src/nix/path-info.cc @@ -185,7 +185,7 @@ struct CmdPathInfo : StorePathsCommand, MixJSON if (info->ultimate) ss.push_back("ultimate"); if (info->ca) ss.push_back("ca:" + renderContentAddress(*info->ca)); for (auto & sig : info->sigs) ss.push_back(sig); - std::cout << concatStringsSep(" ", ss); + std::cout << dropEmptyInitThenConcatStringsSep(" ", ss); } std::cout << std::endl; diff --git a/src/nix/profile.cc b/src/nix/profile.cc index 78532a2ec..c21eb3040 100644 --- a/src/nix/profile.cc +++ b/src/nix/profile.cc @@ -58,7 +58,7 @@ struct ProfileElement StringSet names; for (auto & path : storePaths) names.insert(DrvName(path.name()).name); - return concatStringsSep(", ", names); + return dropEmptyInitThenConcatStringsSep(", ", names); } /** @@ -472,7 +472,7 @@ struct CmdProfileInstall : InstallablesCommand, MixDefaultProfile originalConflictingFilePath, newConflictingFilePath, originalEntryName, - concatStringsSep(" ", newConflictingRefs), + dropEmptyInitThenConcatStringsSep(" ", newConflictingRefs), conflictError.priority, conflictError.priority - 1, conflictError.priority + 1 @@ -813,7 +813,7 @@ struct CmdProfileList : virtual EvalCommand, virtual StoreCommand, MixDefaultPro logger->cout("Original flake URL: %s", element.source->originalRef.to_string()); logger->cout("Locked flake URL: %s", element.source->lockedRef.to_string()); } - logger->cout("Store paths: %s", concatStringsSep(" ", store->printStorePathSet(element.storePaths))); + logger->cout("Store paths: %s", dropEmptyInitThenConcatStringsSep(" ", store->printStorePathSet(element.storePaths))); } } } diff --git a/src/nix/search.cc b/src/nix/search.cc index 97ef1375e..d709774ad 100644 --- a/src/nix/search.cc +++ b/src/nix/search.cc @@ -96,7 +96,7 @@ struct CmdSearch : InstallableValueCommand, MixJSON auto attrPathS = state->symbols.resolve(attrPath); Activity act(*logger, lvlInfo, actUnknown, - fmt("evaluating '%s'", concatStringsSep(".", attrPathS))); + fmt("evaluating '%s'", dropEmptyInitThenConcatStringsSep(".", attrPathS))); try { auto recurse = [&]() { @@ -115,7 +115,7 @@ struct CmdSearch : InstallableValueCommand, MixJSON auto aDescription = aMeta ? aMeta->maybeGetAttr(state->sDescription) : nullptr; auto description = aDescription ? aDescription->getString() : ""; std::replace(description.begin(), description.end(), '\n', ' '); - auto attrPath2 = concatStringsSep(".", attrPathS); + auto attrPath2 = dropEmptyInitThenConcatStringsSep(".", attrPathS); std::vector attrPathMatches; std::vector descriptionMatches; diff --git a/tests/unit/libutil/references.cc b/tests/unit/libutil/references.cc index a517d9aa1..c3efa6d51 100644 --- a/tests/unit/libutil/references.cc +++ b/tests/unit/libutil/references.cc @@ -15,7 +15,7 @@ struct RewriteParams { strRewrites.insert(from + "->" + to); return os << "OriginalString: " << bar.originalString << std::endl << - "Rewrites: " << concatStringsSep(",", strRewrites) << std::endl << + "Rewrites: " << dropEmptyInitThenConcatStringsSep(",", strRewrites) << std::endl << "Expected result: " << bar.finalString; } }; diff --git a/tests/unit/libutil/tests.cc b/tests/unit/libutil/tests.cc index 9be4a400d..8a3ca8561 100644 --- a/tests/unit/libutil/tests.cc +++ b/tests/unit/libutil/tests.cc @@ -227,32 +227,32 @@ namespace nix { } /* ---------------------------------------------------------------------------- - * concatStringsSep + * dropEmptyInitThenConcatStringsSep * --------------------------------------------------------------------------*/ - TEST(concatStringsSep, buildCommaSeparatedString) { + TEST(dropEmptyInitThenConcatStringsSep, buildCommaSeparatedString) { Strings strings; strings.push_back("this"); strings.push_back("is"); strings.push_back("great"); - ASSERT_EQ(concatStringsSep(",", strings), "this,is,great"); + ASSERT_EQ(dropEmptyInitThenConcatStringsSep(",", strings), "this,is,great"); } - TEST(concatStringsSep, buildStringWithEmptySeparator) { + TEST(dropEmptyInitThenConcatStringsSep, buildStringWithEmptySeparator) { Strings strings; strings.push_back("this"); strings.push_back("is"); strings.push_back("great"); - ASSERT_EQ(concatStringsSep("", strings), "thisisgreat"); + ASSERT_EQ(dropEmptyInitThenConcatStringsSep("", strings), "thisisgreat"); } - TEST(concatStringsSep, buildSingleString) { + TEST(dropEmptyInitThenConcatStringsSep, buildSingleString) { Strings strings; strings.push_back("this"); - ASSERT_EQ(concatStringsSep(",", strings), "this"); + ASSERT_EQ(dropEmptyInitThenConcatStringsSep(",", strings), "this"); } /* ---------------------------------------------------------------------------- From 79eb0adf9db942609b22d55ff764d1e759453543 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Fri, 12 Jul 2024 22:33:14 +0200 Subject: [PATCH 02/28] dropEmptyInitThenConcatStringSep: Check that we don't drop... ... initial empty strings. The tests pass, which is encouraging. --- src/libexpr/symbol-table.hh | 5 +++++ src/libutil/util.hh | 7 +++++++ 2 files changed, 12 insertions(+) diff --git a/src/libexpr/symbol-table.hh b/src/libexpr/symbol-table.hh index b85725e12..c7a3563b0 100644 --- a/src/libexpr/symbol-table.hh +++ b/src/libexpr/symbol-table.hh @@ -41,6 +41,11 @@ public: } friend std::ostream & operator <<(std::ostream & os, const SymbolStr & symbol); + + bool empty() const + { + return s->empty(); + } }; /** diff --git a/src/libutil/util.hh b/src/libutil/util.hh index 66cef62ed..c545afd9e 100644 --- a/src/libutil/util.hh +++ b/src/libutil/util.hh @@ -40,6 +40,13 @@ template std::string dropEmptyInitThenConcatStringsSep(const std::string_view sep, const C & ss) { size_t size = 0; + + for (auto & i : ss) { + // Make sure we don't rely on the empty item ignoring behavior + assert(!i.empty()); + break; + } + // need a cast to string_view since this is also called with Symbols for (const auto & s : ss) size += sep.size() + std::string_view(s).size(); std::string s; From a681d354e717a549595e9b687567dc7d05eaf29d Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Fri, 12 Jul 2024 23:02:08 +0200 Subject: [PATCH 03/28] Add fresh concatStringsSep without bug The buggy version was previously renamed to dropEmptyInitThenConcatStringsSep --- src/libutil/meson.build | 3 ++ src/libutil/strings-inline.hh | 31 +++++++++++++ src/libutil/strings.cc | 12 +++++ src/libutil/strings.hh | 21 +++++++++ tests/unit/libutil/strings.cc | 83 +++++++++++++++++++++++++++++++++++ 5 files changed, 150 insertions(+) create mode 100644 src/libutil/strings-inline.hh create mode 100644 src/libutil/strings.cc create mode 100644 src/libutil/strings.hh create mode 100644 tests/unit/libutil/strings.cc diff --git a/src/libutil/meson.build b/src/libutil/meson.build index ac2b83536..fbfcbe67c 100644 --- a/src/libutil/meson.build +++ b/src/libutil/meson.build @@ -148,6 +148,7 @@ sources = files( 'signature/signer.cc', 'source-accessor.cc', 'source-path.cc', + 'strings.cc', 'suggestions.cc', 'tarfile.cc', 'terminal.cc', @@ -215,6 +216,8 @@ headers = [config_h] + files( 'source-accessor.hh', 'source-path.hh', 'split.hh', + 'strings.hh', + 'strings-inline.hh', 'suggestions.hh', 'sync.hh', 'tarfile.hh', diff --git a/src/libutil/strings-inline.hh b/src/libutil/strings-inline.hh new file mode 100644 index 000000000..10c1b19e6 --- /dev/null +++ b/src/libutil/strings-inline.hh @@ -0,0 +1,31 @@ +#pragma once + +#include "strings.hh" + +namespace nix { + +template +std::string concatStringsSep(const std::string_view sep, const C & ss) +{ + size_t size = 0; + bool tail = false; + // need a cast to string_view since this is also called with Symbols + for (const auto & s : ss) { + if (tail) + size += sep.size(); + size += std::string_view(s).size(); + tail = true; + } + std::string s; + s.reserve(size); + tail = false; + for (auto & i : ss) { + if (tail) + s += sep; + s += i; + tail = true; + } + return s; +} + +} // namespace nix diff --git a/src/libutil/strings.cc b/src/libutil/strings.cc new file mode 100644 index 000000000..15937d415 --- /dev/null +++ b/src/libutil/strings.cc @@ -0,0 +1,12 @@ +#include + +#include "strings-inline.hh" +#include "util.hh" + +namespace nix { + +template std::string concatStringsSep(std::string_view, const Strings &); +template std::string concatStringsSep(std::string_view, const StringSet &); +template std::string concatStringsSep(std::string_view, const std::vector &); + +} // namespace nix diff --git a/src/libutil/strings.hh b/src/libutil/strings.hh new file mode 100644 index 000000000..3b112c409 --- /dev/null +++ b/src/libutil/strings.hh @@ -0,0 +1,21 @@ +#pragma once + +#include +#include +#include +#include +#include + +namespace nix { + +/** + * Concatenate the given strings with a separator between the elements. + */ +template +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 &); + +} diff --git a/tests/unit/libutil/strings.cc b/tests/unit/libutil/strings.cc new file mode 100644 index 000000000..47a20770e --- /dev/null +++ b/tests/unit/libutil/strings.cc @@ -0,0 +1,83 @@ +#include + +#include "strings.hh" + +namespace nix { + +using Strings = std::vector; + +/* ---------------------------------------------------------------------------- + * concatStringsSep + * --------------------------------------------------------------------------*/ + +TEST(concatStringsSep, empty) +{ + Strings strings; + + ASSERT_EQ(concatStringsSep(",", strings), ""); +} + +TEST(concatStringsSep, justOne) +{ + Strings strings; + strings.push_back("this"); + + ASSERT_EQ(concatStringsSep(",", strings), "this"); +} + +TEST(concatStringsSep, emptyString) +{ + Strings strings; + strings.push_back(""); + + ASSERT_EQ(concatStringsSep(",", strings), ""); +} + +TEST(concatStringsSep, emptyStrings) +{ + Strings strings; + strings.push_back(""); + strings.push_back(""); + + ASSERT_EQ(concatStringsSep(",", strings), ","); +} + +TEST(concatStringsSep, threeEmptyStrings) +{ + Strings strings; + strings.push_back(""); + strings.push_back(""); + strings.push_back(""); + + ASSERT_EQ(concatStringsSep(",", strings), ",,"); +} + +TEST(concatStringsSep, buildCommaSeparatedString) +{ + Strings strings; + strings.push_back("this"); + strings.push_back("is"); + strings.push_back("great"); + + ASSERT_EQ(concatStringsSep(",", strings), "this,is,great"); +} + +TEST(concatStringsSep, buildStringWithEmptySeparator) +{ + Strings strings; + strings.push_back("this"); + strings.push_back("is"); + strings.push_back("great"); + + ASSERT_EQ(concatStringsSep("", strings), "thisisgreat"); +} + +TEST(concatStringsSep, buildSingleString) +{ + Strings strings; + strings.push_back("this"); + + ASSERT_EQ(concatStringsSep(",", strings), "this"); +} + +} // namespace nix From ea966a70fcae93538f41ff413fb9cec852054b3c Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Fri, 12 Jul 2024 23:06:32 +0200 Subject: [PATCH 04/28] dropEmptyInitThenConcatStringsSep -> concatStringSep: diagnostics and docs These are non-critical, so their behavior is ok to change. Dropping empty items is not needed and usually not expected. --- src/build-remote/build-remote.cc | 9 +++++---- src/libcmd/command.cc | 7 ++++--- src/libcmd/installables.cc | 4 +++- src/libcmd/repl.cc | 4 +++- src/libflake/flake/lockfile.cc | 6 ++++-- src/libmain/shared.cc | 7 ++++--- src/libstore/build/entry-points.cc | 3 ++- src/libstore/misc.cc | 1 + src/libstore/unix/build/hook-instance.cc | 3 ++- 9 files changed, 28 insertions(+), 16 deletions(-) diff --git a/src/build-remote/build-remote.cc b/src/build-remote/build-remote.cc index 1c3ce930a..600fc7ee2 100644 --- a/src/build-remote/build-remote.cc +++ b/src/build-remote/build-remote.cc @@ -16,6 +16,7 @@ #include "serialise.hh" #include "build-result.hh" #include "store-api.hh" +#include "strings.hh" #include "derivations.hh" #include "local-store.hh" #include "legacy.hh" @@ -206,15 +207,15 @@ static int main_build_remote(int argc, char * * argv) error % drvstr % neededSystem - % dropEmptyInitThenConcatStringsSep(", ", requiredFeatures) + % concatStringsSep(", ", requiredFeatures) % machines.size(); for (auto & m : machines) error - % dropEmptyInitThenConcatStringsSep(", ", m.systemTypes) + % concatStringsSep(", ", m.systemTypes) % m.maxJobs - % dropEmptyInitThenConcatStringsSep(", ", m.supportedFeatures) - % dropEmptyInitThenConcatStringsSep(", ", m.mandatoryFeatures); + % concatStringsSep(", ", m.supportedFeatures) + % concatStringsSep(", ", m.mandatoryFeatures); printMsg(couldBuildLocally ? lvlChatty : lvlWarn, error.str()); diff --git a/src/libcmd/command.cc b/src/libcmd/command.cc index 891a01f91..67fef1909 100644 --- a/src/libcmd/command.cc +++ b/src/libcmd/command.cc @@ -1,3 +1,5 @@ +#include + #include "command.hh" #include "markdown.hh" #include "store-api.hh" @@ -6,8 +8,7 @@ #include "nixexpr.hh" #include "profiles.hh" #include "repl.hh" - -#include +#include "strings.hh" extern char * * environ __attribute__((weak)); @@ -42,7 +43,7 @@ void NixMultiCommand::run() for (auto & [name, _] : commands) subCommandTextLines.insert(fmt("- `%s`", name)); std::string markdownError = fmt("`nix %s` requires a sub-command. Available sub-commands:\n\n%s\n", - commandName, dropEmptyInitThenConcatStringsSep("\n", subCommandTextLines)); + commandName, concatStringsSep("\n", subCommandTextLines)); throw UsageError(renderMarkdownToTerminal(markdownError)); } command->second->run(); diff --git a/src/libcmd/installables.cc b/src/libcmd/installables.cc index 1f6ee1e23..406e4bfd8 100644 --- a/src/libcmd/installables.cc +++ b/src/libcmd/installables.cc @@ -27,6 +27,8 @@ #include +#include "strings-inline.hh" + namespace nix { void completeFlakeInputPath( @@ -630,7 +632,7 @@ static void throwBuildErrors( } failedPaths.insert(failedResult->path.to_string(store)); } - throw Error("build of %s failed", dropEmptyInitThenConcatStringsSep(", ", quoteStrings(failedPaths))); + throw Error("build of %s failed", concatStringsSep(", ", quoteStrings(failedPaths))); } } } diff --git a/src/libcmd/repl.cc b/src/libcmd/repl.cc index 1d39ef167..37a34e3de 100644 --- a/src/libcmd/repl.cc +++ b/src/libcmd/repl.cc @@ -33,6 +33,8 @@ #include #endif +#include "strings.hh" + namespace nix { /** @@ -625,7 +627,7 @@ ProcessLineResult NixRepl::processLine(std::string line) markdown += "**Synopsis:** `builtins." + (std::string) (*doc->name) + "` " - + dropEmptyInitThenConcatStringsSep(" ", args) + "\n\n"; + + concatStringsSep(" ", args) + "\n\n"; } markdown += stripIndentation(doc->doc); diff --git a/src/libflake/flake/lockfile.cc b/src/libflake/flake/lockfile.cc index f0e22a75a..80f14ff6f 100644 --- a/src/libflake/flake/lockfile.cc +++ b/src/libflake/flake/lockfile.cc @@ -9,6 +9,8 @@ #include #include +#include "strings.hh" + namespace nix::flake { static FlakeRef getFlakeRef( @@ -61,7 +63,7 @@ static std::shared_ptr doFind(const ref& root, const InputPath & pat std::vector cycle; std::transform(found, visited.cend(), std::back_inserter(cycle), printInputPath); cycle.push_back(printInputPath(path)); - throw Error("follow cycle detected: [%s]", dropEmptyInitThenConcatStringsSep(" -> ", cycle)); + throw Error("follow cycle detected: [%s]", concatStringsSep(" -> ", cycle)); } visited.push_back(path); @@ -367,7 +369,7 @@ void check(); std::string printInputPath(const InputPath & path) { - return dropEmptyInitThenConcatStringsSep("/", path); + return concatStringsSep("/", path); } } diff --git a/src/libmain/shared.cc b/src/libmain/shared.cc index 681c1039f..a224f8d92 100644 --- a/src/libmain/shared.cc +++ b/src/libmain/shared.cc @@ -23,6 +23,7 @@ #include #include "exit.hh" +#include "strings.hh" namespace nix { @@ -301,11 +302,11 @@ void printVersion(const std::string & programName) #endif cfg.push_back("signed-caches"); std::cout << "System type: " << settings.thisSystem << "\n"; - std::cout << "Additional system types: " << dropEmptyInitThenConcatStringsSep(", ", settings.extraPlatforms.get()) << "\n"; - std::cout << "Features: " << dropEmptyInitThenConcatStringsSep(", ", cfg) << "\n"; + std::cout << "Additional system types: " << concatStringsSep(", ", settings.extraPlatforms.get()) << "\n"; + std::cout << "Features: " << concatStringsSep(", ", cfg) << "\n"; std::cout << "System configuration file: " << settings.nixConfDir + "/nix.conf" << "\n"; std::cout << "User configuration files: " << - dropEmptyInitThenConcatStringsSep(":", settings.nixUserConfFiles) + concatStringsSep(":", settings.nixUserConfFiles) << "\n"; std::cout << "Store directory: " << settings.nixStore << "\n"; std::cout << "State directory: " << settings.nixStateDir << "\n"; diff --git a/src/libstore/build/entry-points.cc b/src/libstore/build/entry-points.cc index 8bf7ad35d..4c1373bfa 100644 --- a/src/libstore/build/entry-points.cc +++ b/src/libstore/build/entry-points.cc @@ -4,6 +4,7 @@ # include "derivation-goal.hh" #endif #include "local-store.hh" +#include "strings.hh" namespace nix { @@ -42,7 +43,7 @@ void Store::buildPaths(const std::vector & reqs, BuildMode buildMod throw std::move(*ex); } else if (!failed.empty()) { if (ex) logError(ex->info()); - throw Error(worker.failingExitStatus(), "build of %s failed", dropEmptyInitThenConcatStringsSep(", ", quoteStrings(failed))); + throw Error(worker.failingExitStatus(), "build of %s failed", concatStringsSep(", ", quoteStrings(failed))); } } diff --git a/src/libstore/misc.cc b/src/libstore/misc.cc index 179e5478c..fbfa15f51 100644 --- a/src/libstore/misc.cc +++ b/src/libstore/misc.cc @@ -10,6 +10,7 @@ #include "callback.hh" #include "closure.hh" #include "filetransfer.hh" +#include "strings.hh" namespace nix { diff --git a/src/libstore/unix/build/hook-instance.cc b/src/libstore/unix/build/hook-instance.cc index ba6c3a912..d73d86ff2 100644 --- a/src/libstore/unix/build/hook-instance.cc +++ b/src/libstore/unix/build/hook-instance.cc @@ -3,12 +3,13 @@ #include "hook-instance.hh" #include "file-system.hh" #include "child.hh" +#include "strings.hh" namespace nix { HookInstance::HookInstance() { - debug("starting build hook '%s'", dropEmptyInitThenConcatStringsSep(" ", settings.buildHook.get())); + debug("starting build hook '%s'", concatStringsSep(" ", settings.buildHook.get())); auto buildHookArgs = settings.buildHook.get(); From 39878c89798a95a423395ccfbc64d49f0fad92b9 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Fri, 12 Jul 2024 23:08:23 +0200 Subject: [PATCH 05/28] dropEmptyInitThenConcatStringsSep -> concatStringSep: preserve empty attr The empty attribute name should not be dropped from attribute paths. Rendering attribute paths with concatStringsSep is lossy and wrong, but this is just a first improvement while dealing with the dropEmptyInitThenConcatStringsSep problem. --- src/libcmd/installables.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/libcmd/installables.cc b/src/libcmd/installables.cc index 406e4bfd8..0fe956ec0 100644 --- a/src/libcmd/installables.cc +++ b/src/libcmd/installables.cc @@ -376,7 +376,8 @@ void completeFlakeRefWithFragment( auto attrPath2 = (*attr)->getAttrPath(attr2); /* Strip the attrpath prefix. */ attrPath2.erase(attrPath2.begin(), attrPath2.begin() + attrPathPrefix.size()); - completions.add(flakeRefS + "#" + prefixRoot + dropEmptyInitThenConcatStringsSep(".", evalState->symbols.resolve(attrPath2))); + // FIXME: handle names with dots + completions.add(flakeRefS + "#" + prefixRoot + concatStringsSep(".", evalState->symbols.resolve(attrPath2))); } } } From 3f37785afd002a36ecd5070b16c59902eba9cf88 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Sat, 13 Jul 2024 00:07:42 +0200 Subject: [PATCH 06/28] NIX_REMOTE_SYSTEMS: actually support multiple :-separated entries Bug not reported in 6 years, but here you go. Also it is safe to switch to normal concatStringsSep behavior because tokenizeString does not produce empty items. --- src/libstore/globals.cc | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/libstore/globals.cc b/src/libstore/globals.cc index 5f5b4f89b..4eabf6054 100644 --- a/src/libstore/globals.cc +++ b/src/libstore/globals.cc @@ -35,6 +35,8 @@ #include #endif +#include "strings.hh" + namespace nix { @@ -82,7 +84,7 @@ Settings::Settings() Strings ss; for (auto & p : tokenizeString(*s, ":")) ss.push_back("@" + p); - builders = dropEmptyInitThenConcatStringsSep(" ", ss); + builders = concatStringsSep("\n", ss); } #if defined(__linux__) && defined(SANDBOX_SHELL) From 75dde71ff97fcafeeefa08a5c9adba3414693ea2 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Sat, 13 Jul 2024 00:11:14 +0200 Subject: [PATCH 07/28] dropEmptyInitThenConcatStringsSep -> concatStringSep: sigs are non-empty The sigs field is produced by tokenizeStrings, which does not return empty strings. --- src/libstore/local-store.cc | 10 ++++++---- src/libstore/nar-info-disk-cache.cc | 4 +++- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 8764b88b7..82b70ff21 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -51,6 +51,8 @@ #include +#include "strings.hh" + namespace nix { @@ -656,7 +658,7 @@ void LocalStore::registerDrvOutput(const Realisation & info) combinedSignatures.insert(info.signatures.begin(), info.signatures.end()); state->stmts->UpdateRealisedOutput.use() - (dropEmptyInitThenConcatStringsSep(" ", combinedSignatures)) + (concatStringsSep(" ", combinedSignatures)) (info.id.strHash()) (info.id.outputName) .exec(); @@ -675,7 +677,7 @@ void LocalStore::registerDrvOutput(const Realisation & info) (info.id.strHash()) (info.id.outputName) (printStorePath(info.outPath)) - (dropEmptyInitThenConcatStringsSep(" ", info.signatures)) + (concatStringsSep(" ", info.signatures)) .exec(); } for (auto & [outputId, depPath] : info.dependentRealisations) { @@ -729,7 +731,7 @@ uint64_t LocalStore::addValidPath(State & state, (info.deriver ? printStorePath(*info.deriver) : "", (bool) info.deriver) (info.narSize, info.narSize != 0) (info.ultimate ? 1 : 0, info.ultimate) - (dropEmptyInitThenConcatStringsSep(" ", info.sigs), !info.sigs.empty()) + (concatStringsSep(" ", info.sigs), !info.sigs.empty()) (renderContentAddress(info.ca), (bool) info.ca) .exec(); uint64_t id = state.db.getLastInsertedRowId(); @@ -833,7 +835,7 @@ void LocalStore::updatePathInfo(State & state, const ValidPathInfo & info) (info.narSize, info.narSize != 0) (info.narHash.to_string(HashFormat::Base16, true)) (info.ultimate ? 1 : 0, info.ultimate) - (dropEmptyInitThenConcatStringsSep(" ", info.sigs), !info.sigs.empty()) + (concatStringsSep(" ", info.sigs), !info.sigs.empty()) (renderContentAddress(info.ca), (bool) info.ca) (printStorePath(info.path)) .exec(); diff --git a/src/libstore/nar-info-disk-cache.cc b/src/libstore/nar-info-disk-cache.cc index 42d172237..c75043237 100644 --- a/src/libstore/nar-info-disk-cache.cc +++ b/src/libstore/nar-info-disk-cache.cc @@ -7,6 +7,8 @@ #include #include +#include "strings.hh" + namespace nix { static const char * schema = R"sql( @@ -339,7 +341,7 @@ public: (info->narSize) (dropEmptyInitThenConcatStringsSep(" ", info->shortRefs())) (info->deriver ? std::string(info->deriver->to_string()) : "", (bool) info->deriver) - (dropEmptyInitThenConcatStringsSep(" ", info->sigs)) + (concatStringsSep(" ", info->sigs)) (renderContentAddress(info->ca)) (time(0)).exec(); From 608a425550b3c90de39d4a668a504debcc8c53de Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Sat, 13 Jul 2024 00:13:58 +0200 Subject: [PATCH 08/28] dropEmptyInitThenConcatStringsSep -> concatStringSep: diag --- src/libstore/misc.cc | 2 +- src/libstore/unix/build/local-derivation-goal.cc | 6 ++++-- src/nix/diff-closures.cc | 2 +- src/nix/profile.cc | 4 +++- src/nix/search.cc | 4 +++- 5 files changed, 12 insertions(+), 6 deletions(-) diff --git a/src/libstore/misc.cc b/src/libstore/misc.cc index fbfa15f51..bcc02206b 100644 --- a/src/libstore/misc.cc +++ b/src/libstore/misc.cc @@ -465,7 +465,7 @@ OutputPathMap resolveDerivedPath(Store & store, const DerivedPath::Built & bfd) if (!outputsLeft.empty()) throw Error("derivation '%s' does not have an outputs %s", store.printStorePath(drvPath), - dropEmptyInitThenConcatStringsSep(", ", quoteStrings(std::get(bfd.outputs.raw)))); + concatStringsSep(", ", quoteStrings(std::get(bfd.outputs.raw)))); return outputMap; } diff --git a/src/libstore/unix/build/local-derivation-goal.cc b/src/libstore/unix/build/local-derivation-goal.cc index 2ab4334e9..523fe07e7 100644 --- a/src/libstore/unix/build/local-derivation-goal.cc +++ b/src/libstore/unix/build/local-derivation-goal.cc @@ -64,6 +64,8 @@ #include #include +#include "strings.hh" + namespace nix { void handleDiffHook( @@ -840,7 +842,7 @@ void LocalDerivationGoal::startBuilder() /* Run the builder. */ printMsg(lvlChatty, "executing builder '%1%'", drv->builder); - printMsg(lvlChatty, "using builder args '%1%'", dropEmptyInitThenConcatStringsSep(" ", drv->args)); + printMsg(lvlChatty, "using builder args '%1%'", concatStringsSep(" ", drv->args)); for (auto & i : drv->env) printMsg(lvlVomit, "setting builder env variable '%1%'='%2%'", i.first, i.second); @@ -1063,7 +1065,7 @@ void LocalDerivationGoal::startBuilder() e.addTrace({}, "while waiting for the build environment for '%s' to initialize (%s, previous messages: %s)", worker.store.printStorePath(drvPath), statusToString(status), - dropEmptyInitThenConcatStringsSep("|", msgs)); + concatStringsSep("|", msgs)); throw; } }(); diff --git a/src/nix/diff-closures.cc b/src/nix/diff-closures.cc index 204213396..9e5a7e4c3 100644 --- a/src/nix/diff-closures.cc +++ b/src/nix/diff-closures.cc @@ -97,7 +97,7 @@ void printClosureDiff( items.push_back(fmt("%s → %s", showVersions(removed), showVersions(added))); if (showDelta) items.push_back(fmt("%s%+.1f KiB" ANSI_NORMAL, sizeDelta > 0 ? ANSI_RED : ANSI_GREEN, sizeDelta / 1024.0)); - logger->cout("%s%s: %s", indent, name, dropEmptyInitThenConcatStringsSep(", ", items)); + logger->cout("%s%s: %s", indent, name, concatStringsSep(", ", items)); } } } diff --git a/src/nix/profile.cc b/src/nix/profile.cc index c21eb3040..548a793d8 100644 --- a/src/nix/profile.cc +++ b/src/nix/profile.cc @@ -17,6 +17,8 @@ #include #include +#include "strings.hh" + using namespace nix; struct ProfileElementSource @@ -472,7 +474,7 @@ struct CmdProfileInstall : InstallablesCommand, MixDefaultProfile originalConflictingFilePath, newConflictingFilePath, originalEntryName, - dropEmptyInitThenConcatStringsSep(" ", newConflictingRefs), + concatStringsSep(" ", newConflictingRefs), conflictError.priority, conflictError.priority - 1, conflictError.priority + 1 diff --git a/src/nix/search.cc b/src/nix/search.cc index d709774ad..7c46f9fec 100644 --- a/src/nix/search.cc +++ b/src/nix/search.cc @@ -15,6 +15,8 @@ #include #include +#include "strings.hh" + using namespace nix; using json = nlohmann::json; @@ -96,7 +98,7 @@ struct CmdSearch : InstallableValueCommand, MixJSON auto attrPathS = state->symbols.resolve(attrPath); Activity act(*logger, lvlInfo, actUnknown, - fmt("evaluating '%s'", dropEmptyInitThenConcatStringsSep(".", attrPathS))); + fmt("evaluating '%s'", concatStringsSep(".", attrPathS))); try { auto recurse = [&]() { From d3e49ac881654529f8de3e2a3b39f5840d8ed669 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Sat, 13 Jul 2024 00:50:39 +0200 Subject: [PATCH 09/28] dropEmptyInitThenConcatStringsSep -> concatStringSep: shortRefs are not empty --- src/libstore/nar-info-disk-cache.cc | 2 +- src/libstore/nar-info.cc | 3 ++- src/libstore/path-info.hh | 3 +++ tests/unit/libstore/path-info.cc | 16 ++++++++++++++-- 4 files changed, 20 insertions(+), 4 deletions(-) diff --git a/src/libstore/nar-info-disk-cache.cc b/src/libstore/nar-info-disk-cache.cc index c75043237..288f618d5 100644 --- a/src/libstore/nar-info-disk-cache.cc +++ b/src/libstore/nar-info-disk-cache.cc @@ -339,7 +339,7 @@ public: (narInfo ? narInfo->fileSize : 0, narInfo != 0 && narInfo->fileSize) (info->narHash.to_string(HashFormat::Nix32, true)) (info->narSize) - (dropEmptyInitThenConcatStringsSep(" ", info->shortRefs())) + (concatStringsSep(" ", info->shortRefs())) (info->deriver ? std::string(info->deriver->to_string()) : "", (bool) info->deriver) (concatStringsSep(" ", info->sigs)) (renderContentAddress(info->ca)) diff --git a/src/libstore/nar-info.cc b/src/libstore/nar-info.cc index 577466f55..2442a7b09 100644 --- a/src/libstore/nar-info.cc +++ b/src/libstore/nar-info.cc @@ -1,6 +1,7 @@ #include "globals.hh" #include "nar-info.hh" #include "store-api.hh" +#include "strings.hh" namespace nix { @@ -111,7 +112,7 @@ std::string NarInfo::to_string(const Store & store) const res += "NarHash: " + narHash.to_string(HashFormat::Nix32, true) + "\n"; res += "NarSize: " + std::to_string(narSize) + "\n"; - res += "References: " + dropEmptyInitThenConcatStringsSep(" ", shortRefs()) + "\n"; + res += "References: " + concatStringsSep(" ", shortRefs()) + "\n"; if (deriver) res += "Deriver: " + std::string(deriver->to_string()) + "\n"; diff --git a/src/libstore/path-info.hh b/src/libstore/path-info.hh index caefa7975..71f1476a6 100644 --- a/src/libstore/path-info.hh +++ b/src/libstore/path-info.hh @@ -171,6 +171,9 @@ struct ValidPathInfo : UnkeyedValidPathInfo { */ bool checkSignature(const Store & store, const PublicKeys & publicKeys, const std::string & sig) const; + /** + * References as store path basenames, including a self reference if it has one. + */ Strings shortRefs() const; ValidPathInfo(const ValidPathInfo & other) = default; diff --git a/tests/unit/libstore/path-info.cc b/tests/unit/libstore/path-info.cc index 7637cb366..9e9c6303d 100644 --- a/tests/unit/libstore/path-info.cc +++ b/tests/unit/libstore/path-info.cc @@ -26,9 +26,9 @@ static UnkeyedValidPathInfo makeEmpty() }; } -static UnkeyedValidPathInfo makeFull(const Store & store, bool includeImpureInfo) +static ValidPathInfo makeFullKeyed(const Store & store, bool includeImpureInfo) { - UnkeyedValidPathInfo info = ValidPathInfo { + ValidPathInfo info = ValidPathInfo { store, "foo", FixedOutputInfo { @@ -57,6 +57,9 @@ static UnkeyedValidPathInfo makeFull(const Store & store, bool includeImpureInfo } return info; } +static UnkeyedValidPathInfo makeFull(const Store & store, bool includeImpureInfo) { + return makeFullKeyed(store, includeImpureInfo); +} #define JSON_TEST(STEM, OBJ, PURE) \ TEST_F(PathInfoTest, PathInfo_ ## STEM ## _from_json) { \ @@ -86,4 +89,13 @@ JSON_TEST(empty_impure, makeEmpty(), true) JSON_TEST(pure, makeFull(*store, false), false) JSON_TEST(impure, makeFull(*store, true), true) +TEST_F(PathInfoTest, PathInfo_full_shortRefs) { + ValidPathInfo it = makeFullKeyed(*store, true); + // it.references = unkeyed.references; + auto refs = it.shortRefs(); + ASSERT_EQ(refs.size(), 2); + ASSERT_EQ(*refs.begin(), "g1w7hy3qg1w7hy3qg1w7hy3qg1w7hy3q-bar"); + ASSERT_EQ(*++refs.begin(), "n5wkd9frr45pa74if5gpz9j7mifg27fh-foo"); } + +} // namespace nix From 49d100ba8b5d65c6f2df909e53ec92ba279cfc4d Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Sat, 13 Jul 2024 01:00:06 +0200 Subject: [PATCH 10/28] dropEmptyInitThenConcatStringsSep -> concatStringSep: output name empty not feasible I don't think it's completely impossible, but I can't construct one easily as derivationStrict seems to (re)tokenize the outputs attribute, dropping the empty output. It's not a scenario we have to account for here. --- src/libstore/build/derivation-goal.cc | 2 +- src/libstore/outputs-spec.cc | 3 ++- src/libstore/path-with-outputs.cc | 6 ++++-- 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc index c0a784349..99d9cceda 100644 --- a/src/libstore/build/derivation-goal.cc +++ b/src/libstore/build/derivation-goal.cc @@ -1505,7 +1505,7 @@ std::pair DerivationGoal::checkPathValidity() if (!wantedOutputsLeft.empty()) throw Error("derivation '%s' does not have wanted outputs %s", worker.store.printStorePath(drvPath), - dropEmptyInitThenConcatStringsSep(", ", quoteStrings(wantedOutputsLeft))); + concatStringsSep(", ", quoteStrings(wantedOutputsLeft))); bool allValid = true; for (auto & [_, status] : initialOutputs) { diff --git a/src/libstore/outputs-spec.cc b/src/libstore/outputs-spec.cc index 4ed8f95ae..86788a87e 100644 --- a/src/libstore/outputs-spec.cc +++ b/src/libstore/outputs-spec.cc @@ -5,6 +5,7 @@ #include "regex-combinators.hh" #include "outputs-spec.hh" #include "path-regex.hh" +#include "strings-inline.hh" namespace nix { @@ -83,7 +84,7 @@ std::string OutputsSpec::to_string() const return "*"; }, [&](const OutputsSpec::Names & outputNames) -> std::string { - return dropEmptyInitThenConcatStringsSep(",", outputNames); + return concatStringsSep(",", outputNames); }, }, raw); } diff --git a/src/libstore/path-with-outputs.cc b/src/libstore/path-with-outputs.cc index 5fa38d5d9..161d023d1 100644 --- a/src/libstore/path-with-outputs.cc +++ b/src/libstore/path-with-outputs.cc @@ -1,7 +1,9 @@ +#include + #include "path-with-outputs.hh" #include "store-api.hh" +#include "strings.hh" -#include namespace nix { @@ -9,7 +11,7 @@ std::string StorePathWithOutputs::to_string(const StoreDirConfig & store) const { return outputs.empty() ? store.printStorePath(path) - : store.printStorePath(path) + "!" + dropEmptyInitThenConcatStringsSep(",", outputs); + : store.printStorePath(path) + "!" + concatStringsSep(",", outputs); } From f1966e22d9e959b6885bd9270d2789b484690aa8 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Sat, 13 Jul 2024 01:02:30 +0200 Subject: [PATCH 11/28] dropEmptyInitThenConcatStringsSep -> concatStringSep: store paths are not empty --- src/libstore/build/derivation-goal.cc | 4 +++- src/libstore/path-info.cc | 3 ++- src/libstore/store-api.cc | 4 +++- src/nix/profile.cc | 2 +- 4 files changed, 9 insertions(+), 4 deletions(-) diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc index 99d9cceda..f795b05a1 100644 --- a/src/libstore/build/derivation-goal.cc +++ b/src/libstore/build/derivation-goal.cc @@ -32,6 +32,8 @@ #include +#include "strings.hh" + namespace nix { DerivationGoal::DerivationGoal(const StorePath & drvPath, @@ -895,7 +897,7 @@ void runPostBuildHook( std::map hookEnvironment = getEnv(); hookEnvironment.emplace("DRV_PATH", store.printStorePath(drvPath)); - hookEnvironment.emplace("OUT_PATHS", chomp(dropEmptyInitThenConcatStringsSep(" ", store.printStorePathSet(outputPaths)))); + hookEnvironment.emplace("OUT_PATHS", chomp(concatStringsSep(" ", store.printStorePathSet(outputPaths)))); hookEnvironment.emplace("NIX_CONFIG", globalConfig.toKeyValue()); struct LogSink : Sink { diff --git a/src/libstore/path-info.cc b/src/libstore/path-info.cc index a13bb8bef..6e87e60f4 100644 --- a/src/libstore/path-info.cc +++ b/src/libstore/path-info.cc @@ -4,6 +4,7 @@ #include "store-api.hh" #include "json-utils.hh" #include "comparator.hh" +#include "strings.hh" namespace nix { @@ -30,7 +31,7 @@ std::string ValidPathInfo::fingerprint(const Store & store) const "1;" + store.printStorePath(path) + ";" + narHash.to_string(HashFormat::Nix32, true) + ";" + std::to_string(narSize) + ";" - + dropEmptyInitThenConcatStringsSep(",", store.printStorePathSet(references)); + + concatStringsSep(",", store.printStorePathSet(references)); } diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc index 6904996b5..2c4dee518 100644 --- a/src/libstore/store-api.cc +++ b/src/libstore/store-api.cc @@ -22,6 +22,8 @@ #include #include +#include "strings.hh" + using json = nlohmann::json; namespace nix { @@ -1208,7 +1210,7 @@ std::string StoreDirConfig::showPaths(const StorePathSet & paths) std::string showPaths(const PathSet & paths) { - return dropEmptyInitThenConcatStringsSep(", ", quoteStrings(paths)); + return concatStringsSep(", ", quoteStrings(paths)); } diff --git a/src/nix/profile.cc b/src/nix/profile.cc index 548a793d8..1096f4386 100644 --- a/src/nix/profile.cc +++ b/src/nix/profile.cc @@ -815,7 +815,7 @@ struct CmdProfileList : virtual EvalCommand, virtual StoreCommand, MixDefaultPro logger->cout("Original flake URL: %s", element.source->originalRef.to_string()); logger->cout("Locked flake URL: %s", element.source->lockedRef.to_string()); } - logger->cout("Store paths: %s", dropEmptyInitThenConcatStringsSep(" ", store->printStorePathSet(element.storePaths))); + logger->cout("Store paths: %s", concatStringsSep(" ", store->printStorePathSet(element.storePaths))); } } } From e64643bf6372af1ee7f56ac602f25564201df7f0 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Sat, 13 Jul 2024 01:12:28 +0200 Subject: [PATCH 12/28] dropEmptyInitThenConcatStringsSep -> concatStringSep: feature should not be empty (System) features are unlikely to be empty strings, but when they come in through structuredAttrs, they probably can. I don't think this means we should drop them, but most likely they will be dropped after this because next time they'll be parsed with tokenizeString. TODO: We should forbid empty features. --- src/libstore/unix/build/local-derivation-goal.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libstore/unix/build/local-derivation-goal.cc b/src/libstore/unix/build/local-derivation-goal.cc index 523fe07e7..c3a65e34b 100644 --- a/src/libstore/unix/build/local-derivation-goal.cc +++ b/src/libstore/unix/build/local-derivation-goal.cc @@ -498,10 +498,10 @@ void LocalDerivationGoal::startBuilder() if (!parsedDrv->canBuildLocally(worker.store)) throw Error("a '%s' with features {%s} is required to build '%s', but I am a '%s' with features {%s}", drv->platform, - dropEmptyInitThenConcatStringsSep(", ", parsedDrv->getRequiredSystemFeatures()), + concatStringsSep(", ", parsedDrv->getRequiredSystemFeatures()), worker.store.printStorePath(drvPath), settings.thisSystem, - dropEmptyInitThenConcatStringsSep(", ", worker.store.systemFeatures)); + concatStringsSep(", ", worker.store.systemFeatures)); /* Create a temporary directory where the build will take place. */ From 3b77f134515866f28cc7b7ddb06274fccc5766f9 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Sat, 13 Jul 2024 01:16:33 +0200 Subject: [PATCH 13/28] dropEmptyInitThenConcatStringsSep -> concatStringSep: experimental features do not render as empty strings --- src/libutil/config.cc | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/libutil/config.cc b/src/libutil/config.cc index 81ec9a4c3..25bfe462f 100644 --- a/src/libutil/config.cc +++ b/src/libutil/config.cc @@ -9,6 +9,8 @@ #include +#include "strings.hh" + namespace nix { Config::Config(StringMap initials) @@ -362,7 +364,7 @@ template<> std::string BaseSetting>::to_string() c StringSet stringifiedXpFeatures; for (const auto & feature : value) stringifiedXpFeatures.insert(std::string(showExperimentalFeature(feature))); - return dropEmptyInitThenConcatStringsSep(" ", stringifiedXpFeatures); + return concatStringsSep(" ", stringifiedXpFeatures); } template<> StringMap BaseSetting::parse(const std::string & str) const From 837c3612d40c1e33be94080b9b2063c3ca0795ed Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Sat, 13 Jul 2024 01:20:20 +0200 Subject: [PATCH 14/28] dropEmptyInitThenConcatStringsSep -> concatStringSep: escaped shell args are never empty --- src/nix/develop.cc | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/nix/develop.cc b/src/nix/develop.cc index 807c75d4a..7cc0965a9 100644 --- a/src/nix/develop.cc +++ b/src/nix/develop.cc @@ -19,6 +19,8 @@ #include #include +#include "strings.hh" + using namespace nix; struct DevelopSettings : Config @@ -608,7 +610,7 @@ struct CmdDevelop : Common, MixEnvironment std::vector args; for (auto s : command) args.push_back(shellEscape(s)); - script += fmt("exec %s\n", dropEmptyInitThenConcatStringsSep(" ", args)); + script += fmt("exec %s\n", concatStringsSep(" ", args)); } else { From 4b34feb4c2fded8b4ca0968f33f0e34514520b1a Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Sat, 13 Jul 2024 01:22:51 +0200 Subject: [PATCH 15/28] dropEmptyInitThenConcatStringsSep -> concatStringSep: system string should not be empty --- src/nix/flake.cc | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/nix/flake.cc b/src/nix/flake.cc index 3ee2b1838..1ed071ec8 100644 --- a/src/nix/flake.cc +++ b/src/nix/flake.cc @@ -21,6 +21,8 @@ #include #include +#include "strings-inline.hh" + using namespace nix; using namespace nix::flake; using json = nlohmann::json; @@ -802,10 +804,11 @@ struct CmdFlakeCheck : FlakeCommand throw Error("some errors were encountered during the evaluation"); if (!omittedSystems.empty()) { + // TODO: empty system is not visible; render all as nix strings? warn( "The check omitted these incompatible systems: %s\n" "Use '--all-systems' to check all.", - dropEmptyInitThenConcatStringsSep(", ", omittedSystems) + concatStringsSep(", ", omittedSystems) ); }; }; From 0480bfe50bf1deb3b5c93761f7fbba1bc59ef059 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Sat, 13 Jul 2024 01:23:44 +0200 Subject: [PATCH 16/28] dropEmptyInitThenConcatStringsSep -> concatStringSep: do not drop attributes with empty names Empty attributes are probably not well supported, but the least we could do is leave a hint. Attribute path rendering and parsing should be done according to Nix expression syntax in my opinion. --- src/nix/flake.cc | 8 ++++---- src/nix/search.cc | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/nix/flake.cc b/src/nix/flake.cc index 1ed071ec8..3f9f8f99b 100644 --- a/src/nix/flake.cc +++ b/src/nix/flake.cc @@ -1214,7 +1214,7 @@ struct CmdFlakeShow : FlakeCommand, MixJSON auto attrPathS = state->symbols.resolve(attrPath); Activity act(*logger, lvlInfo, actUnknown, - fmt("evaluating '%s'", dropEmptyInitThenConcatStringsSep(".", attrPathS))); + fmt("evaluating '%s'", concatStringsSep(".", attrPathS))); try { auto recurse = [&]() @@ -1294,7 +1294,7 @@ struct CmdFlakeShow : FlakeCommand, MixJSON if (!json) logger->cout(fmt("%s " ANSI_WARNING "omitted" ANSI_NORMAL " (use '--all-systems' to show)", headerPrefix)); else { - logger->warn(fmt("%s omitted (use '--all-systems' to show)", dropEmptyInitThenConcatStringsSep(".", attrPathS))); + logger->warn(fmt("%s omitted (use '--all-systems' to show)", concatStringsSep(".", attrPathS))); } } else { if (visitor.isDerivation()) @@ -1318,13 +1318,13 @@ struct CmdFlakeShow : FlakeCommand, MixJSON if (!json) logger->cout(fmt("%s " ANSI_WARNING "omitted" ANSI_NORMAL " (use '--legacy' to show)", headerPrefix)); else { - logger->warn(fmt("%s omitted (use '--legacy' to show)", dropEmptyInitThenConcatStringsSep(".", attrPathS))); + logger->warn(fmt("%s omitted (use '--legacy' to show)", concatStringsSep(".", attrPathS))); } } else if (!showAllSystems && std::string(attrPathS[1]) != localSystem) { if (!json) logger->cout(fmt("%s " ANSI_WARNING "omitted" ANSI_NORMAL " (use '--all-systems' to show)", headerPrefix)); else { - logger->warn(fmt("%s omitted (use '--all-systems' to show)", dropEmptyInitThenConcatStringsSep(".", attrPathS))); + logger->warn(fmt("%s omitted (use '--all-systems' to show)", concatStringsSep(".", attrPathS))); } } else { if (visitor.isDerivation()) diff --git a/src/nix/search.cc b/src/nix/search.cc index 7c46f9fec..7f8504d3f 100644 --- a/src/nix/search.cc +++ b/src/nix/search.cc @@ -117,7 +117,7 @@ struct CmdSearch : InstallableValueCommand, MixJSON auto aDescription = aMeta ? aMeta->maybeGetAttr(state->sDescription) : nullptr; auto description = aDescription ? aDescription->getString() : ""; std::replace(description.begin(), description.end(), '\n', ' '); - auto attrPath2 = dropEmptyInitThenConcatStringsSep(".", attrPathS); + auto attrPath2 = concatStringsSep(".", attrPathS); std::vector attrPathMatches; std::vector descriptionMatches; From 062672b022a35fb0251e37cc0f428778d82fb934 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Sat, 13 Jul 2024 01:27:59 +0200 Subject: [PATCH 17/28] dropEmptyInitThenConcatStringsSep -> concatStringSep: CLI commands are not empty --- src/nix/main.cc | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/nix/main.cc b/src/nix/main.cc index aa4ced623..21d364a18 100644 --- a/src/nix/main.cc +++ b/src/nix/main.cc @@ -41,6 +41,8 @@ extern std::string chrootHelperName; void chrootHelper(int argc, char * * argv); #endif +#include "strings.hh" + namespace nix { enum struct AliasStatus { @@ -185,7 +187,7 @@ struct NixArgs : virtual MultiCommand, virtual MixCommonArgs, virtual RootArgs auto & info = i->second; if (info.status == AliasStatus::Deprecated) { warn("'%s' is a deprecated alias for '%s'", - arg, dropEmptyInitThenConcatStringsSep(" ", info.replacement)); + arg, concatStringsSep(" ", info.replacement)); } pos = args.erase(pos); for (auto j = info.replacement.rbegin(); j != info.replacement.rend(); ++j) From d9043021dfc4b7238f538c8064a3587a6a8d473d Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Sat, 13 Jul 2024 01:29:15 +0200 Subject: [PATCH 18/28] dropEmptyInitThenConcatStringsSep -> concatStringSep: break nix help "" "" "" build Garbage in, error out. Experimental CLI. Zero derivations given. --- src/nix/main.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/nix/main.cc b/src/nix/main.cc index 21d364a18..00ad6fe2c 100644 --- a/src/nix/main.cc +++ b/src/nix/main.cc @@ -240,7 +240,7 @@ struct NixArgs : virtual MultiCommand, virtual MixCommonArgs, virtual RootArgs lowdown. */ static void showHelp(std::vector subcommand, NixArgs & toplevel) { - auto mdName = subcommand.empty() ? "nix" : fmt("nix3-%s", dropEmptyInitThenConcatStringsSep("-", subcommand)); + auto mdName = subcommand.empty() ? "nix" : fmt("nix3-%s", concatStringsSep("-", subcommand)); evalSettings.restrictEval = false; evalSettings.pureEval = false; @@ -275,7 +275,7 @@ static void showHelp(std::vector subcommand, NixArgs & toplevel) auto attr = vRes->attrs()->get(state.symbols.create(mdName + ".md")); if (!attr) - throw UsageError("Nix has no subcommand '%s'", dropEmptyInitThenConcatStringsSep("", subcommand)); + throw UsageError("Nix has no subcommand '%s'", concatStringsSep("", subcommand)); auto markdown = state.forceString(*attr->value, noPos, "while evaluating the lowdown help text"); From cf3c5cd189b5818e837f308507db3e92a81d66d0 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Sat, 13 Jul 2024 01:34:41 +0200 Subject: [PATCH 19/28] dropEmptyInitThenConcatStringsSep -> concatStringSep: showVersions version is not empty --- src/nix/diff-closures.cc | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/nix/diff-closures.cc b/src/nix/diff-closures.cc index 9e5a7e4c3..46c94b211 100644 --- a/src/nix/diff-closures.cc +++ b/src/nix/diff-closures.cc @@ -6,6 +6,8 @@ #include +#include "strings.hh" + namespace nix { struct Info @@ -49,7 +51,7 @@ std::string showVersions(const std::set & versions) std::set versions2; for (auto & version : versions) versions2.insert(version.empty() ? "ε" : version); - return dropEmptyInitThenConcatStringsSep(", ", versions2); + return concatStringsSep(", ", versions2); } void printClosureDiff( From 0fe3525223976e1c0ad047cc706da66208214fb5 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Sat, 13 Jul 2024 01:39:06 +0200 Subject: [PATCH 20/28] illegal configuration line -> syntax error in configuration line The law has nothing to do with this, although I do feel like a badass when I mess with the config. I'm a conf artist. --- src/libutil/config.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libutil/config.cc b/src/libutil/config.cc index 25bfe462f..b3e2d1a48 100644 --- a/src/libutil/config.cc +++ b/src/libutil/config.cc @@ -116,7 +116,7 @@ static void parseConfigFiles(const std::string & contents, const std::string & p if (tokens.empty()) continue; if (tokens.size() < 2) - throw UsageError("illegal configuration line '%1%' in '%2%'", line, path); + throw UsageError("syntax error in configuration line '%1%' in '%2%'", line, path); auto include = false; auto ignoreMissing = false; @@ -129,7 +129,7 @@ static void parseConfigFiles(const std::string & contents, const std::string & p if (include) { if (tokens.size() != 2) - throw UsageError("illegal configuration line '%1%' in '%2%'", line, path); + throw UsageError("syntax error in configuration line '%1%' in '%2%'", line, path); auto p = absPath(tokens[1], dirOf(path)); if (pathExists(p)) { try { @@ -145,7 +145,7 @@ static void parseConfigFiles(const std::string & contents, const std::string & p } if (tokens[1] != "=") - throw UsageError("illegal configuration line '%1%' in '%2%'", line, path); + throw UsageError("syntax error in configuration line '%1%' in '%2%'", line, path); std::string name = std::move(tokens[0]); From 4029426ca8e8d48a74222d2058ff152d7fd36027 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Sat, 13 Jul 2024 01:41:49 +0200 Subject: [PATCH 21/28] dropEmptyInitThenConcatStringsSep -> concatStringSep: tokens from tokenizeString are not empty --- src/libutil/config.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libutil/config.cc b/src/libutil/config.cc index b3e2d1a48..6d929b7f7 100644 --- a/src/libutil/config.cc +++ b/src/libutil/config.cc @@ -154,7 +154,7 @@ static void parseConfigFiles(const std::string & contents, const std::string & p parsedContents.push_back({ std::move(name), - dropEmptyInitThenConcatStringsSep(" ", Strings(i, tokens.end())), + concatStringsSep(" ", Strings(i, tokens.end())), }); }; } From 9ca42d5da20cbbb76cd2f35f2a442d70505bf862 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Sat, 13 Jul 2024 01:43:11 +0200 Subject: [PATCH 22/28] dropEmptyInitThenConcatStringsSep -> concatStringSep: setting value was already harmed Considering that `value` was probably parsed with tokenizeString prior, it's unlikely to contain empty strings, and we have no reason to remove them either. --- src/libutil/config.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libutil/config.cc b/src/libutil/config.cc index 6d929b7f7..726e5091e 100644 --- a/src/libutil/config.cc +++ b/src/libutil/config.cc @@ -320,7 +320,7 @@ template<> void BaseSetting::appendOrSet(Strings newValue, bool append) template<> std::string BaseSetting::to_string() const { - return dropEmptyInitThenConcatStringsSep(" ", value); + return concatStringsSep(" ", value); } template<> StringSet BaseSetting::parse(const std::string & str) const @@ -336,7 +336,7 @@ template<> void BaseSetting::appendOrSet(StringSet newValue, bool app template<> std::string BaseSetting::to_string() const { - return dropEmptyInitThenConcatStringsSep(" ", value); + return concatStringsSep(" ", value); } template<> std::set BaseSetting>::parse(const std::string & str) const From 76b2d5ef3ddd876fbe825f5804f1331dfcf2ecfe Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Sat, 13 Jul 2024 01:49:34 +0200 Subject: [PATCH 23/28] dropEmptyInitThenConcatStringsSep -> concatStringSep: PATH handling It's still wrong, but one step closer to correct. Not that anyone should use "" or "." in their PATH, but that is not for us to intervene. --- src/nix/env.cc | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/nix/env.cc b/src/nix/env.cc index 7cc019c1d..bc4e1b5e5 100644 --- a/src/nix/env.cc +++ b/src/nix/env.cc @@ -3,6 +3,7 @@ #include "command.hh" #include "run.hh" +#include "strings.hh" using namespace nix; @@ -92,9 +93,10 @@ struct CmdShell : InstallablesCommand, MixEnvironment } } + // TODO: split losslessly; empty means . auto unixPath = tokenizeString(getEnv("PATH").value_or(""), ":"); unixPath.insert(unixPath.begin(), pathAdditions.begin(), pathAdditions.end()); - auto unixPathString = dropEmptyInitThenConcatStringsSep(":", unixPath); + auto unixPathString = concatStringsSep(":", unixPath); setEnv("PATH", unixPathString.c_str()); Strings args; From 6b2c277c363f59b0e200882644deb5d0e658ec20 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Sat, 13 Jul 2024 01:51:50 +0200 Subject: [PATCH 24/28] dropEmptyInitThenConcatStringsSep -> concatStringSep: sigs are not empty ... but if they are, I'd like to see at least a hint of it so that I'd know to fix it. --- src/nix/path-info.cc | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/nix/path-info.cc b/src/nix/path-info.cc index 2383fbe08..e7cfb6e7a 100644 --- a/src/nix/path-info.cc +++ b/src/nix/path-info.cc @@ -9,6 +9,8 @@ #include +#include "strings.hh" + using namespace nix; using nlohmann::json; @@ -185,7 +187,7 @@ struct CmdPathInfo : StorePathsCommand, MixJSON if (info->ultimate) ss.push_back("ultimate"); if (info->ca) ss.push_back("ca:" + renderContentAddress(*info->ca)); for (auto & sig : info->sigs) ss.push_back(sig); - std::cout << dropEmptyInitThenConcatStringsSep(" ", ss); + std::cout << concatStringsSep(" ", ss); } std::cout << std::endl; From 1c97718146264de4c4bc6e1694774da5d5b31188 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Sat, 13 Jul 2024 02:16:13 +0200 Subject: [PATCH 25/28] dropEmptyInitThenConcatStringsSep: Allow it to drop items again It's usually harmless, if it occurs at all. --- src/libutil/util.hh | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/libutil/util.hh b/src/libutil/util.hh index c545afd9e..b653bf115 100644 --- a/src/libutil/util.hh +++ b/src/libutil/util.hh @@ -41,11 +41,13 @@ std::string dropEmptyInitThenConcatStringsSep(const std::string_view sep, const { size_t size = 0; - for (auto & i : ss) { - // Make sure we don't rely on the empty item ignoring behavior - assert(!i.empty()); - break; - } + // TODO? remove to make sure we don't rely on the empty item ignoring behavior, + // or just get rid of this function by understanding the remaining calls. + // for (auto & i : ss) { + // // Make sure we don't rely on the empty item ignoring behavior + // assert(!i.empty()); + // break; + // } // need a cast to string_view since this is also called with Symbols for (const auto & s : ss) size += sep.size() + std::string_view(s).size(); From d40fdb5711e8f7c7dff88f611d5bc26d37ba79e9 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Sun, 14 Jul 2024 11:50:14 +0200 Subject: [PATCH 26/28] dropEmptyInitThenConcatStringsSep: Update doc and deprecate --- src/libutil/util.hh | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/libutil/util.hh b/src/libutil/util.hh index b653bf115..971ecf63b 100644 --- a/src/libutil/util.hh +++ b/src/libutil/util.hh @@ -33,10 +33,14 @@ template C tokenizeString(std::string_view s, std::string_view separato /** - * Concatenate the given strings with a separator between the - * elements. + * Ignore any empty strings at the start of the list, and then concatenate the + * given strings with a separator between the elements. + * + * @deprecated This function exists for historical reasons. You probably just + * want to use `concatStringsSep`. */ template +[[deprecated("Consider removing the empty string dropping behavior. If acceptable, use concatStringsSep instead.")]] std::string dropEmptyInitThenConcatStringsSep(const std::string_view sep, const C & ss) { size_t size = 0; From 97e01107ecf4d5cea97bd20423409ba2a112612c Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Sun, 14 Jul 2024 11:51:53 +0200 Subject: [PATCH 27/28] dropEmptyInitThenConcatStringsSep -> concatStringSep: empty separator When the separator is empty, no difference is observable. Note that concatStringsSep has centralized definitions. This adds the required definitions. Alternatively, `strings-inline.hh` could be included at call sites. --- src/libutil/strings.cc | 7 +++++++ src/libutil/util.hh | 4 +++- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/src/libutil/strings.cc b/src/libutil/strings.cc index 15937d415..7ec618bf4 100644 --- a/src/libutil/strings.cc +++ b/src/libutil/strings.cc @@ -9,4 +9,11 @@ template std::string concatStringsSep(std::string_view, const Strings &); template std::string concatStringsSep(std::string_view, const StringSet &); template std::string concatStringsSep(std::string_view, const std::vector &); +typedef std::string_view strings_2[2]; +template std::string concatStringsSep(std::string_view, const strings_2 &); +typedef std::string_view strings_3[3]; +template std::string concatStringsSep(std::string_view, const strings_3 &); +typedef std::string_view strings_4[4]; +template std::string concatStringsSep(std::string_view, const strings_4 &); + } // namespace nix diff --git a/src/libutil/util.hh b/src/libutil/util.hh index 971ecf63b..877d15279 100644 --- a/src/libutil/util.hh +++ b/src/libutil/util.hh @@ -11,6 +11,8 @@ #include #include +#include "strings.hh" + namespace nix { void initLibUtil(); @@ -69,7 +71,7 @@ auto concatStrings(Parts && ... parts) -> std::enable_if_t<(... && std::is_convertible_v), std::string> { std::string_view views[sizeof...(parts)] = { parts... }; - return dropEmptyInitThenConcatStringsSep({}, views); + return concatStringsSep({}, views); } From 7e604f716cd3a4bcd47407bda27874262212dcbc Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Sun, 14 Jul 2024 12:19:55 +0200 Subject: [PATCH 28/28] concatStrings: Give compiler access to definition for inlining ... at call sites that are may be in the hot path. I do not know how clever the compiler gets at these sites. My primary concern is to not regress performance and I am confident that this achieves it the easy way. --- src/libexpr/eval.cc | 2 ++ src/libexpr/nixexpr.cc | 2 ++ src/libstore/derivations.cc | 2 ++ src/libutil/canon-path.cc | 1 + src/libutil/file-system.cc | 2 ++ 5 files changed, 9 insertions(+) diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 2ede55de7..a4cf2e8c8 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -49,6 +49,8 @@ #endif +#include "strings-inline.hh" + using json = nlohmann::json; namespace nix { diff --git a/src/libexpr/nixexpr.cc b/src/libexpr/nixexpr.cc index 816389165..c1ffe3435 100644 --- a/src/libexpr/nixexpr.cc +++ b/src/libexpr/nixexpr.cc @@ -7,6 +7,8 @@ #include #include +#include "strings-inline.hh" + namespace nix { unsigned long Expr::nrExprs = 0; diff --git a/src/libstore/derivations.cc b/src/libstore/derivations.cc index 6dfcc408c..8f9c71851 100644 --- a/src/libstore/derivations.cc +++ b/src/libstore/derivations.cc @@ -10,6 +10,8 @@ #include #include +#include "strings-inline.hh" + namespace nix { std::optional DerivationOutput::path(const StoreDirConfig & store, std::string_view drvName, OutputNameView outputName) const diff --git a/src/libutil/canon-path.cc b/src/libutil/canon-path.cc index 27f048697..03db6378a 100644 --- a/src/libutil/canon-path.cc +++ b/src/libutil/canon-path.cc @@ -1,6 +1,7 @@ #include "canon-path.hh" #include "util.hh" #include "file-path-impl.hh" +#include "strings-inline.hh" namespace nix { diff --git a/src/libutil/file-system.cc b/src/libutil/file-system.cc index f75851bbd..9042e3a5e 100644 --- a/src/libutil/file-system.cc +++ b/src/libutil/file-system.cc @@ -23,6 +23,8 @@ # include #endif +#include "strings-inline.hh" + namespace fs = std::filesystem; namespace nix {