From 21c4583dd637bf0fa13e9359b9a8b13e9c3c3e42 Mon Sep 17 00:00:00 2001 From: Alex Tercete Date: Mon, 23 Sep 2024 14:58:34 +0100 Subject: [PATCH 1/6] chore: apply buildifier to files --- labgrid/genrule/rule.bzl | 4 ++-- labgrid/run_binary/rule.bzl | 22 +++++++++++----------- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/labgrid/genrule/rule.bzl b/labgrid/genrule/rule.bzl index f692f85d..d4f53d73 100644 --- a/labgrid/genrule/rule.bzl +++ b/labgrid/genrule/rule.bzl @@ -1,5 +1,5 @@ -load("//labgrid/cfg:store.bzl", _cfg = "store") load("@bazel_skylib//lib:shell.bzl", "shell") +load("//labgrid/cfg:store.bzl", _cfg = "store") visibility("//...") @@ -19,7 +19,7 @@ ATTRS = { "srcs": attr.label_list( doc = "A list of inputs for this rule, such as files to process.", cfg = "target", - allow_files = True + allow_files = True, ), "tools": attr.label_list( doc = "A list of tools for this rule, that will run on the executor.", diff --git a/labgrid/run_binary/rule.bzl b/labgrid/run_binary/rule.bzl index 4f682b97..28ea2726 100644 --- a/labgrid/run_binary/rule.bzl +++ b/labgrid/run_binary/rule.bzl @@ -7,10 +7,10 @@ DOC = """Executes a binary on the host within a LabGrid environment.""" ATTRS = { "tool": attr.label( doc = "The tool to run in the action.\n\nMust be the label of a *_binary rule," + - " of a rule that generates an executable file, or of a file that can be" + - " executed as a subprocess (e.g. an .exe or .bat file on Windows or a binary" + - " with executable permission on Linux). This label is available for" + - " `$(execpath)` and `$(location)` expansion in `args` and `env`.", + " of a rule that generates an executable file, or of a file that can be" + + " executed as a subprocess (e.g. an .exe or .bat file on Windows or a binary" + + " with executable permission on Linux). This label is available for" + + " `$(execpath)` and `$(location)` expansion in `args` and `env`.", executable = True, allow_files = True, mandatory = True, @@ -18,23 +18,23 @@ ATTRS = { ), "env": attr.string_dict( doc = "Environment variables of the action.\n\nSubject to " + - " [`$(execpath)` and `$(location)`](https://bazel.build/reference/be/make-variables#predefined_label_variables)" + - " expansion.", + " [`$(execpath)` and `$(location)`](https://bazel.build/reference/be/make-variables#predefined_label_variables)" + + " expansion.", ), "srcs": attr.label_list( allow_files = True, doc = "Additional inputs of the action.\n\nThese labels are available for" + - " `$(execpath)` and `$(location)` expansion in `args` and `env`.", + " `$(execpath)` and `$(location)` expansion in `args` and `env`.", ), "outs": attr.output_list( mandatory = True, doc = "Output files generated by the action.\n\nThese labels are available for" + - " `$(execpath)` and `$(location)` expansion in `args` and `env`.", + " `$(execpath)` and `$(location)` expansion in `args` and `env`.", ), "args": attr.string_list( doc = "Command line arguments of the binary.\n\nSubject to" + - " [`$(execpath)` and `$(location)`](https://bazel.build/reference/be/make-variables#predefined_label_variables)" + - " expansion.", + " [`$(execpath)` and `$(location)`](https://bazel.build/reference/be/make-variables#predefined_label_variables)" + + " expansion.", ), "platform": attr.label( doc = "Platform to transition to.", @@ -45,7 +45,7 @@ ATTRS = { default = "//labgrid/executor", executable = True, cfg = "exec", - ) + ), } def implementation(ctx): -- GitLab From f52f2a8613e4173ec4f418afd265e8b5a5f7acf5 Mon Sep 17 00:00:00 2001 From: Alex Tercete Date: Mon, 23 Sep 2024 16:19:06 +0100 Subject: [PATCH 2/6] refactor(executor): use args file instead of env vars This creates a bit of duplication between `labgrid_genrule` and `labgrid_run_binary` in how they consume the executor. Ideally, the `//labgrid/executor` would bring default arguments baked in, but I couldn't find a clean way to do it. Something for later. --- labgrid/executor/BUILD.bazel | 24 ++++++------------------ labgrid/executor/args.bzl | 24 ++++++++++++++++++++++++ labgrid/executor/executor.py | 5 ----- labgrid/genrule/rule.bzl | 8 ++++++++ labgrid/run_binary/rule.bzl | 9 ++++++++- 5 files changed, 46 insertions(+), 24 deletions(-) create mode 100644 labgrid/executor/args.bzl diff --git a/labgrid/executor/BUILD.bazel b/labgrid/executor/BUILD.bazel index 20db4415..25b3c110 100644 --- a/labgrid/executor/BUILD.bazel +++ b/labgrid/executor/BUILD.bazel @@ -1,11 +1,14 @@ load("@rules_python//python:defs.bzl", "py_binary", "py_library", "py_test") +load(":args.bzl", "args") + +args( + name = "args", + visibility = ["//:__subpackages__"], +) py_binary( name = "executor", srcs = ["executor.py"], - env = { - "LABGRID_EXECUTOR_MANAGER": "$(location //labgrid/manager)", - }, tags = ["manual"], visibility = ["//:__subpackages__"], deps = [ @@ -37,18 +40,3 @@ py_test( main = "executor.py", deps = ["host"], ) - -py_test( - name = "envvar", - size = "small", - srcs = ["executor.py"], - args = [ - "echo", - "$${DATA}", - ], - env = { - "LABGRID_EXECUTOR_MANAGER": "labgrid.executor.host:manager", - }, - main = "executor.py", - deps = ["host"], -) diff --git a/labgrid/executor/args.bzl b/labgrid/executor/args.bzl new file mode 100644 index 00000000..1d16093f --- /dev/null +++ b/labgrid/executor/args.bzl @@ -0,0 +1,24 @@ +visibility("//labgrid/executor/...") + +DOC = "Creates an args file to be passed to `//labgrid/executor`" + +ATTRS = { + "_manager": attr.label( + allow_single_file = True, + doc = "The manager to use", + default = "//labgrid/manager:src", + cfg = "exec", + ), +} + +def implementation(ctx): + output = ctx.actions.declare_file("executor-args.txt") + content = "--manager={}".format(ctx.file._manager.short_path) + ctx.actions.write(output, content) + return DefaultInfo(files = depset([output])) + +args = rule( + doc = DOC, + attrs = ATTRS, + implementation = implementation, +) diff --git a/labgrid/executor/executor.py b/labgrid/executor/executor.py index 68f9cf7d..8f69957b 100644 --- a/labgrid/executor/executor.py +++ b/labgrid/executor/executor.py @@ -52,11 +52,6 @@ def arguments(prsr: ArgumentParser) -> None: "--manager", help="A manager to load from a Python module, of the form `module.to.load:Manager`.", type=load, - default=( - load(environ["LABGRID_EXECUTOR_MANAGER"]) - if "LABGRID_EXECUTOR_MANAGER" in environ - else None - ), ) diff --git a/labgrid/genrule/rule.bzl b/labgrid/genrule/rule.bzl index d4f53d73..fd0a3de2 100644 --- a/labgrid/genrule/rule.bzl +++ b/labgrid/genrule/rule.bzl @@ -40,6 +40,12 @@ ATTRS = { executable = True, cfg = "exec", ), + "_executor_args": attr.label( + allow_single_file = True, + doc = "CLI args for the Labgrid environment executor", + default = "//labgrid/executor:args", + cfg = "exec", + ), "_sh": attr.label( doc = "The Shell interpreter", default = "@ape//:bash", @@ -79,6 +85,7 @@ def implementation(ctx): cmd = cmd.replace(key, value) args = ctx.actions.args() + args.add(ctx.file._executor_args, format = "@%s") args.add("--") args.add(ctx.executable._sh) args.add("-c") @@ -87,6 +94,7 @@ def implementation(ctx): ctx.actions.run( executable = ctx.executable._executor, arguments = [args], + inputs = [ctx.file._executor_args], outputs = ctx.outputs.outs, tools = [t.files_to_run for t in (ctx.attr.tools + ctx.attr.srcs)] + [ctx.executable._sh], env = ctx.attr._executor[RunEnvironmentInfo].environment, diff --git a/labgrid/run_binary/rule.bzl b/labgrid/run_binary/rule.bzl index 28ea2726..f46d0292 100644 --- a/labgrid/run_binary/rule.bzl +++ b/labgrid/run_binary/rule.bzl @@ -46,6 +46,12 @@ ATTRS = { executable = True, cfg = "exec", ), + "_executor_args": attr.label( + allow_single_file = True, + doc = "CLI args for the Labgrid environment executor", + default = "//labgrid/executor:args", + cfg = "exec", + ), } def implementation(ctx): @@ -71,13 +77,14 @@ def implementation(ctx): } args = ctx.actions.args() + args.add(ctx.file._executor_args, format = "@%s") args.add("--") args.add(ctx.executable.tool) args.add_all(expanded_args) ctx.actions.run( outputs = ctx.outputs.outs, - inputs = ctx.files.srcs, + inputs = ctx.files.srcs + [ctx.file._executor_args], tools = [ctx.executable.tool], executable = ctx.executable._executor, arguments = [args], -- GitLab From 257abec8dde188fe28ec580ab2c4cb9e2523a042 Mon Sep 17 00:00:00 2001 From: Alex Tercete Date: Tue, 24 Sep 2024 21:06:30 +0100 Subject: [PATCH 3/6] fix(run): improve arg handling Previously, a default value was used for the --config arg when the LG_ENV env var wasn't set. We now fail early instead. --- labgrid/run/run.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/labgrid/run/run.py b/labgrid/run/run.py index 0c1ff803..990284b5 100644 --- a/labgrid/run/run.py +++ b/labgrid/run/run.py @@ -25,10 +25,9 @@ def arguments(prsr: ArgumentParser) -> None: "arguments", metavar="ARG", nargs="*", help="Command to run over SSH." ) prsr.add_argument( - "--lg-env", - help="The LabGrid environment configuration.", - dest="config", - default=PurePath(environ.get("LG_ENV", "config.yaml")), + "--config", + help="The LabGrid configuration.", + default=PurePath(environ["LG_ENV"]), type=PurePath, ) -- GitLab From f8c321cd951f447d5718a511ed81f46fdf34d0fc Mon Sep 17 00:00:00 2001 From: Alex Tercete Date: Tue, 24 Sep 2024 21:34:14 +0100 Subject: [PATCH 4/6] feat(config_toolchain): accept state We were previously hardcoding the state in `//labgrid/run`. The state attribute passed to `labgrid_config_toolchain` will eventually translate into a `LG_STATE` env var. This is currently done in the executor, but perhaps it would be better to do it within a manager. --- e2e/docker/BUILD.bazel | 1 + labgrid/config/BUILD.bazel | 8 +++++++- labgrid/config/providers.bzl | 6 ++++++ labgrid/config/rule.bzl | 5 +++++ labgrid/config/state.bzl | 21 +++++++++++++++++++++ labgrid/config/toolchain/macro.bzl | 3 ++- labgrid/executor/BUILD.bazel | 2 ++ labgrid/executor/args.bzl | 16 ++++++++++++++-- labgrid/executor/executor.py | 6 ++++++ labgrid/run/run.py | 7 ++++++- 10 files changed, 70 insertions(+), 5 deletions(-) create mode 100644 labgrid/config/providers.bzl create mode 100644 labgrid/config/state.bzl diff --git a/e2e/docker/BUILD.bazel b/e2e/docker/BUILD.bazel index dd0cf11a..4c490900 100644 --- a/e2e/docker/BUILD.bazel +++ b/e2e/docker/BUILD.bazel @@ -26,6 +26,7 @@ labgrid_config_toolchain( name = "config", src = "local-ubuntu.16.04-gnu.yaml", manager = "@rules_labgrid//labgrid/manager:config", + state = "accessible", target_compatible_with = [ ":constraint", "@toolchain_utils//toolchain/constraint/os:linux", diff --git a/labgrid/config/BUILD.bazel b/labgrid/config/BUILD.bazel index 71e79285..a131e87a 100644 --- a/labgrid/config/BUILD.bazel +++ b/labgrid/config/BUILD.bazel @@ -1,5 +1,6 @@ -load(":src.bzl", "src") load(":deps.bzl", "deps") +load(":src.bzl", "src") +load(":state.bzl", "state") src( name = "src", @@ -12,6 +13,11 @@ alias( visibility = ["//visibility:public"], ) +state( + name = "state", + visibility = ["//visibility:public"], +) + deps( name = "deps", visibility = ["//visibility:public"], diff --git a/labgrid/config/providers.bzl b/labgrid/config/providers.bzl new file mode 100644 index 00000000..6b694d3a --- /dev/null +++ b/labgrid/config/providers.bzl @@ -0,0 +1,6 @@ +visibility("//...") + +StateInfo = provider( + doc = "A wrapper around the LabGrid state string value.", + fields = ["value"], +) diff --git a/labgrid/config/rule.bzl b/labgrid/config/rule.bzl index ced0a6f8..b78a34f5 100644 --- a/labgrid/config/rule.bzl +++ b/labgrid/config/rule.bzl @@ -9,6 +9,7 @@ Binds a LabGrid configuration file to the associated data required to setup, loa labgrid_config( name = "config", src = "docker.yaml", + state = "accessible", deps = ["@pip//:docker"], ) @@ -50,6 +51,9 @@ ATTRS = { doc = "The LabGrid configuration YAML.", allow_single_file = [".yml", ".yaml"], ), + "state": attr.string( + doc = "The state to transition the LabGrid strategy to.", + ), "deps": attr.label_list( doc = "LabGrid configuration Python dependencies.", providers = [[DefaultInfo, PyInfo]], @@ -67,6 +71,7 @@ def implementation(ctx): default = DefaultInfo(files = depset([ctx.file.src])) toolchain = platform_common.ToolchainInfo( src = ctx.file.src, + state = ctx.attr.state, deps = _flatten([_forward(d) for d in ctx.attr.deps]), ) diff --git a/labgrid/config/state.bzl b/labgrid/config/state.bzl new file mode 100644 index 00000000..29f75ea6 --- /dev/null +++ b/labgrid/config/state.bzl @@ -0,0 +1,21 @@ +load("//labgrid/cfg:unstore.bzl", _cfg = "unstore") +load(":providers.bzl", "StateInfo") + +visibility("//labgrid/config/...") + +DOC = "Unpacks the `state` from the `//labgrid/toolchain/config:type` toolchain" + +ATTRS = {} + +def implementation(ctx): + toolchain = ctx.toolchains["//labgrid/toolchain/config:type"] + return StateInfo(value = toolchain.state) + +state = rule( + doc = DOC, + attrs = ATTRS, + implementation = implementation, + provides = [StateInfo], + toolchains = ["//labgrid/toolchain/config:type"], + cfg = _cfg, +) diff --git a/labgrid/config/toolchain/macro.bzl b/labgrid/config/toolchain/macro.bzl index 1ad3692d..4b622152 100644 --- a/labgrid/config/toolchain/macro.bzl +++ b/labgrid/config/toolchain/macro.bzl @@ -3,11 +3,12 @@ load("@rules_labgrid//labgrid/manager:defs.bzl", "labgrid_manager") visibility("//...") -def labgrid_config_toolchain(*, name, src, manager, target_compatible_with, deps = ()): +def labgrid_config_toolchain(*, name, src, state, manager, target_compatible_with, deps = ()): labgrid_config( name = "{}-config".format(name), src = native.package_relative_label(src), deps = [native.package_relative_label(d) for d in deps], + state = state, ) labgrid_manager( diff --git a/labgrid/executor/BUILD.bazel b/labgrid/executor/BUILD.bazel index 25b3c110..992d1ac9 100644 --- a/labgrid/executor/BUILD.bazel +++ b/labgrid/executor/BUILD.bazel @@ -34,6 +34,8 @@ py_test( args = [ "--manager", "labgrid.executor.host:manager", + "--state", + "any", "echo", "$${DATA}", ], diff --git a/labgrid/executor/args.bzl b/labgrid/executor/args.bzl index 1d16093f..8cff4535 100644 --- a/labgrid/executor/args.bzl +++ b/labgrid/executor/args.bzl @@ -1,3 +1,5 @@ +load("//labgrid/config:providers.bzl", "StateInfo") + visibility("//labgrid/executor/...") DOC = "Creates an args file to be passed to `//labgrid/executor`" @@ -9,12 +11,22 @@ ATTRS = { default = "//labgrid/manager:src", cfg = "exec", ), + "_state": attr.label( + doc = "The state to transition the LabGrid strategy to", + default = "//labgrid/config:state", + cfg = "exec", + ), } def implementation(ctx): output = ctx.actions.declare_file("executor-args.txt") - content = "--manager={}".format(ctx.file._manager.short_path) - ctx.actions.write(output, content) + lines = [ + "--manager", + ctx.file._manager.short_path, + "--state", + ctx.attr._state[StateInfo].value, + ] + ctx.actions.write(output, "\n".join(lines)) return DefaultInfo(files = depset([output])) args = rule( diff --git a/labgrid/executor/executor.py b/labgrid/executor/executor.py index 8f69957b..c8d85823 100644 --- a/labgrid/executor/executor.py +++ b/labgrid/executor/executor.py @@ -53,6 +53,10 @@ def arguments(prsr: ArgumentParser) -> None: help="A manager to load from a Python module, of the form `module.to.load:Manager`.", type=load, ) + prsr.add_argument( + "--state", + help="The state to transition the LabGrid strategy to.", + ) def main(exe: Path, *args: str) -> int: @@ -80,6 +84,8 @@ def main(exe: Path, *args: str) -> int: with parsed.manager() as data: env = {k: v for k, v in data.env.items() if k != "RUNFILES_DIR"} + # TODO: Move this to a manager + env |= {"LG_STATE": parsed.state} try: run(cmd, env=env, check=True) except CalledProcessError as e: diff --git a/labgrid/run/run.py b/labgrid/run/run.py index 990284b5..f2f71055 100644 --- a/labgrid/run/run.py +++ b/labgrid/run/run.py @@ -30,6 +30,11 @@ def arguments(prsr: ArgumentParser) -> None: default=PurePath(environ["LG_ENV"]), type=PurePath, ) + prsr.add_argument( + "--state", + help="The state to transition the LabGrid strategy to", + default=environ["LG_STATE"], + ) def main(exe: Path, *args: str) -> int: @@ -45,7 +50,7 @@ def main(exe: Path, *args: str) -> int: env = Environment(str(parsed.config)) target = env.get_target() strategy = target.get_driver("DockerStrategy") - strategy.transition("accessible") + strategy.transition(parsed.state) # Retrieve the communication protocols shell = target.get_driver("CommandProtocol") -- GitLab From 5a954c11c2929bf436da77d7c454ac9cf195cec7 Mon Sep 17 00:00:00 2001 From: Alex Tercete Date: Tue, 24 Sep 2024 21:41:06 +0100 Subject: [PATCH 5/6] feat(run): use strategy from config By using the abstract `Strategy`, we find the one specified in the config file. This is also how `labgrid-client` works[1]. [1]: https://github.com/labgrid-project/labgrid/blob/6b541210c6063e97d81e257d2d6bda1444d9ec78/labgrid/remote/client.py#L800 --- labgrid/run/run.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/labgrid/run/run.py b/labgrid/run/run.py index f2f71055..44e157bb 100644 --- a/labgrid/run/run.py +++ b/labgrid/run/run.py @@ -49,7 +49,7 @@ def main(exe: Path, *args: str) -> int: # Start up Docker container with LabGrid env = Environment(str(parsed.config)) target = env.get_target() - strategy = target.get_driver("DockerStrategy") + strategy = target.get_driver("Strategy") strategy.transition(parsed.state) # Retrieve the communication protocols -- GitLab From 6ac2640d882feff85a3872c74d4cfd61a57afbc3 Mon Sep 17 00:00:00 2001 From: Alex Tercete Date: Tue, 1 Oct 2024 10:28:13 +0100 Subject: [PATCH 6/6] fix(executor): avoid setting empty LG_STATE --- labgrid/executor/executor.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/labgrid/executor/executor.py b/labgrid/executor/executor.py index c8d85823..ba2f1c93 100644 --- a/labgrid/executor/executor.py +++ b/labgrid/executor/executor.py @@ -85,7 +85,8 @@ def main(exe: Path, *args: str) -> int: with parsed.manager() as data: env = {k: v for k, v in data.env.items() if k != "RUNFILES_DIR"} # TODO: Move this to a manager - env |= {"LG_STATE": parsed.state} + if parsed.state is not None: + env |= {"LG_STATE": parsed.state} try: run(cmd, env=env, check=True) except CalledProcessError as e: -- GitLab