refactor: expose base rule construction via builders to allow customization for testing (#2600)

The py_reconfig rules work by wrapping: The outer reconfig rule applies
a transition, depends
on an inner py base rule, then jumps through various hoops to ensure it
looks and acts
like the target it's wrapping.

This is error prone, incomplete, and annoying code to maintain. Phil
recently discovered it wasn't properly propagating the output group, so
he had to add that. I wasted time trying to fix a bug I _thought_ was in
it, but actually was working correctly. The logic within it is a bit
hacky as it tries to emulate some of the platform-specific stuff for
windows. Every time
py_reconfig gains something to transition on, there's numerous places to
define, propagate,
and extract the pieces necessary to do it.

To fix this, make the py_reconfig rules not wrap an inner base py rule.
Instead, they use the same underlying rule args that the base rules do.
This lets them act directly as the rule they're designed to test.

Customization is done by capturing all the rule args in builder objects.
The py_reconfig code constructs the same builder the base rules do, then
modifies it as necessary (adding attributes, wrapping the base
transition function). As a bonus, this sets some ground work to allow
more easily defining derivative rules without having to copy/paste
arbitrary parts of how the base rules are defined.

Work towards #1647

Author: rickeylev

python/private/builders.bzl

@@ -96,6 +96,145 @@ def _DepsetBuilder_build(self):
             kwargs["order"] = self._order[0]
         return depset(direct = self.direct, transitive = self.transitive, **kwargs)
 
+def _Optional(*initial):
+    """A wrapper for a re-assignable value that may or may not be set.
+
+    This allows structs to have attributes that aren't inherently mutable
+    and must be re-assigned to have their value updated.
+
+    Args:
+        *initial: A single vararg to be the initial value, or no args
+            to leave it unset.
+
+    Returns:
+        {type}`Optional`
+    """
+    if len(initial) > 1:
+        fail("Only zero or one positional arg allowed")
+
+    # buildifier: disable=uninitialized
+    self = struct(
+        _value = list(initial),
+        present = lambda *a, **k: _Optional_present(self, *a, **k),
+        set = lambda *a, **k: _Optional_set(self, *a, **k),
+        get = lambda *a, **k: _Optional_get(self, *a, **k),
+    )
+    return self
+
+def _Optional_set(self, value):
+    """Sets the value of the optional.
+
+    Args:
+        self: implicitly added
+        value: the value to set.
+    """
+    if len(self._value) == 0:
+        self._value.append(value)
+    else:
+        self._value[0] = value
+
+def _Optional_get(self):
+    """Gets the value of the optional, or error.
+
+    Args:
+        self: implicitly added
+
+    Returns:
+        The stored value, or error if not set.
+    """
+    if not len(self._value):
+        fail("Value not present")
+    return self._value[0]
+
+def _Optional_present(self):
+    """Tells if a value is present.
+
+    Args:
+        self: implicitly added
+
+    Returns:
+        {type}`bool` True if the value is set, False if not.
+    """
+    return len(self._value) > 0
+
+def _RuleBuilder(implementation = None, **kwargs):
+    """Builder for creating rules.
+
+    Args:
+        implementation: {type}`callable` The rule implementation function.
+        **kwargs: The same as the `rule()` function, but using builders
+            for the non-mutable Bazel objects.
+    """
+
+    # buildifier: disable=uninitialized
+    self = struct(
+        attrs = dict(kwargs.pop("attrs", None) or {}),
+        cfg = kwargs.pop("cfg", None) or _TransitionBuilder(),
+        exec_groups = dict(kwargs.pop("exec_groups", None) or {}),
+        executable = _Optional(),
+        fragments = list(kwargs.pop("fragments", None) or []),
+        implementation = _Optional(implementation),
+        extra_kwargs = kwargs,
+        provides = list(kwargs.pop("provides", None) or []),
+        test = _Optional(),
+        toolchains = list(kwargs.pop("toolchains", None) or []),
+        build = lambda *a, **k: _RuleBuilder_build(self, *a, **k),
+        to_kwargs = lambda *a, **k: _RuleBuilder_to_kwargs(self, *a, **k),
+    )
+    if "test" in kwargs:
+        self.test.set(kwargs.pop("test"))
+    if "executable" in kwargs:
+        self.executable.set(kwargs.pop("executable"))
+    return self
+
+def _RuleBuilder_build(self, debug = ""):
+    """Builds a `rule` object
+
+    Args:
+        self: implicitly added
+        debug: {type}`str` If set, prints the args used to create the rule.
+
+    Returns:
+        {type}`rule`
+    """
+    kwargs = self.to_kwargs()
+    if debug:
+        lines = ["=" * 80, "rule kwargs: {}:".format(debug)]
+        for k, v in sorted(kwargs.items()):
+            lines.append("  {}={}".format(k, v))
+        print("\n".join(lines))  # buildifier: disable=print
+    return rule(**kwargs)
+
+def _RuleBuilder_to_kwargs(self):
+    """Builds the arguments for calling `rule()`.
+
+    Args:
+        self: implicitly added
+
+    Returns:
+        {type}`dict`
+    """
+    kwargs = {}
+    if self.executable.present():
+        kwargs["executable"] = self.executable.get()
+    if self.test.present():
+        kwargs["test"] = self.test.get()
+
+    kwargs.update(
+        implementation = self.implementation.get(),
+        cfg = self.cfg.build() if self.cfg.implementation.present() else None,
+        attrs = {
+            k: (v.build() if hasattr(v, "build") else v)
+            for k, v in self.attrs.items()
+        },
+        exec_groups = self.exec_groups,
+        fragments = self.fragments,
+        provides = self.provides,
+        toolchains = self.toolchains,
+    )
+    kwargs.update(self.extra_kwargs)
+    return kwargs
+
 def _RunfilesBuilder():
     """Creates a `RunfilesBuilder`.
 
@@ -177,6 +316,91 @@ def _RunfilesBuilder_build(self, ctx, **kwargs):
         **kwargs
     ).merge_all(self.runfiles)
 
+def _SetBuilder(initial = None):
+    """Builder for list of unique values.
+
+    Args:
+        initial: {type}`list | None` The initial values.
+
+    Returns:
+        {type}`SetBuilder`
+    """
+    initial = {} if not initial else {v: None for v in initial}
+
+    # buildifier: disable=uninitialized
+    self = struct(
+        # TODO - Switch this to use set() builtin when available
+        # https://bazel.build/rules/lib/core/set
+        _values = initial,
+        update = lambda *a, **k: _SetBuilder_update(self, *a, **k),
+        build = lambda *a, **k: _SetBuilder_build(self, *a, **k),
+    )
+    return self
+
+def _SetBuilder_build(self):
+    """Builds the values into a list
+
+    Returns:
+        {type}`list`
+    """
+    return self._values.keys()
+
+def _SetBuilder_update(self, *others):
+    """Adds values to the builder.
+
+    Args:
+        self: implicitly added
+        *others: {type}`list` values to add to the set.
+    """
+    for other in others:
+        for value in other:
+            if value not in self._values:
+                self._values[value] = None
+
+def _TransitionBuilder(implementation = None, inputs = None, outputs = None, **kwargs):
+    """Builder for transition objects.
+
+    Args:
+        implementation: {type}`callable` the transition implementation function.
+        inputs: {type}`list[str]` the inputs for the transition.
+        outputs: {type}`list[str]` the outputs of the transition.
+        **kwargs: Extra keyword args to use when building.
+
+    Returns:
+        {type}`TransitionBuilder`
+    """
+
+    # buildifier: disable=uninitialized
+    self = struct(
+        implementation = _Optional(implementation),
+        # Bazel requires transition.inputs to have unique values, so use set
+        # semantics so extenders of a transition can easily add/remove values.
+        # TODO - Use set builtin instead of custom builder, when available.
+        # https://bazel.build/rules/lib/core/set
+        inputs = _SetBuilder(inputs),
+        # Bazel requires transition.inputs to have unique values, so use set
+        # semantics so extenders of a transition can easily add/remove values.
+        # TODO - Use set builtin instead of custom builder, when available.
+        # https://bazel.build/rules/lib/core/set
+        outputs = _SetBuilder(outputs),
+        extra_kwargs = kwargs,
+        build = lambda *a, **k: _TransitionBuilder_build(self, *a, **k),
+    )
+    return self
+
+def _TransitionBuilder_build(self):
+    """Creates a transition from the builder.
+
+    Returns:
+        {type}`transition`
+    """
+    return transition(
+        implementation = self.implementation.get(),
+        inputs = self.inputs.build(),
+        outputs = self.outputs.build(),
+        **self.extra_kwargs
+    )
+
 # Skylib's types module doesn't have is_file, so roll our own
 def _is_file(value):
     return type(value) == "File"
@@ -187,4 +411,8 @@ def _is_runfiles(value):
 builders = struct(
     DepsetBuilder = _DepsetBuilder,
     RunfilesBuilder = _RunfilesBuilder,
+    RuleBuilder = _RuleBuilder,
+    TransitionBuilder = _TransitionBuilder,
+    SetBuilder = _SetBuilder,
+    Optional = _Optional,
 )

python/private/py_binary_macro.bzl

@@ -17,5 +17,8 @@ load(":py_binary_rule.bzl", py_binary_rule = "py_binary")
 load(":py_executable.bzl", "convert_legacy_create_init_to_int")
 
 def py_binary(**kwargs):
+    py_binary_macro(py_binary_rule, **kwargs)
+
+def py_binary_macro(py_rule, **kwargs):
     convert_legacy_create_init_to_int(kwargs)
-    py_binary_rule(**kwargs)
+    py_rule(**kwargs)

python/private/py_binary_rule.bzl

@@ -13,15 +13,14 @@
 # limitations under the License.
 """Rule implementation of py_binary for Bazel."""
 
-load("@bazel_skylib//lib:dicts.bzl", "dicts")
 load(":attributes.bzl", "AGNOSTIC_BINARY_ATTRS")
 load(
     ":py_executable.bzl",
-    "create_executable_rule",
+    "create_executable_rule_builder",
     "py_executable_impl",
 )
 
-_PY_TEST_ATTRS = {
+_COVERAGE_ATTRS = {
     # Magic attribute to help C++ coverage work. There's no
     # docs about this; see TestActionBuilder.java
     "_collect_cc_coverage": attr.label(
@@ -45,8 +44,13 @@ def _py_binary_impl(ctx):
         inherited_environment = [],
     )
 
-py_binary = create_executable_rule(
-    implementation = _py_binary_impl,
-    attrs = dicts.add(AGNOSTIC_BINARY_ATTRS, _PY_TEST_ATTRS),
-    executable = True,
-)
+def create_binary_rule_builder():
+    builder = create_executable_rule_builder(
+        implementation = _py_binary_impl,
+        executable = True,
+    )
+    builder.attrs.update(AGNOSTIC_BINARY_ATTRS)
+    builder.attrs.update(_COVERAGE_ATTRS)
+    return builder
+
+py_binary = create_binary_rule_builder().build()

python/private/py_executable.bzl

@@ -1747,50 +1747,40 @@ def _transition_executable_impl(input_settings, attr):
         settings[_PYTHON_VERSION_FLAG] = attr.python_version
     return settings
 
-_transition_executable = transition(
-    implementation = _transition_executable_impl,
-    inputs = [
-        _PYTHON_VERSION_FLAG,
-    ],
-    outputs = [
-        _PYTHON_VERSION_FLAG,
-    ],
-)
-
 def create_executable_rule(*, attrs, **kwargs):
     return create_base_executable_rule(
         attrs = attrs,
         fragments = ["py", "bazel_py"],
         **kwargs
     )
 
-def create_base_executable_rule(*, attrs, fragments = [], **kwargs):
+def create_base_executable_rule():
     """Create a function for defining for Python binary/test targets.
 
-    Args:
-        attrs: Rule attributes
-        fragments: List of str; extra config fragments that are required.
-        **kwargs: Additional args to pass onto `rule()`
-
     Returns:
         A rule function
     """
-    if "py" not in fragments:
-        # The list might be frozen, so use concatentation
-        fragments = fragments + ["py"]
-    kwargs.setdefault("provides", []).append(PyExecutableInfo)
-    kwargs["exec_groups"] = REQUIRED_EXEC_GROUPS | (kwargs.get("exec_groups") or {})
-    kwargs.setdefault("cfg", _transition_executable)
-    return rule(
-        # TODO: add ability to remove attrs, i.e. for imports attr
-        attrs = dicts.add(EXECUTABLE_ATTRS, attrs),
+    return create_executable_rule_builder().build()
+
+def create_executable_rule_builder(implementation, **kwargs):
+    builder = builders.RuleBuilder(
+        implementation = implementation,
+        attrs = EXECUTABLE_ATTRS,
+        exec_groups = REQUIRED_EXEC_GROUPS,
+        fragments = ["py", "bazel_py"],
+        provides = [PyExecutableInfo],
         toolchains = [
             TOOLCHAIN_TYPE,
             config_common.toolchain_type(EXEC_TOOLS_TOOLCHAIN_TYPE, mandatory = False),
         ] + _CC_TOOLCHAINS,
-        fragments = fragments,
+        cfg = builders.TransitionBuilder(
+            implementation = _transition_executable_impl,
+            inputs = [_PYTHON_VERSION_FLAG],
+            outputs = [_PYTHON_VERSION_FLAG],
+        ),
         **kwargs
     )
+    return builder
 
 def cc_configure_features(
         ctx,

python/private/py_test_macro.bzl

@@ -17,5 +17,8 @@ load(":py_executable.bzl", "convert_legacy_create_init_to_int")
 load(":py_test_rule.bzl", py_test_rule = "py_test")
 
 def py_test(**kwargs):
+    py_test_macro(py_test_rule, **kwargs)
+
+def py_test_macro(py_rule, **kwargs):
     convert_legacy_create_init_to_int(kwargs)
-    py_test_rule(**kwargs)
+    py_rule(**kwargs)