Skip to content

Commit

Permalink
pystar: support builtin providers for compatibility
Browse files Browse the repository at this point in the history
This makes the rules_python Starlark implementation accept and return
the builtin providers. This allows depending on, and being depended on
by, the builtin rules, which enables the two rule sets to interoperate
better.

Work towards #1069
  • Loading branch information
rickeylev committed Nov 21, 2023
1 parent cde1b52 commit 1a1452d
Show file tree
Hide file tree
Showing 8 changed files with 101 additions and 44 deletions.
6 changes: 2 additions & 4 deletions python/defs.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@

load("@bazel_tools//tools/python:srcs_version.bzl", _find_requirements = "find_requirements")
load("//python:py_binary.bzl", _py_binary = "py_binary")
load("//python:py_info.bzl", internal_PyInfo = "PyInfo")
load("//python:py_info.bzl", _PyInfo = "PyInfo")
load("//python:py_library.bzl", _py_library = "py_library")
load("//python:py_runtime.bzl", _py_runtime = "py_runtime")
load("//python:py_runtime_info.bzl", internal_PyRuntimeInfo = "PyRuntimeInfo")
Expand All @@ -26,9 +26,7 @@ load(":py_import.bzl", _py_import = "py_import")

# Patching placeholder: end of loads

# Exports of native-defined providers.

PyInfo = internal_PyInfo
PyInfo = _PyInfo

PyRuntimeInfo = internal_PyRuntimeInfo

Expand Down
7 changes: 6 additions & 1 deletion python/private/common/attributes.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@

