Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 81c6798

Browse filesBrowse files
rickeylevrickeylev
authored
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
1 parent 428c1bb commit 81c6798
Copy full SHA for 81c6798

File tree

Expand file treeCollapse file tree

8 files changed

+330
-190
lines changed
Filter options
Expand file treeCollapse file tree

8 files changed

+330
-190
lines changed

‎python/private/builders.bzl

Copy file name to clipboardExpand all lines: python/private/builders.bzl
+228Lines changed: 228 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,145 @@ def _DepsetBuilder_build(self):
9696
kwargs["order"] = self._order[0]
9797
return depset(direct = self.direct, transitive = self.transitive, **kwargs)
9898

99+
def _Optional(*initial):
100+
"""A wrapper for a re-assignable value that may or may not be set.
101+
102+
This allows structs to have attributes that aren't inherently mutable
103+
and must be re-assigned to have their value updated.
104+
105+
Args:
106+
*initial: A single vararg to be the initial value, or no args
107+
to leave it unset.
108+
109+
Returns:
110+
{type}`Optional`
111+
"""
112+
if len(initial) > 1:
113+
fail("Only zero or one positional arg allowed")
114+
115+
# buildifier: disable=uninitialized
116+
self = struct(
117+
_value = list(initial),
118+
present = lambda *a, **k: _Optional_present(self, *a, **k),
119+
set = lambda *a, **k: _Optional_set(self, *a, **k),
120+
get = lambda *a, **k: _Optional_get(self, *a, **k),
121+
)
122+
return self
123+
124+
def _Optional_set(self, value):
125+
"""Sets the value of the optional.
126+
127+
Args:
128+
self: implicitly added
129+
value: the value to set.
130+
"""
131+
if len(self._value) == 0:
132+
self._value.append(value)
133+
else:
134+
self._value[0] = value
135+
136+
def _Optional_get(self):
137+
"""Gets the value of the optional, or error.
138+
139+
Args:
140+
self: implicitly added
141+
142+
Returns:
143+
The stored value, or error if not set.
144+
"""
145+
if not len(self._value):
146+
fail("Value not present")
147+
return self._value[0]
148+
149+
def _Optional_present(self):
150+
"""Tells if a value is present.
151+
152+
Args:
153+
self: implicitly added
154+
155+
Returns:
156+
{type}`bool` True if the value is set, False if not.
157+
"""
158+
return len(self._value) > 0
159+
160+
def _RuleBuilder(implementation = None, **kwargs):
161+
"""Builder for creating rules.
162+
163+
Args:
164+
implementation: {type}`callable` The rule implementation function.
165+
**kwargs: The same as the `rule()` function, but using builders
166+
for the non-mutable Bazel objects.
167+
"""
168+
169+
# buildifier: disable=uninitialized
170+
self = struct(
171+
attrs = dict(kwargs.pop("attrs", None) or {}),
172+
cfg = kwargs.pop("cfg", None) or _TransitionBuilder(),
173+
exec_groups = dict(kwargs.pop("exec_groups", None) or {}),
174+
executable = _Optional(),
175+
fragments = list(kwargs.pop("fragments", None) or []),
176+
implementation = _Optional(implementation),
177+
extra_kwargs = kwargs,
178+
provides = list(kwargs.pop("provides", None) or []),
179+
test = _Optional(),
180+
toolchains = list(kwargs.pop("toolchains", None) or []),
181+
build = lambda *a, **k: _RuleBuilder_build(self, *a, **k),
182+
to_kwargs = lambda *a, **k: _RuleBuilder_to_kwargs(self, *a, **k),
183+
)
184+
if "test" in kwargs:
185+
self.test.set(kwargs.pop("test"))
186+
if "executable" in kwargs:
187+
self.executable.set(kwargs.pop("executable"))
188+
return self
189+
190+
def _RuleBuilder_build(self, debug = ""):
191+
"""Builds a `rule` object
192+
193+
Args:
194+
self: implicitly added
195+
debug: {type}`str` If set, prints the args used to create the rule.
196+
197+
Returns:
198+
{type}`rule`
199+
"""
200+
kwargs = self.to_kwargs()
201+
if debug:
202+
lines = ["=" * 80, "rule kwargs: {}:".format(debug)]
203+
for k, v in sorted(kwargs.items()):
204+
lines.append(" {}={}".format(k, v))
205+
print("\n".join(lines)) # buildifier: disable=print
206+
return rule(**kwargs)
207+
208+
def _RuleBuilder_to_kwargs(self):
209+
"""Builds the arguments for calling `rule()`.
210+
211+
Args:
212+
self: implicitly added
213+
214+
Returns:
215+
{type}`dict`
216+
"""
217+
kwargs = {}
218+
if self.executable.present():
219+
kwargs["executable"] = self.executable.get()
220+
if self.test.present():
221+
kwargs["test"] = self.test.get()
222+
223+
kwargs.update(
224+
implementation = self.implementation.get(),
225+
cfg = self.cfg.build() if self.cfg.implementation.present() else None,
226+
attrs = {
227+
k: (v.build() if hasattr(v, "build") else v)
228+
for k, v in self.attrs.items()
229+
},
230+
exec_groups = self.exec_groups,
231+
fragments = self.fragments,
232+
provides = self.provides,
233+
toolchains = self.toolchains,
234+
)
235+
kwargs.update(self.extra_kwargs)
236+
return kwargs
237+
99238
def _RunfilesBuilder():
100239
"""Creates a `RunfilesBuilder`.
101240
@@ -177,6 +316,91 @@ def _RunfilesBuilder_build(self, ctx, **kwargs):
177316
**kwargs
178317
).merge_all(self.runfiles)
179318

319+
def _SetBuilder(initial = None):
320+
"""Builder for list of unique values.
321+
322+
Args:
323+
initial: {type}`list | None` The initial values.
324+
325+
Returns:
326+
{type}`SetBuilder`
327+
"""
328+
initial = {} if not initial else {v: None for v in initial}
329+
330+
# buildifier: disable=uninitialized
331+
self = struct(
332+
# TODO - Switch this to use set() builtin when available
333+
# https://bazel.build/rules/lib/core/set
334+
_values = initial,
335+
update = lambda *a, **k: _SetBuilder_update(self, *a, **k),
336+
build = lambda *a, **k: _SetBuilder_build(self, *a, **k),
337+
)
338+
return self
339+
340+
def _SetBuilder_build(self):
341+
"""Builds the values into a list
342+
343+
Returns:
344+
{type}`list`
345+
"""
346+
return self._values.keys()
347+
348+
def _SetBuilder_update(self, *others):
349+
"""Adds values to the builder.
350+
351+
Args:
352+
self: implicitly added
353+
*others: {type}`list` values to add to the set.
354+
"""
355+
for other in others:
356+
for value in other:
357+
if value not in self._values:
358+
self._values[value] = None
359+
360+
def _TransitionBuilder(implementation = None, inputs = None, outputs = None, **kwargs):
361+
"""Builder for transition objects.
362+
363+
Args:
364+
implementation: {type}`callable` the transition implementation function.
365+
inputs: {type}`list[str]` the inputs for the transition.
366+
outputs: {type}`list[str]` the outputs of the transition.
367+
**kwargs: Extra keyword args to use when building.
368+
369+
Returns:
370+
{type}`TransitionBuilder`
371+
"""
372+
373+
# buildifier: disable=uninitialized
374+
self = struct(
375+
implementation = _Optional(implementation),
376+
# Bazel requires transition.inputs to have unique values, so use set
377+
# semantics so extenders of a transition can easily add/remove values.
378+
# TODO - Use set builtin instead of custom builder, when available.
379+
# https://bazel.build/rules/lib/core/set
380+
inputs = _SetBuilder(inputs),
381+
# Bazel requires transition.inputs to have unique values, so use set
382+
# semantics so extenders of a transition can easily add/remove values.
383+
# TODO - Use set builtin instead of custom builder, when available.
384+
# https://bazel.build/rules/lib/core/set
385+
outputs = _SetBuilder(outputs),
386+
extra_kwargs = kwargs,
387+
build = lambda *a, **k: _TransitionBuilder_build(self, *a, **k),
388+
)
389+
return self
390+
391+
def _TransitionBuilder_build(self):
392+
"""Creates a transition from the builder.
393+
394+
Returns:
395+
{type}`transition`
396+
"""
397+
return transition(
398+
implementation = self.implementation.get(),
399+
inputs = self.inputs.build(),
400+
outputs = self.outputs.build(),
401+
**self.extra_kwargs
402+
)
403+
180404
# Skylib's types module doesn't have is_file, so roll our own
181405
def _is_file(value):
182406
return type(value) == "File"
@@ -187,4 +411,8 @@ def _is_runfiles(value):
187411
builders = struct(
188412
DepsetBuilder = _DepsetBuilder,
189413
RunfilesBuilder = _RunfilesBuilder,
414+
RuleBuilder = _RuleBuilder,
415+
TransitionBuilder = _TransitionBuilder,
416+
SetBuilder = _SetBuilder,
417+
Optional = _Optional,
190418
)

‎python/private/py_binary_macro.bzl

Copy file name to clipboardExpand all lines: python/private/py_binary_macro.bzl
+4-1Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,5 +17,8 @@ load(":py_binary_rule.bzl", py_binary_rule = "py_binary")
1717
load(":py_executable.bzl", "convert_legacy_create_init_to_int")
1818

1919
def py_binary(**kwargs):
20+
py_binary_macro(py_binary_rule, **kwargs)
21+
22+
def py_binary_macro(py_rule, **kwargs):
2023
convert_legacy_create_init_to_int(kwargs)
21-
py_binary_rule(**kwargs)
24+
py_rule(**kwargs)

‎python/private/py_binary_rule.bzl

Copy file name to clipboardExpand all lines: python/private/py_binary_rule.bzl
+12-8Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -13,15 +13,14 @@
1313
# limitations under the License.
1414
"""Rule implementation of py_binary for Bazel."""
1515

16-
load("@bazel_skylib//lib:dicts.bzl", "dicts")
1716
load(":attributes.bzl", "AGNOSTIC_BINARY_ATTRS")
1817
load(
1918
":py_executable.bzl",
20-
"create_executable_rule",
19+
"create_executable_rule_builder",
2120
"py_executable_impl",
2221
)
2322

24-
_PY_TEST_ATTRS = {
23+
_COVERAGE_ATTRS = {
2524
# Magic attribute to help C++ coverage work. There's no
2625
# docs about this; see TestActionBuilder.java
2726
"_collect_cc_coverage": attr.label(
@@ -45,8 +44,13 @@ def _py_binary_impl(ctx):
4544
inherited_environment = [],
4645
)
4746

48-
py_binary = create_executable_rule(
49-
implementation = _py_binary_impl,
50-
attrs = dicts.add(AGNOSTIC_BINARY_ATTRS, _PY_TEST_ATTRS),
51-
executable = True,
52-
)
47+
def create_binary_rule_builder():
48+
builder = create_executable_rule_builder(
49+
implementation = _py_binary_impl,
50+
executable = True,
51+
)
52+
builder.attrs.update(AGNOSTIC_BINARY_ATTRS)
53+
builder.attrs.update(_COVERAGE_ATTRS)
54+
return builder
55+
56+
py_binary = create_binary_rule_builder().build()

‎python/private/py_executable.bzl

Copy file name to clipboardExpand all lines: python/private/py_executable.bzl
+16-26Lines changed: 16 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -1747,50 +1747,40 @@ def _transition_executable_impl(input_settings, attr):
17471747
settings[_PYTHON_VERSION_FLAG] = attr.python_version
17481748
return settings
17491749

1750-
_transition_executable = transition(
1751-
implementation = _transition_executable_impl,
1752-
inputs = [
1753-
_PYTHON_VERSION_FLAG,
1754-
],
1755-
outputs = [
1756-
_PYTHON_VERSION_FLAG,
1757-
],
1758-
)
1759-
17601750
def create_executable_rule(*, attrs, **kwargs):
17611751
return create_base_executable_rule(
17621752
attrs = attrs,
17631753
fragments = ["py", "bazel_py"],
17641754
**kwargs
17651755
)
17661756

1767-
def create_base_executable_rule(*, attrs, fragments = [], **kwargs):
1757+
def create_base_executable_rule():
17681758
"""Create a function for defining for Python binary/test targets.
17691759
1770-
Args:
1771-
attrs: Rule attributes
1772-
fragments: List of str; extra config fragments that are required.
1773-
**kwargs: Additional args to pass onto `rule()`
1774-
17751760
Returns:
17761761
A rule function
17771762
"""
1778-
if "py" not in fragments:
1779-
# The list might be frozen, so use concatentation
1780-
fragments = fragments + ["py"]
1781-
kwargs.setdefault("provides", []).append(PyExecutableInfo)
1782-
kwargs["exec_groups"] = REQUIRED_EXEC_GROUPS | (kwargs.get("exec_groups") or {})
1783-
kwargs.setdefault("cfg", _transition_executable)
1784-
return rule(
1785-
# TODO: add ability to remove attrs, i.e. for imports attr
1786-
attrs = dicts.add(EXECUTABLE_ATTRS, attrs),
1763+
return create_executable_rule_builder().build()
1764+
1765+
def create_executable_rule_builder(implementation, **kwargs):
1766+
builder = builders.RuleBuilder(
1767+
implementation = implementation,
1768+
attrs = EXECUTABLE_ATTRS,
1769+
exec_groups = REQUIRED_EXEC_GROUPS,
1770+
fragments = ["py", "bazel_py"],
1771+
provides = [PyExecutableInfo],
17871772
toolchains = [
17881773
TOOLCHAIN_TYPE,
17891774
config_common.toolchain_type(EXEC_TOOLS_TOOLCHAIN_TYPE, mandatory = False),
17901775
] + _CC_TOOLCHAINS,
1791-
fragments = fragments,
1776+
cfg = builders.TransitionBuilder(
1777+
implementation = _transition_executable_impl,
1778+
inputs = [_PYTHON_VERSION_FLAG],
1779+
outputs = [_PYTHON_VERSION_FLAG],
1780+
),
17921781
**kwargs
17931782
)
1783+
return builder
17941784

17951785
def cc_configure_features(
17961786
ctx,

‎python/private/py_test_macro.bzl

Copy file name to clipboardExpand all lines: python/private/py_test_macro.bzl
+4-1Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,5 +17,8 @@ load(":py_executable.bzl", "convert_legacy_create_init_to_int")
1717
load(":py_test_rule.bzl", py_test_rule = "py_test")
1818

1919
def py_test(**kwargs):
20+
py_test_macro(py_test_rule, **kwargs)
21+
22+
def py_test_macro(py_rule, **kwargs):
2023
convert_legacy_create_init_to_int(kwargs)
21-
py_test_rule(**kwargs)
24+
py_rule(**kwargs)

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.