From 0488a0181df4bc5848ebb7b96e8ed0f49d64bffc Mon Sep 17 00:00:00 2001 From: Maximilian Bosch Date: Sun, 8 Mar 2026 15:05:20 +0100 Subject: [PATCH] libcmd: allow setting nested attributes via `--arg`/`--argstr` Closes #496 When running nix-build ../nixpkgs --arg config.allowUnfree true -A hello-unfree the package `hello-unfree` is now built rather than getting an eval-error rejecting instantiation. This is because `config.allowUnfree` is now interpreted as nested attribute-set declaration, similar to how it's done in `nix repl`. To prevent sudden breakage, this behavior was carefully deprecated with Nix throwing an error if the identifier for `--arg` is not a pure identifier, but an expression as above. Any kind of merging is rejected. I.e. doing nix-build ../nixpkgs --arg config '{cudaSupport = true;}' --arg config.allowUnfree true is prohibited. That way we don't have to think about merge semantics for cases like this (or even worse `--arg config 'rec { ... }'`). Another nice side-effect of this is that we don't need to create an EvalState to force the values and implement merging. Change-Id: I8b560883a4468a3f32f915764b08f5fdd8fe71bb --- doc/manual/rl-next/arg-behavior.md | 10 ++ doc/manual/src/command-ref/opt-common.md | 6 ++ lix/libcmd/common-eval-args.cc | 109 +++++++++++++------ lix/libexpr/attr-path.cc | 9 +- lix/libexpr/attr-path.hh | 2 +- tests/functional2/eval/test_autoargs_cli.py | 110 ++++++++++++++++++++ 6 files changed, 209 insertions(+), 37 deletions(-) create mode 100644 doc/manual/rl-next/arg-behavior.md create mode 100644 tests/functional2/eval/test_autoargs_cli.py diff --git a/doc/manual/rl-next/arg-behavior.md b/doc/manual/rl-next/arg-behavior.md new file mode 100644 index 000000000..3afac40db --- /dev/null +++ b/doc/manual/rl-next/arg-behavior.md @@ -0,0 +1,10 @@ +--- +synopsis: "allow setting nested attributes via `--arg`/`--argstr`" +cls: [5338] +category: "Features" +credits: [ma27] +issues: [fj#496] +--- + +Passing `--arg config.allowUnfree true` to e.g. `nix-build` now results in `config` with value +`{ allowUnfree = true; }` passed to the expression. diff --git a/doc/manual/src/command-ref/opt-common.md b/doc/manual/src/command-ref/opt-common.md index 0e76d9538..dc7850e78 100644 --- a/doc/manual/src/command-ref/opt-common.md +++ b/doc/manual/src/command-ref/opt-common.md @@ -177,6 +177,12 @@ Most commands in Lix accept the following command-line options: You can override this using `--arg`, e.g., `nix-env --install --attr pkgname --arg system \"i686-freebsd\"`. (Note that since the argument is a Nix string literal, you have to escape the quotes.) + Additionally, dots are interpreted as attribute-path separators. + I.e. `nix-instantiate '' -A hello-unfree --arg config.allowUnfree true` will result in an argument `config` with value `{ allowUnfree = true; }` being passed to ``. + + Please note that merging of different arguments is rejected. + I.e. `--arg config '{ cudaSupport = true; }' --arg config.allowUnfree true` will not work whereas `--arg config.cudaSupport true --arg config.allowUnfree true` is accepted. + - [`--argstr`](#opt-argstr) *name* *value* This option is like `--arg`, only the value is not a Nix expression but a string. diff --git a/lix/libcmd/common-eval-args.cc b/lix/libcmd/common-eval-args.cc index 65a998549..57416670d 100644 --- a/lix/libcmd/common-eval-args.cc +++ b/lix/libcmd/common-eval-args.cc @@ -1,3 +1,6 @@ +#include "libexpr/value.hh" +#include "libutil/strings.hh" +#include "lix/libexpr/attr-path.hh" #include "lix/libexpr/eval-settings.hh" #include "lix/libcmd/common-eval-args.hh" #include "lix/libmain/shared.hh" @@ -10,31 +13,10 @@ #include "lix/libcmd/command.hh" #include "lix/libutil/async.hh" #include "lix/libutil/error.hh" -#include "lix/libutil/regex.hh" - -#include +#include namespace nix { -static std::regex const identifierRegex = regex::parse("^[A-Za-z_][A-Za-z0-9_'-]*$"); -static void checkValidNixIdentifier(const std::string & name) -{ - std::smatch match; - if (!std::regex_match(name, match, identifierRegex)) { - throw UsageError( - "This invocation specifies a value for argument '%s' " - "which isn't a valid Nix identifier. " - "The project is dropping support for this so that it's possible to make e.g. " - "'%s' evaluating to '%s' in the future. " - "If you depend on this behavior, please reach out in " - " so we can discuss your use-case.", - name, - "--arg config.allowUnfree true", - "{ config.allowUnfree = true; }" - ); - } -} - MixEvalArgs::MixEvalArgs() { addFlag( @@ -42,10 +24,7 @@ MixEvalArgs::MixEvalArgs() .description = "Pass the value *expr* as the argument *name* to Nix functions.", .category = category, .labels = {"name", "expr"}, - .handler = {[&](std::string name, std::string expr) { - checkValidNixIdentifier(name); - autoArgs[name] = ExprArgument(expr); - }}} + .handler = {[&](std::string name, std::string expr) { autoArgs[name] = ExprArgument(expr); }}} ); addFlag({ @@ -53,10 +32,7 @@ MixEvalArgs::MixEvalArgs() .description = "Pass the string *string* as the argument *name* to Nix functions.", .category = category, .labels = {"name", "string"}, - .handler = {[&](std::string name, std::string s) { - checkValidNixIdentifier(name); - autoArgs[name] = StringArgument(s); - }}, + .handler = {[&](std::string name, std::string s) { autoArgs[name] = StringArgument(s); }}, }); addFlag({ @@ -179,9 +155,75 @@ MixEvalArgs::MixEvalArgs() }); } +struct AutoArgsContainer +{ + std::map> data; + + Bindings * toBindings(Evaluator & state) + { + auto bb = state.buildBindings(data.size()); + + for (auto & [sym, v] : data) { + bb.insert( + sym, + std::visit( + overloaded{ + [&](Value & v) { return v; }, + [&](AutoArgsContainer & aac) -> Value { + return {NewValueAs::attrs, aac.toBindings(state)}; + } + }, + v + ) + ); + } + + return bb.finish(); + } +}; + +static void addAutoArgRecursive( + AutoArgsContainer & container, + Evaluator & state, + std::vector && path, + Value & val, + const std::string_view pathStr +) +{ + auto * data = &container.data; + auto size = path.size(); + for (auto [i, pathCmp] : enumerate(path)) { + auto next = state.symbols.create(pathCmp); + auto entry = data->find(next); + + if (entry == data->end()) { + if (i == size - 1) { + (*data)[next] = val; + } else { + (*data)[next] = AutoArgsContainer{}; + data = &std::get((*data)[next]).data; + } + } else { + std::visit( + overloaded{ + [&](Value & v) { + throw Error( + "Cannot set %s via --arg/--argstr when it's the path-extension of another " + "auto-argument!", + pathStr + ); + }, + [&](AutoArgsContainer & v) { data = &v.data; } + }, + entry->second + ); + } + } +} + Bindings * MixEvalArgs::getAutoArgs(Evaluator & state) { - auto res = state.buildBindings(autoArgs.size()); + AutoArgsContainer aac; for (auto & [name, value] : autoArgs) { Value v = std::visit( overloaded{ @@ -193,9 +235,10 @@ Bindings * MixEvalArgs::getAutoArgs(Evaluator & state) value ); - res.insert(state.symbols.create(name), v); + addAutoArgRecursive(aac, state, parseAttrPath(name, false), v, name); } - return res.finish(); + + return aac.toBindings(state); } kj::Promise>> diff --git a/lix/libexpr/attr-path.cc b/lix/libexpr/attr-path.cc index 67aa78260..4a02c2358 100644 --- a/lix/libexpr/attr-path.cc +++ b/lix/libexpr/attr-path.cc @@ -7,8 +7,7 @@ namespace nix { - -std::vector parseAttrPath(std::string_view const s) +std::vector parseAttrPath(std::string_view const s, bool allowRhsTrailingDot) { std::vector res; std::string cur; @@ -52,7 +51,11 @@ std::vector parseAttrPath(std::string_view const s) } ++i; } - if (haveData) res.push_back(cur); + if (haveData) { + res.push_back(cur); + } else if (!allowRhsTrailingDot) { + throw ParseError("Trailing dot on the right-hand side of path expr '%1%' is not allowed!", s); + }; return res; } diff --git a/lix/libexpr/attr-path.hh b/lix/libexpr/attr-path.hh index 78667a7e1..7df91e1e5 100644 --- a/lix/libexpr/attr-path.hh +++ b/lix/libexpr/attr-path.hh @@ -25,7 +25,7 @@ std::pair findPackageFilename(EvalState & state, Value & v * Such an attr path is a dot-separated sequence of attribute names, which are possibly quoted. * No escaping is performed; attribute names containing double quotes are unrepresentable. */ -std::vector parseAttrPath(std::string_view const s); +std::vector parseAttrPath(std::string_view const s, bool allowRhsTrailingDot = true); /** * Converts an attr path from a list of strings into a string once more. diff --git a/tests/functional2/eval/test_autoargs_cli.py b/tests/functional2/eval/test_autoargs_cli.py new file mode 100644 index 000000000..459433b9d --- /dev/null +++ b/tests/functional2/eval/test_autoargs_cli.py @@ -0,0 +1,110 @@ +from typing import Any + +import pytest + +from testlib.fixtures.nix import Nix + + +def do_evaluate(nix: Nix, args: list[str], expect_success: bool = True) -> dict[str, Any] | str: + res = ( + nix.nix_instantiate( + [ + "--eval", + "--json", + "-E", + "{ arg1, arg2 ? null }: { inherit arg1 arg2; }", + "--strict", + *args, + ] + ) + .run() + .expect(0 if expect_success else 1) + ) + + if expect_success: + return res.json() + return res.stderr_s + + +def test_trivial(nix: Nix): + res = do_evaluate(nix, args=["--arg", "arg1", "[ 1 2 3 ]", "--arg", "arg2", "1"]) + + assert res["arg1"] == [1, 2, 3] + assert res["arg2"] == 1 + + +def test_recursive(nix: Nix): + res = do_evaluate( + nix, + args=["--arg", "arg1.foo.bar", "1", "--arg", "arg1.foo.baz", "2", "--arg", "arg1.bar", "3"], + ) + + assert res["arg1"] == {"foo": {"bar": 1, "baz": 2}, "bar": 3} + assert res["arg2"] is None + + +@pytest.mark.parametrize( + ("attribute_path", "expected"), + [("arg1", 2), ("arg1.foo", {"foo": 2}), ("arg1.foo.bar", {"foo": {"bar": 2}})], +) +def test_override(nix: Nix, attribute_path: str, expected: Any): + res = do_evaluate(nix, args=["--arg", attribute_path, "1", "--arg", attribute_path, "2"]) + + assert res["arg1"] == expected + + +def test_quoting(nix: Nix): + res = do_evaluate( + nix, + args=[ + "--arg", + "arg1.foo.bar", + "1", + "--arg", + 'arg1."foo bar baz".baz', + "2", + "--arg", + "arg1.bar", + "2", + ], + ) + + assert res["arg1"] == {"foo": {"bar": 1}, "bar": 2, "foo bar baz": {"baz": 2}} + assert res["arg2"] is None + + +def test_quoting_error(nix: Nix): + res = do_evaluate(nix, ["--arg", 'arg1."foo bar.baz', "1"], expect_success=False) + assert "error: missing closing quote in selection path 'arg1.\"foo bar.baz'" in res + + +def test_trailing_dot(nix: Nix): + # This is what `parseAttrPath` from `libutil` does and is consistent with the selection path + # passed to e.g. `nix-build -A`. + res = do_evaluate(nix, args=["--arg", "arg1.bar.", "[ 1 2 3 ]"], expect_success=False) + + assert ( + "error: Trailing dot on the right-hand side of path expr 'arg1.bar.' is not allowed!" in res + ) + + +@pytest.mark.parametrize( + "args", + [ + ["--arg", "arg1.foo", "1", "--arg", "arg1.foo.bar", "2"], + ["--arg", "arg1.foo.bar", "2", "--arg", "arg1.foo", "1"], + ], +) +def test_conflict(nix: Nix, args: list[str]): + res = do_evaluate(nix, args, expect_success=False) + + assert ( + "error: Cannot set arg1.foo.bar via --arg/--argstr when it's the path-extension of another auto-argument!" + in res + ) + + +@pytest.mark.parametrize("selection", ["foo..bar", "foo.bar.."]) +def test_no_empty_items(nix: Nix, selection: str): + res = do_evaluate(nix, ["--arg", selection, "1"], expect_success=False) + assert f"error: consecutive dots not allowed in selection path '{selection}'" in res