load(":common.bzl", "union_attrs")
load(":providers.bzl", "PyInfo")
load("//python/private:reexports.bzl", "BuiltinPyInfo")
load(":py_internal.bzl", "py_internal")
load(
":semantics.bzl",
Expand Down Expand Up @@ -127,7 +128,11 @@ COMMON_ATTRS = union_attrs(
PY_SRCS_ATTRS = union_attrs(
{
"deps": attr.label_list(
providers = [[PyInfo], [_CcInfo]],
providers = [
[PyInfo],
[_CcInfo],
[BuiltinPyInfo],
],
# TODO(b/228692666): Google-specific; remove these allowances once
# the depot is cleaned up.
allow_rules = DEPS_ATTR_ALLOW_RULES,
Expand Down
28 changes: 20 additions & 8 deletions python/private/common/common.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@

load(":cc_helper.bzl", "cc_helper")
load(":providers.bzl", "PyInfo")
load("//python/private:reexports.bzl", "BuiltinPyInfo")
load(":py_internal.bzl", "py_internal")
load(
":semantics.bzl",
Expand Down Expand Up @@ -265,6 +266,10 @@ def collect_imports(ctx, semantics):
dep[PyInfo].imports
for dep in ctx.attr.deps
if PyInfo in dep
] + [
dep[BuiltinPyInfo].imports
for dep in ctx.attr.deps
if BuiltinPyInfo in dep
])

def collect_runfiles(ctx, files):
Expand Down Expand Up @@ -355,8 +360,8 @@ def create_py_info(ctx, *, direct_sources, imports):
transitive_sources_files = [] # list of Files
for target in ctx.attr.deps:
# PyInfo may not be present e.g. cc_library rules.
if PyInfo in target:
info = target[PyInfo]
if PyInfo in target or BuiltinPyInfo in target:
info = _get_py_info(target)
transitive_sources_depsets.append(info.transitive_sources)
uses_shared_libraries = uses_shared_libraries or info.uses_shared_libraries
has_py2_only_sources = has_py2_only_sources or info.has_py2_only_sources
Expand Down Expand Up @@ -384,8 +389,8 @@ def create_py_info(ctx, *, direct_sources, imports):
for target in ctx.attr.data:
# TODO(b/234730058): Remove checking for PyInfo in data once depot
# cleaned up.
if PyInfo in target:
info = target[PyInfo]
if PyInfo in target or BuiltinPyInfo in target:
info = _get_py_info(target)
uses_shared_libraries = info.uses_shared_libraries
else:
files = target.files.to_list()
Expand All @@ -396,9 +401,7 @@ def create_py_info(ctx, *, direct_sources, imports):
if uses_shared_libraries:
break

# TODO(b/203567235): Set `uses_shared_libraries` field, though the Bazel
# docs indicate it's unused in Bazel and may be removed.
py_info = PyInfo(
py_info_kwargs = dict(
transitive_sources = depset(
transitive = [deps_transitive_sources, direct_sources],
),
Expand All @@ -410,7 +413,16 @@ def create_py_info(ctx, *, direct_sources, imports):
has_py3_only_sources = has_py3_only_sources,
uses_shared_libraries = uses_shared_libraries,
)
return py_info, deps_transitive_sources

# TODO(b/203567235): Set `uses_shared_libraries` field, though the Bazel
# docs indicate it's unused in Bazel and may be removed.
py_info = PyInfo(**py_info_kwargs)
builtin_py_info = BuiltinPyInfo(**py_info_kwargs)

return py_info, deps_transitive_sources, builtin_py_info

def _get_py_info(target):
return target[PyInfo] if PyInfo in target else target[BuiltinPyInfo]

def create_instrumented_files_info(ctx):
return _coverage_common.instrumented_files_info(
Expand Down
3 changes: 2 additions & 1 deletion python/private/common/py_executable.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -765,7 +765,7 @@ def _create_providers(
PyCcLinkParamsProvider(cc_info = cc_info),
)

py_info, deps_transitive_sources = create_py_info(
py_info, deps_transitive_sources, builtin_py_info = create_py_info(
ctx,
direct_sources = depset(direct_sources),
imports = imports,
Expand All @@ -780,6 +780,7 @@ def _create_providers(
)

providers.append(py_info)
providers.append(builtin_py_info)
providers.append(create_output_group_info(py_info.transitive_sources, output_groups))

extra_legacy_providers, extra_providers = semantics.get_extra_providers(
Expand Down
23 changes: 8 additions & 15 deletions python/private/reexports.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -12,36 +12,29 @@
# See the License for the specific language governing permissions and
# limitations under the License.

"""Internal re-exports of built-in symbols.
"""Internal re-exports of builtin symbols.
Currently the definitions here are re-exports of the native rules, "blessed" to
work under `--incompatible_load_python_rules_from_bzl`. As the native rules get
migrated to Starlark, their implementations will be removed from here.
We want to use both the PyInfo defined by builtins and the one defined by
rules_python. Because the builtin symbol is going away, the rules_python
PyInfo symbol is given preference. Unfortunately, that masks the builtin,
so we have to rebind it to another name and load it to make it available again.
We want to re-export a built-in symbol as if it were defined in a Starlark
file, so that users can for instance do:
```
load("@rules_python//python:defs.bzl", "PyInfo")
```
Unfortunately, we can't just write in defs.bzl
Unfortunately, we can't just write:
```
PyInfo = PyInfo
```
because the declaration of module-level symbol `PyInfo` makes the builtin
inaccessible. So instead we access the builtin here and export it under a
different name. Then we can load it from defs.bzl and export it there under
the original name.
different name. Then we can load it from elsewhere.
"""

# Don't use underscore prefix, since that would make the symbol local to this
# file only. Use a non-conventional name to emphasize that this is not a public
# symbol.
# buildifier: disable=name-conventions
internal_PyInfo = PyInfo
BuiltinPyInfo = PyInfo

# buildifier: disable=name-conventions
internal_PyRuntimeInfo = PyRuntimeInfo
4 changes: 2 additions & 2 deletions python/py_info.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
"""Public entry point for PyInfo."""

load("@rules_python_internal//:rules_python_config.bzl", "config")
load("//python/private:reexports.bzl", "internal_PyInfo")
load("//python/private:reexports.bzl", "BuiltinPyInfo")
load("//python/private/common:providers.bzl", _starlark_PyInfo = "PyInfo")

PyInfo = _starlark_PyInfo if config.enable_pystar else internal_PyInfo
PyInfo = _starlark_PyInfo if config.enable_pystar else BuiltinPyInfo
72 changes: 60 additions & 12 deletions tests/base_rules/base_tests.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -17,17 +17,37 @@ load("@rules_testing//lib:analysis_test.bzl", "analysis_test")
load("@rules_testing//lib:truth.bzl", "matching")
load("@rules_testing//lib:util.bzl", "PREVENT_IMPLICIT_BUILDING_TAGS", rt_util = "util")
load("//python:defs.bzl", "PyInfo")
load("//python/private:reexports.bzl", "BuiltinPyInfo")
load("//tests/base_rules:py_info_subject.bzl", "py_info_subject")
load("//tests/base_rules:util.bzl", pt_util = "util")

_tests = []

_PRODUCES_PY_INFO_ATTRS = {
"srcs": attr.label_list(allow_files = True),
"imports": attr.string_list(),
}

def _create_py_info(ctx, provider_type):
return [provider_type(
transitive_sources = depset(ctx.files.srcs),
imports = depset(ctx.attr.imports),
)]

def _produces_builtin_py_info_impl(ctx):
return _create_py_info(ctx, BuiltinPyInfo)

_produces_builtin_py_info = rule(
implementation = _produces_builtin_py_info_impl,
attrs = _PRODUCES_PY_INFO_ATTRS,
)

def _produces_py_info_impl(ctx):
return [PyInfo(transitive_sources = depset(ctx.files.srcs))]
return _create_py_info(ctx, BuiltinPyInfo)

_produces_py_info = rule(
implementation = _produces_py_info_impl,
attrs = {"srcs": attr.label_list(allow_files = True)},
attrs = _PRODUCES_PY_INFO_ATTRS,
)

def _not_produces_py_info_impl(ctx):
Expand All @@ -38,30 +58,58 @@ _not_produces_py_info = rule(
implementation = _not_produces_py_info_impl,
)

def _test_consumes_provider(name, config):
def _py_info_propagation_setup(name, config, produce_py_info_rule, test_impl):
rt_util.helper_target(
config.base_test_rule,
name = name + "_subject",
deps = [name + "_produces_py_info"],
deps = [name + "_produces_builtin_py_info"],
)
rt_util.helper_target(
_produces_py_info,
name = name + "_produces_py_info",
produce_py_info_rule,
name = name + "_produces_builtin_py_info",
srcs = [rt_util.empty_file(name + "_produce.py")],
imports = ["custom-import"],
)
analysis_test(
name = name,
target = name + "_subject",
impl = _test_consumes_provider_impl,
impl = test_impl,
)

def _test_consumes_provider_impl(env, target):
env.expect.that_target(target).provider(
PyInfo,
def _py_info_propagation_test_impl(env, target, provider_type):
info = env.expect.that_target(target).provider(
provider_type,
factory = py_info_subject,
).transitive_sources().contains("{package}/{test_name}_produce.py")
)

info.transitive_sources().contains("{package}/{test_name}_produce.py")
info.imports().contains("custom-import")

def _test_py_info_propagation_builtin(name, config):
_py_info_propagation_setup(
name,
config,
_produces_builtin_py_info,
_test_py_info_propagation_builtin_impl,
)

def _test_py_info_propagation_builtin_impl(env, target):
_py_info_propagation_test_impl(env, target, BuiltinPyInfo)

_tests.append(_test_py_info_propagation_builtin)

def _test_py_info_propagation(name, config):
_py_info_propagation_setup(
name,
config,
_produces_py_info,
_test_py_info_propagation_impl,
)

def _test_py_info_propagation_impl(env, target):
_py_info_propagation_test_impl(env, target, PyInfo)

_tests.append(_test_consumes_provider)
_tests.append(_test_py_info_propagation)

def _test_requires_provider(name, config):
rt_util.helper_target(
Expand Down
2 changes: 1 addition & 1 deletion tests/base_rules/py_info_subject.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ def _py_info_subject_imports(self):
Method: PyInfoSubject.imports
"""
return subjects.collection(
self.actual.imports,
self.actual.imports.to_list(),
meta = self.meta.derive("imports()"),
)

Expand Down

0 comments on commit 1a1452d

Please sign in to comment.