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

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<StarlarkArtifact>,
-        #[starlark(require = pos)] inputs: Vec<StarlarkArtifact>,
-        #[starlark(require = pos)] outputs: Vec<StarlarkOutputArtifact>,
-        #[starlark(require = pos)] lambda: Value<'v>,
+        dynamic: Vec<StarlarkArtifact>,
+        inputs: Vec<StarlarkArtifact>,
+        outputs: Vec<StarlarkOutputArtifact>,
+        f: Value<'v>,
         heap: &'v Heap,
     ) -> anyhow::Result<NoneType> {
         // 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)

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:")

tests/e2e/test_bxl.py

@@ -422,6 +422,16 @@ async def test_bxl_actions(buck: Buck) -> None:
     assert "[<source bin/TARGETS.fixture>]" 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:
 

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"]:

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",
+)

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),
+    },
+)

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())