From 0b790b4849c9298f99846b707012aa3507a64ea1 Mon Sep 17 00:00:00 2001 From: Bryan Honof Date: Thu, 12 Sep 2024 03:04:52 +0200 Subject: [PATCH 1/3] feat: add flag `set-env-var` to `MixEnvironment` --- src/libcmd/command.cc | 88 ++++++++++++++++++++-------- src/libcmd/command.hh | 13 ++-- src/libutil/environment-variables.hh | 2 + tests/functional/flakes/develop.sh | 6 +- 4 files changed, 76 insertions(+), 33 deletions(-) diff --git a/src/libcmd/command.cc b/src/libcmd/command.cc index 6d8bfc19b1e..295d290c5d9 100644 --- a/src/libcmd/command.cc +++ b/src/libcmd/command.cc @@ -1,3 +1,4 @@ +#include #include #include "command.hh" @@ -9,8 +10,7 @@ #include "profiles.hh" #include "repl.hh" #include "strings.hh" - -extern char * * environ __attribute__((weak)); +#include "environment-variables.hh" namespace nix { @@ -285,48 +285,88 @@ MixDefaultProfile::MixDefaultProfile() MixEnvironment::MixEnvironment() : ignoreEnvironment(false) { addFlag({ - .longName = "ignore-environment", + .longName = "ignore-env", + .aliases = {"ignore-environment"}, .shortName = 'i', - .description = "Clear the entire environment (except those specified with `--keep`).", + .description = "Clear the entire environment, except for those specified with `--keep-env-var`.", + .category = environmentVariablesCategory, .handler = {&ignoreEnvironment, true}, }); addFlag({ - .longName = "keep", + .longName = "keep-env-var", + .aliases = {"keep"}, .shortName = 'k', - .description = "Keep the environment variable *name*.", + .description = "Keep the environment variable *name*, when using `--ignore-env`.", + .category = environmentVariablesCategory, .labels = {"name"}, - .handler = {[&](std::string s) { keep.insert(s); }}, + .handler = {[&](std::string s) { keepVars.insert(s); }}, }); addFlag({ - .longName = "unset", + .longName = "unset-env-var", + .aliases = {"unset"}, .shortName = 'u', .description = "Unset the environment variable *name*.", + .category = environmentVariablesCategory, .labels = {"name"}, - .handler = {[&](std::string s) { unset.insert(s); }}, + .handler = {[&](std::string name) { + if (setVars.contains(name)) + throw UsageError("Cannot unset environment variable '%s' that is set with '%s'", name, "--set-env-var"); + + unsetVars.insert(name); + }}, + }); + + addFlag({ + .longName = "set-env-var", + .shortName = 's', + .description = "Add/override an environment variable *name* with *value*.\n\n" + "> **Notes**\n" + ">\n" + "> Duplicate definitions will be overwritten, last one wins.\n\n" + "> Cancles out with `--unset-env-var`.\n\n", + .category = environmentVariablesCategory, + .labels = {"name", "value"}, + .handler = {[&](std::string name, std::string value) { + if (unsetVars.contains(name)) + throw UsageError( + "Cannot set environment variable '%s' that is unset with '%s'", name, "--unset-env-var"); + + if (setVars.contains(name)) + throw UsageError( + "Duplicate definition of environment variable '%s' with '%s' is ambiguous", name, "--set-env-var"); + + setVars.insert_or_assign(name, value); + }}, }); } void MixEnvironment::setEnviron() { - if (ignoreEnvironment) { - if (!unset.empty()) - throw UsageError("--unset does not make sense with --ignore-environment"); + if (ignoreEnvironment && !unsetVars.empty()) + throw UsageError("--unset-env-var does not make sense with --ignore-env"); - for (const auto & var : keep) { - auto val = getenv(var.c_str()); - if (val) stringsEnv.emplace_back(fmt("%s=%s", var.c_str(), val)); - } + if (!ignoreEnvironment && !keepVars.empty()) + throw UsageError("--keep-env-var does not make sense without --ignore-env"); - vectorEnv = stringsToCharPtrs(stringsEnv); - environ = vectorEnv.data(); - } else { - if (!keep.empty()) - throw UsageError("--keep does not make sense without --ignore-environment"); + auto env = getEnv(); - for (const auto & var : unset) - unsetenv(var.c_str()); - } + if (ignoreEnvironment) + std::erase_if(env, [&](const auto & var) { + return !keepVars.contains(var.first); + }); + + for (const auto & [name, value] : setVars) + env[name] = value; + + if (!unsetVars.empty()) + std::erase_if(env, [&](const auto & var) { + return unsetVars.contains(var.first); + }); + + replaceEnv(env); + + return; } } diff --git a/src/libcmd/command.hh b/src/libcmd/command.hh index 4a72627ed4d..b823a1c6b05 100644 --- a/src/libcmd/command.hh +++ b/src/libcmd/command.hh @@ -315,17 +315,18 @@ struct MixDefaultProfile : MixProfile struct MixEnvironment : virtual Args { - StringSet keep, unset; - Strings stringsEnv; - std::vector vectorEnv; + StringSet keepVars; + StringSet unsetVars; + std::map setVars; bool ignoreEnvironment; MixEnvironment(); /*** - * Modify global environ based on `ignoreEnvironment`, `keep`, and - * `unset`. It's expected that exec will be called before this class - * goes out of scope, otherwise `environ` will become invalid. + * Modify global environ based on `ignoreEnvironment`, `keep`, + * `unset`, and `added`. It's expected that exec will be called + * before this class goes out of scope, otherwise `environ` will + * become invalid. */ void setEnviron(); }; diff --git a/src/libutil/environment-variables.hh b/src/libutil/environment-variables.hh index 879e1f30492..1a95f5c97e7 100644 --- a/src/libutil/environment-variables.hh +++ b/src/libutil/environment-variables.hh @@ -13,6 +13,8 @@ namespace nix { +static constexpr auto environmentVariablesCategory = "Options that change environment variables"; + /** * @return an environment variable. */ diff --git a/tests/functional/flakes/develop.sh b/tests/functional/flakes/develop.sh index df24f19f08b..ba41a5febfc 100755 --- a/tests/functional/flakes/develop.sh +++ b/tests/functional/flakes/develop.sh @@ -47,9 +47,9 @@ echo "\$ENVVAR" EOF )" = "a" ]] -# Test whether `nix develop --ignore-environment` does _not_ pass through environment variables. +# Test whether `nix develop --ignore-env` does _not_ pass through environment variables. [[ -z "$( - ENVVAR=a nix develop --ignore-environment --no-write-lock-file .#hello < Date: Thu, 19 Sep 2024 18:43:22 +0200 Subject: [PATCH 2/3] test(functional): add tests for new environment operation flags --- maintainers/flake-module.nix | 1 - src/libcmd/command.cc | 6 +-- tests/functional/flakes/develop.sh | 87 +++++++++++++++++++++++++++--- 3 files changed, 81 insertions(+), 13 deletions(-) diff --git a/maintainers/flake-module.nix b/maintainers/flake-module.nix index 225b0b30099..5bf7837f7de 100644 --- a/maintainers/flake-module.nix +++ b/maintainers/flake-module.nix @@ -547,7 +547,6 @@ ''^tests/functional/flakes/absolute-paths\.sh$'' ''^tests/functional/flakes/check\.sh$'' ''^tests/functional/flakes/config\.sh$'' - ''^tests/functional/flakes/develop\.sh$'' ''^tests/functional/flakes/flakes\.sh$'' ''^tests/functional/flakes/follow-paths\.sh$'' ''^tests/functional/flakes/prefetch\.sh$'' diff --git a/src/libcmd/command.cc b/src/libcmd/command.cc index 295d290c5d9..2d9731ca6c5 100644 --- a/src/libcmd/command.cc +++ b/src/libcmd/command.cc @@ -321,11 +321,7 @@ MixEnvironment::MixEnvironment() : ignoreEnvironment(false) addFlag({ .longName = "set-env-var", .shortName = 's', - .description = "Add/override an environment variable *name* with *value*.\n\n" - "> **Notes**\n" - ">\n" - "> Duplicate definitions will be overwritten, last one wins.\n\n" - "> Cancles out with `--unset-env-var`.\n\n", + .description = "Sets an environment variable *name* with *value*.", .category = environmentVariablesCategory, .labels = {"name", "value"}, .handler = {[&](std::string name, std::string value) { diff --git a/tests/functional/flakes/develop.sh b/tests/functional/flakes/develop.sh index ba41a5febfc..2e75081d476 100755 --- a/tests/functional/flakes/develop.sh +++ b/tests/functional/flakes/develop.sh @@ -5,11 +5,11 @@ source ../common.sh TODO_NixOS clearStore -rm -rf $TEST_HOME/.cache $TEST_HOME/.config $TEST_HOME/.local +rm -rf "$TEST_HOME/.cache" "$TEST_HOME/.config" "$TEST_HOME/.local" # Create flake under test. -cp ../shell-hello.nix "${config_nix}" $TEST_HOME/ -cat <$TEST_HOME/flake.nix +cp ../shell-hello.nix "$config_nix" "$TEST_HOME/" +cat <"$TEST_HOME/flake.nix" { inputs.nixpkgs.url = "$TEST_HOME/nixpkgs"; outputs = {self, nixpkgs}: { @@ -24,13 +24,13 @@ cat <$TEST_HOME/flake.nix EOF # Create fake nixpkgs flake. -mkdir -p $TEST_HOME/nixpkgs -cp "${config_nix}" ../shell.nix $TEST_HOME/nixpkgs +mkdir -p "$TEST_HOME/nixpkgs" +cp "${config_nix}" ../shell.nix "$TEST_HOME/nixpkgs" # `config.nix` cannot be gotten via build dir / env var (runs afoul pure eval). Instead get from flake. removeBuildDirRef "$TEST_HOME/nixpkgs"/*.nix -cat <$TEST_HOME/nixpkgs/flake.nix +cat <"$TEST_HOME/nixpkgs/flake.nix" { outputs = {self}: { legacyPackages.$system.bashInteractive = (import ./shell.nix {}).bashInteractive; @@ -38,7 +38,7 @@ cat <$TEST_HOME/nixpkgs/flake.nix } EOF -cd $TEST_HOME +cd "$TEST_HOME" # Test whether `nix develop` passes through environment variables. [[ "$( @@ -54,6 +54,79 @@ echo "\$ENVVAR" EOF )" ]] +# Test wether `--keep-env-var` keeps the environment variable. +( + expect='BAR' + got="$(FOO='BAR' nix develop --ignore-env --keep-env-var FOO --no-write-lock-file .#hello < Date: Thu, 19 Sep 2024 19:15:31 +0200 Subject: [PATCH 3/3] chore: run formatters --- maintainers/flake-module.nix | 2 - src/libcmd/command.cc | 82 ++++++++++++++++++------------------ src/libcmd/command.hh | 33 +++++++-------- 3 files changed, 57 insertions(+), 60 deletions(-) diff --git a/maintainers/flake-module.nix b/maintainers/flake-module.nix index 5bf7837f7de..fdb031302eb 100644 --- a/maintainers/flake-module.nix +++ b/maintainers/flake-module.nix @@ -28,8 +28,6 @@ ''^src/build-remote/build-remote\.cc$'' ''^src/libcmd/built-path\.cc$'' ''^src/libcmd/built-path\.hh$'' - ''^src/libcmd/command\.cc$'' - ''^src/libcmd/command\.hh$'' ''^src/libcmd/common-eval-args\.cc$'' ''^src/libcmd/common-eval-args\.hh$'' ''^src/libcmd/editor-for\.cc$'' diff --git a/src/libcmd/command.cc b/src/libcmd/command.cc index 2d9731ca6c5..1a4c76ec5be 100644 --- a/src/libcmd/command.cc +++ b/src/libcmd/command.cc @@ -23,7 +23,8 @@ nix::Commands RegisterCommand::getCommandsFor(const std::vector & p if (name.size() == prefix.size() + 1) { bool equal = true; for (size_t i = 0; i < prefix.size(); ++i) - if (name[i] != prefix[i]) equal = false; + if (name[i] != prefix[i]) + equal = false; if (equal) res.insert_or_assign(name[prefix.size()], command); } @@ -42,16 +43,16 @@ void NixMultiCommand::run() std::set subCommandTextLines; 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)); + std::string markdownError = + fmt("`nix %s` requires a sub-command. Available sub-commands:\n\n%s\n", + commandName, + concatStringsSep("\n", subCommandTextLines)); throw UsageError(renderMarkdownToTerminal(markdownError)); } command->second->run(); } -StoreCommand::StoreCommand() -{ -} +StoreCommand::StoreCommand() {} ref StoreCommand::getStore() { @@ -126,10 +127,8 @@ ref EvalCommand::getEvalStore() ref EvalCommand::getEvalState() { if (!evalState) { - evalState = - std::allocate_shared( - traceable_allocator(), - lookupPath, getEvalStore(), fetchSettings, evalSettings, getStore()); + evalState = std::allocate_shared( + traceable_allocator(), lookupPath, getEvalStore(), fetchSettings, evalSettings, getStore()); evalState->repair = repair; @@ -144,7 +143,8 @@ MixOperateOnOptions::MixOperateOnOptions() { addFlag({ .longName = "derivation", - .description = "Operate on the [store derivation](@docroot@/glossary.md#gloss-store-derivation) rather than its outputs.", + .description = + "Operate on the [store derivation](@docroot@/glossary.md#gloss-store-derivation) rather than its outputs.", .category = installablesCategory, .handler = {&operateOn, OperateOn::Derivation}, }); @@ -233,46 +233,48 @@ void StorePathCommand::run(ref store, StorePaths && storePaths) MixProfile::MixProfile() { - addFlag({ - .longName = "profile", - .description = "The profile to operate on.", - .labels = {"path"}, - .handler = {&profile}, - .completer = completePath - }); + addFlag( + {.longName = "profile", + .description = "The profile to operate on.", + .labels = {"path"}, + .handler = {&profile}, + .completer = completePath}); } void MixProfile::updateProfile(const StorePath & storePath) { - if (!profile) return; + if (!profile) + return; auto store = getStore().dynamic_pointer_cast(); - if (!store) throw Error("'--profile' is not supported for this Nix store"); + if (!store) + throw Error("'--profile' is not supported for this Nix store"); auto profile2 = absPath(*profile); - switchLink(profile2, - createGeneration(*store, profile2, storePath)); + switchLink(profile2, createGeneration(*store, profile2, storePath)); } void MixProfile::updateProfile(const BuiltPaths & buildables) { - if (!profile) return; + if (!profile) + return; StorePaths result; for (auto & buildable : buildables) { - std::visit(overloaded { - [&](const BuiltPath::Opaque & bo) { - result.push_back(bo.path); + std::visit( + overloaded{ + [&](const BuiltPath::Opaque & bo) { result.push_back(bo.path); }, + [&](const BuiltPath::Built & bfd) { + for (auto & output : bfd.outputs) { + result.push_back(output.second); + } + }, }, - [&](const BuiltPath::Built & bfd) { - for (auto & output : bfd.outputs) { - result.push_back(output.second); - } - }, - }, buildable.raw()); + buildable.raw()); } if (result.size() != 1) - throw UsageError("'--profile' requires that the arguments produce a single store path, but there are %d", result.size()); + throw UsageError( + "'--profile' requires that the arguments produce a single store path, but there are %d", result.size()); updateProfile(result[0]); } @@ -282,7 +284,8 @@ MixDefaultProfile::MixDefaultProfile() profile = getDefaultProfile(); } -MixEnvironment::MixEnvironment() : ignoreEnvironment(false) +MixEnvironment::MixEnvironment() + : ignoreEnvironment(false) { addFlag({ .longName = "ignore-env", @@ -338,7 +341,8 @@ MixEnvironment::MixEnvironment() : ignoreEnvironment(false) }); } -void MixEnvironment::setEnviron() { +void MixEnvironment::setEnviron() +{ if (ignoreEnvironment && !unsetVars.empty()) throw UsageError("--unset-env-var does not make sense with --ignore-env"); @@ -348,17 +352,13 @@ void MixEnvironment::setEnviron() { auto env = getEnv(); if (ignoreEnvironment) - std::erase_if(env, [&](const auto & var) { - return !keepVars.contains(var.first); - }); + std::erase_if(env, [&](const auto & var) { return !keepVars.contains(var.first); }); for (const auto & [name, value] : setVars) env[name] = value; if (!unsetVars.empty()) - std::erase_if(env, [&](const auto & var) { - return unsetVars.contains(var.first); - }); + std::erase_if(env, [&](const auto & var) { return unsetVars.contains(var.first); }); replaceEnv(env); diff --git a/src/libcmd/command.hh b/src/libcmd/command.hh index b823a1c6b05..8da4327c25d 100644 --- a/src/libcmd/command.hh +++ b/src/libcmd/command.hh @@ -13,7 +13,7 @@ namespace nix { extern std::string programPath; -extern char * * savedArgv; +extern char ** savedArgv; class EvalState; struct Pos; @@ -24,7 +24,8 @@ static constexpr Command::Category catSecondary = 100; static constexpr Command::Category catUtility = 101; static constexpr Command::Category catNixInstallation = 102; -static constexpr auto installablesCategory = "Options that change the interpretation of [installables](@docroot@/command-ref/new-cli/nix.md#installables)"; +static constexpr auto installablesCategory = + "Options that change the interpretation of [installables](@docroot@/command-ref/new-cli/nix.md#installables)"; struct NixMultiCommand : MultiCommand, virtual Command { @@ -112,7 +113,9 @@ struct MixFlakeOptions : virtual Args, EvalCommand * arguments) so that the completions for these flags can use them. */ virtual std::vector getFlakeRefsForCompletion() - { return {}; } + { + return {}; + } }; struct SourceExprCommand : virtual Args, MixFlakeOptions @@ -122,11 +125,9 @@ struct SourceExprCommand : virtual Args, MixFlakeOptions SourceExprCommand(); - Installables parseInstallables( - ref store, std::vector ss); + Installables parseInstallables(ref store, std::vector ss); - ref parseInstallable( - ref store, const std::string & installable); + ref parseInstallable(ref store, const std::string & installable); virtual Strings getDefaultFlakeAttrPaths(); @@ -272,10 +273,10 @@ struct RegisterCommand typedef std::map, std::function()>> Commands; static Commands * commands; - RegisterCommand(std::vector && name, - std::function()> command) + RegisterCommand(std::vector && name, std::function()> command) { - if (!commands) commands = new Commands; + if (!commands) + commands = new Commands; commands->emplace(name, command); } @@ -285,13 +286,13 @@ struct RegisterCommand template static RegisterCommand registerCommand(const std::string & name) { - return RegisterCommand({name}, [](){ return make_ref(); }); + return RegisterCommand({name}, []() { return make_ref(); }); } template static RegisterCommand registerCommand2(std::vector && name) { - return RegisterCommand(std::move(name), [](){ return make_ref(); }); + return RegisterCommand(std::move(name), []() { return make_ref(); }); } struct MixProfile : virtual StoreCommand @@ -313,7 +314,8 @@ struct MixDefaultProfile : MixProfile MixDefaultProfile(); }; -struct MixEnvironment : virtual Args { +struct MixEnvironment : virtual Args +{ StringSet keepVars; StringSet unsetVars; @@ -350,9 +352,6 @@ void completeFlakeRefWithFragment( std::string showVersions(const std::set & versions); void printClosureDiff( - ref store, - const StorePath & beforePath, - const StorePath & afterPath, - std::string_view indent); + ref store, const StorePath & beforePath, const StorePath & afterPath, std::string_view indent); }