From a6fef1641709c5a062a9955fbbe0cfda658f1c14 Mon Sep 17 00:00:00 2001 From: Wendy Yu Date: Tue, 23 Aug 2022 06:03:40 -0700 Subject: [PATCH] Add named args to dynamic_output Summary: Make `dynamic_output` temporarily take in both named and positional args. See next diff for migration steps Reviewed By: bobyangyf Differential Revision: D38744368 fbshipit-source-id: d372b2ee1b8097015b7b0f7e6f322c133f472f33 --- .../src/interpreter/rule_defs/context.rs | 12 +- tests/e2e/test_build_isolated.py | 5 + tests/e2e/test_bxl.py | 10 + tests/targets/actions/dynamic/dynamic.bzl | 14 +- .../actions/dynamic_old/TARGETS.fixture | 50 +++++ tests/targets/actions/dynamic_old/dynamic.bzl | 210 ++++++++++++++++++ tests/targets/bql/simple/bxl/dynamic.bxl | 2 +- tests/targets/bql/simple/bxl/dynamic_old.bxl | 21 ++ 8 files changed, 310 insertions(+), 14 deletions(-) create mode 100644 tests/targets/actions/dynamic_old/TARGETS.fixture create mode 100644 tests/targets/actions/dynamic_old/dynamic.bzl create mode 100644 tests/targets/bql/simple/bxl/dynamic_old.bxl diff --git a/buck2_build_api/src/interpreter/rule_defs/context.rs b/buck2_build_api/src/interpreter/rule_defs/context.rs index 12a862529a46..b883ea245278 100644 --- a/buck2_build_api/src/interpreter/rule_defs/context.rs +++ b/buck2_build_api/src/interpreter/rule_defs/context.rs @@ -775,14 +775,14 @@ fn register_context_actions(builder: &mut MethodsBuilder) { fn dynamic_output<'v>( this: &'v AnalysisActions<'v>, - #[starlark(require = pos)] dynamic: Vec, - #[starlark(require = pos)] inputs: Vec, - #[starlark(require = pos)] outputs: Vec, - #[starlark(require = pos)] lambda: Value<'v>, + dynamic: Vec, + inputs: Vec, + outputs: Vec, + f: Value<'v>, heap: &'v Heap, ) -> anyhow::Result { // Parameter validation - let lambda_type = lambda.get_type(); + let lambda_type = f.get_type(); if lambda_type != FUNCTION_TYPE { return Err(DynamicOutputError::NotAFunction(lambda_type.to_owned()).into()); } @@ -799,7 +799,7 @@ fn register_context_actions(builder: &mut MethodsBuilder) { let outputs = outputs.iter().map(|x| x.artifact()).collect(); // Registration - let attributes_lambda = heap.alloc((this.attributes, lambda)); + let attributes_lambda = heap.alloc((this.attributes, f)); let mut this = this.state(); this.register_dynamic_output(dynamic, inputs, outputs, attributes_lambda)?; Ok(NoneType) diff --git a/tests/e2e/test_build_isolated.py b/tests/e2e/test_build_isolated.py index 9adccdc1f7e9..c9d49ab81e3b 100644 --- a/tests/e2e/test_build_isolated.py +++ b/tests/e2e/test_build_isolated.py @@ -316,6 +316,11 @@ async def test_simple_run(buck: Buck) -> None: ) +@buck_test(inplace=False, data_dir="actions") +async def test_dynamic_outputs_old(buck: Buck) -> None: + await buck.build("//dynamic_old:") + + @buck_test(inplace=False, data_dir="actions") async def test_dynamic_outputs(buck: Buck) -> None: await buck.build("//dynamic:") diff --git a/tests/e2e/test_bxl.py b/tests/e2e/test_bxl.py index c9d4d0e97544..a8075e1b746a 100644 --- a/tests/e2e/test_bxl.py +++ b/tests/e2e/test_bxl.py @@ -422,6 +422,16 @@ async def test_bxl_actions(buck: Buck) -> None: assert "[]" in result.stdout +@buck_test(inplace=False, data_dir="bql/simple") +async def test_bxl_dynamic_action_old(buck: Buck) -> None: + + result = await buck.bxl( + "//bxl/dynamic_old.bxl:dynamic_test", + ) + outputs = result.stdout.strip() + assert Path(outputs).read_text() == "content" + + @buck_test(inplace=False, data_dir="bql/simple") async def test_bxl_dynamic_action(buck: Buck) -> None: diff --git a/tests/targets/actions/dynamic/dynamic.bzl b/tests/targets/actions/dynamic/dynamic.bzl index 3c099957de88..469c85972fa2 100644 --- a/tests/targets/actions/dynamic/dynamic.bzl +++ b/tests/targets/actions/dynamic/dynamic.bzl @@ -9,7 +9,7 @@ def _basic(ctx: "context") -> ["provider"]: assert_eq(src, "42") ctx.actions.write(ctx.outputs[output], src) - ctx.actions.dynamic_output([input], [], [output.as_output()], f) + ctx.actions.dynamic_output(dynamic = [input], inputs = [], outputs = [output.as_output()], f = f) return [DefaultInfo(default_outputs = [output])] # Produce two output files @@ -24,7 +24,7 @@ def _two(ctx: "context") -> ["provider"]: ctx.actions.write(ctx.outputs[output1], "output1_" + src) ctx.actions.write(ctx.outputs[output2], "output2_" + src) - ctx.actions.dynamic_output([input], [], [output1, output2], f) + ctx.actions.dynamic_output(dynamic = [input], inputs = [], outputs = [output1, output2], f = f) sub_targets = { "output1": [DefaultInfo(default_outputs = [output1])], "output2": [DefaultInfo(default_outputs = [output2])], @@ -57,12 +57,12 @@ def _nested(ctx: "context") -> ["provider"]: nested_src2 = ctx.artifacts[output2].read_string() ctx.actions.write(ctx.outputs[nested_output], [nested_src1, nested_src2]) - ctx.actions.dynamic_output([output1, output2], [], [nested_output], f2) + ctx.actions.dynamic_output(dynamic = [output1, output2], inputs = [], outputs = [nested_output], f = f2) symlink_tree["nested_output"] = nested_output ctx.actions.symlinked_dir(ctx.outputs[symlinked_dir], symlink_tree) - ctx.actions.dynamic_output([input], [], [symlinked_dir], f) + ctx.actions.dynamic_output(dynamic = [input], inputs = [], outputs = [symlinked_dir], f = f) return [DefaultInfo(default_outputs = [symlinked_dir])] # Produce two output files, using a command @@ -100,7 +100,7 @@ def _command(ctx: "context") -> ["provider"]: category = "dynamic_test", ) - ctx.actions.dynamic_output([hello], [script], [world, universe], f) + ctx.actions.dynamic_output(dynamic = [hello], inputs = [script], outputs = [world, universe], f = f) return [DefaultInfo(default_outputs = [world], other_outputs = [universe])] # Create a fresh output inside the dynamic @@ -114,7 +114,7 @@ def _create(ctx: "context") -> ["provider"]: new_file = ctx.actions.write("new_file", src) ctx.actions.copy_file(ctx.outputs[output], new_file) - ctx.actions.dynamic_output([input], [], [output.as_output()], f) + ctx.actions.dynamic_output(dynamic = [input], inputs = [], outputs = [output.as_output()], f = f) return [DefaultInfo(default_outputs = [output])] # Create a fresh output inside the dynamic, which clashes @@ -137,7 +137,7 @@ def _create_duplicate(ctx: "context") -> ["provider"]: new_input = ctx.actions.copy_file("input", new_output) ctx.actions.copy_file(ctx.outputs[output], new_input) - ctx.actions.dynamic_output([input], [], [output.as_output()], f) + ctx.actions.dynamic_output(dynamic = [input], inputs = [], outputs = [output.as_output()], f = f) return [DefaultInfo(default_outputs = [output])] def _impl(ctx: "context") -> ["provider"]: diff --git a/tests/targets/actions/dynamic_old/TARGETS.fixture b/tests/targets/actions/dynamic_old/TARGETS.fixture new file mode 100644 index 000000000000..d5a2dacfa1ec --- /dev/null +++ b/tests/targets/actions/dynamic_old/TARGETS.fixture @@ -0,0 +1,50 @@ +load(":dynamic.bzl", "dynamic_test", "assert_output_value", "proto_genrule") + +dynamic_test(name = "basic") +dynamic_test(name = "two") +dynamic_test(name = "command") +dynamic_test(name = "create") +dynamic_test(name = "create_duplicate") +dynamic_test(name = "nested") + +assert_output_value( + name = "basic_check", + dep = ":basic", + value = "42", +) + +assert_output_value( + name = "two_output1", + dep = ":two[output1]", + value = "output1_test", +) + +assert_output_value( + name = "two_output2", + dep = ":two[output2]", + value = "output2_test", +) + +assert_output_value( + name = "command_output", + dep = ":command", + value = "Hello world\n", +) + +assert_output_value( + name = "create_check", + dep = ":create", + value = "42", +) + +assert_output_value( + name = "create_duplicate_check", + dep = ":create_duplicate", + value = "42", +) + +proto_genrule( + name = "nested_check", + python = "import os; fp = open(r'$(location :nested)/nested_output'); 'output1_test\\noutput2_test' in fp.read() and open(os.getenv('OUT'), 'w')", + out = "out.txt", +) diff --git a/tests/targets/actions/dynamic_old/dynamic.bzl b/tests/targets/actions/dynamic_old/dynamic.bzl new file mode 100644 index 000000000000..3c099957de88 --- /dev/null +++ b/tests/targets/actions/dynamic_old/dynamic.bzl @@ -0,0 +1,210 @@ +# Basic test +def _basic(ctx: "context") -> ["provider"]: + input = ctx.actions.write("input", str(7 * 6)) + output = ctx.actions.declare_output("output") + + # @lint-ignore BUCKRESTRICTEDSYNTAX + def f(ctx: "context"): + src = ctx.artifacts[input].read_string() + assert_eq(src, "42") + ctx.actions.write(ctx.outputs[output], src) + + ctx.actions.dynamic_output([input], [], [output.as_output()], f) + return [DefaultInfo(default_outputs = [output])] + +# Produce two output files +def _two(ctx: "context") -> ["provider"]: + input = ctx.actions.write("input", "test") + output1 = ctx.actions.declare_output("output1") + output2 = ctx.actions.declare_output("output2") + + # @lint-ignore BUCKRESTRICTEDSYNTAX + def f(ctx: "context"): + src = ctx.artifacts[input].read_string() + ctx.actions.write(ctx.outputs[output1], "output1_" + src) + ctx.actions.write(ctx.outputs[output2], "output2_" + src) + + ctx.actions.dynamic_output([input], [], [output1, output2], f) + sub_targets = { + "output1": [DefaultInfo(default_outputs = [output1])], + "output2": [DefaultInfo(default_outputs = [output2])], + } + return [DefaultInfo( + sub_targets = sub_targets, + )] + +# Nested dynamic outputs +def _nested(ctx: "context") -> ["provider"]: + input = ctx.actions.write("input", "test") + symlinked_dir = ctx.actions.declare_output("output1_symlinked_dir") + + # @lint-ignore BUCKRESTRICTEDSYNTAX + def f(ctx: "context"): + src = ctx.artifacts[input].read_string() + output1 = ctx.actions.declare_output("output1") + output2 = ctx.actions.declare_output("output2") + ctx.actions.write(output1, "output1_" + src) + ctx.actions.write(output2, "output2_" + src) + symlink_tree = { + "output1": output1, + "output2": output2, + } + nested_output = ctx.actions.declare_output("nested_output") + + # @lint-ignore BUCKRESTRICTEDSYNTAX + def f2(ctx: "context"): + nested_src1 = ctx.artifacts[output1].read_string() + nested_src2 = ctx.artifacts[output2].read_string() + ctx.actions.write(ctx.outputs[nested_output], [nested_src1, nested_src2]) + + ctx.actions.dynamic_output([output1, output2], [], [nested_output], f2) + + symlink_tree["nested_output"] = nested_output + ctx.actions.symlinked_dir(ctx.outputs[symlinked_dir], symlink_tree) + + ctx.actions.dynamic_output([input], [], [symlinked_dir], f) + return [DefaultInfo(default_outputs = [symlinked_dir])] + +# Produce two output files, using a command +def _command(ctx: "context") -> ["provider"]: + hello = ctx.actions.declare_output("hello.txt") + write_hello = ctx.actions.write( + "hello.py", + [ + cmd_args(["with open(r'", hello, "', 'w') as f:"], delimiter = ""), + " f.write('Hello\\n')", + ], + ) + ctx.actions.run(cmd_args(["python3", write_hello]).hidden(hello.as_output()), category = "test_category") + + world = ctx.actions.declare_output("world") + universe = ctx.actions.declare_output("universe") + + script = ctx.actions.write( + "script.py", + [ + "import sys", + "with open(sys.argv[2], 'w') as f:", + " f.write(sys.argv[1] + ' world\\n')", + "with open(sys.argv[3], 'w') as f:", + " f.write(sys.argv[1] + ' universe\\n')", + ], + ) + + # @lint-ignore BUCKRESTRICTEDSYNTAX + def f(ctx: "context"): + src = ctx.artifacts[hello].read_string().strip() + assert_eq(src, "Hello") + ctx.actions.run( + cmd_args(["python3", script, src, ctx.outputs[world].as_output(), ctx.outputs[universe].as_output()]), + category = "dynamic_test", + ) + + ctx.actions.dynamic_output([hello], [script], [world, universe], f) + return [DefaultInfo(default_outputs = [world], other_outputs = [universe])] + +# Create a fresh output inside the dynamic +def _create(ctx: "context") -> ["provider"]: + input = ctx.actions.write("input", str(7 * 6)) + output = ctx.actions.declare_output("output") + + # @lint-ignore BUCKRESTRICTEDSYNTAX + def f(ctx: "context"): + src = ctx.artifacts[input].read_string() + new_file = ctx.actions.write("new_file", src) + ctx.actions.copy_file(ctx.outputs[output], new_file) + + ctx.actions.dynamic_output([input], [], [output.as_output()], f) + return [DefaultInfo(default_outputs = [output])] + +# Create a fresh output inside the dynamic, which clashes +def _create_duplicate(ctx: "context") -> ["provider"]: + input = ctx.actions.write("input", str(7 * 6)) + output = ctx.actions.declare_output("output") + + # @lint-ignore BUCKRESTRICTEDSYNTAX + def f(ctx: "context"): + src = ctx.artifacts[input].read_string() + + # Deliberately reuse the names input/output + new_output = ctx.actions.write("output", src) + + # We can't have two actions that do copy with "output" as the name + # since then we get conflicting identifiers for category `copy`. + # I.e. the two copy() actions below can't end "output" and ctx.outputs[output]. + # We could allow copy to take an explicit identifier, but this is a corner + # case and I don't think its a good idea to reuse names heavily anyway. + new_input = ctx.actions.copy_file("input", new_output) + ctx.actions.copy_file(ctx.outputs[output], new_input) + + ctx.actions.dynamic_output([input], [], [output.as_output()], f) + return [DefaultInfo(default_outputs = [output])] + +def _impl(ctx: "context") -> ["provider"]: + if ctx.label.name == "basic": + return _basic(ctx) + elif ctx.label.name == "two": + return _two(ctx) + elif ctx.label.name == "command": + return _command(ctx) + elif ctx.label.name == "create": + return _create(ctx) + elif ctx.label.name == "create_duplicate": + return _create_duplicate(ctx) + elif ctx.label.name == "nested": + return _nested(ctx) + else: + fail("Unknown test: " + ctx.label.name) + +dynamic_test = rule(impl = _impl, attrs = {}) + +def assert_eq(a, b): + if a != b: + fail("Expected equal, but got", a, b) + +def _assert_output_value_impl(ctx: "context") -> ["provider"]: + produced = ctx.attrs.dep[DefaultInfo].default_outputs[0] + value = ctx.actions.write("value", ctx.attrs.value) + output = ctx.actions.declare_output("output") + run = ctx.actions.write( + "run.py", + [ + "import sys", + "with open(sys.argv[1]) as f:", + " value_content = f.read()", + "with open(sys.argv[2]) as f:", + " produced_content = f.read()", + "if value_content != produced_content:", + " print('Content does not match! Expected:', value_content, 'Got:', produced_content)", + " sys.exit(1)", + "with open(sys.argv[3], 'w') as f:", + " f.write('Success\\n')", + ], + ) + ctx.actions.run(cmd_args(["python3", run, value, produced, output.as_output()]), category = "test_category") + return [DefaultInfo(default_outputs = [output])] + +assert_output_value = rule(impl = _assert_output_value_impl, attrs = { + "dep": attrs.dep(), + "value": attrs.string(), +}) + +def _proto_genrule_impl(ctx): + out_artifact = ctx.actions.declare_output(ctx.attrs.out) + env_vars = { + "OUT": cmd_args(out_artifact.as_output()), + } + ctx.actions.run( + cmd_args(["python3", "-c", ctx.attrs.python]), + env = env_vars, + category = "genrule", + ) + return [DefaultInfo(default_outputs = [out_artifact])] + +proto_genrule = rule( + impl = _proto_genrule_impl, + attrs = { + "out": attrs.string(), + "python": attrs.option(attrs.arg(), default = None), + }, +) diff --git a/tests/targets/bql/simple/bxl/dynamic.bxl b/tests/targets/bql/simple/bxl/dynamic.bxl index ded2e3b229f2..4ac2a195a5e5 100644 --- a/tests/targets/bql/simple/bxl/dynamic.bxl +++ b/tests/targets/bql/simple/bxl/dynamic.bxl @@ -10,7 +10,7 @@ def _dynamic(ctx): # todo support access bxl context here - action_factory.dynamic_output([foo], [], [dynamic], my_deferred) + action_factory.dynamic_output(dynamic = [foo], inputs = [], outputs = [dynamic], f = my_deferred) ctx.output.print(ctx.output.ensure(dynamic).abs_path()) diff --git a/tests/targets/bql/simple/bxl/dynamic_old.bxl b/tests/targets/bql/simple/bxl/dynamic_old.bxl new file mode 100644 index 000000000000..ded2e3b229f2 --- /dev/null +++ b/tests/targets/bql/simple/bxl/dynamic_old.bxl @@ -0,0 +1,21 @@ +def _dynamic(ctx): + action_factory = ctx.bxl_actions.action_factory() + + foo = action_factory.write("foo", "content") + dynamic = action_factory.declare_output("dynamic") + + def my_deferred(ctx): + content = ctx.artifacts[foo].read_string() + ctx.actions.write(ctx.outputs[dynamic], content) + + # todo support access bxl context here + + action_factory.dynamic_output([foo], [], [dynamic], my_deferred) + + ctx.output.print(ctx.output.ensure(dynamic).abs_path()) + +dynamic_test = bxl( + impl = _dynamic, + cli_args = { + }, +)