Skip to content

Commit

Permalink
fix(PyRuntimeInfo): use builtin PyRuntimeInfo unless pystar is enable…
Browse files Browse the repository at this point in the history
…d. (#1748)

This fixes the bug where the PyRuntimeInfo symbol rules_python exported
wasn't matching the provider identity that `py_binary` actually
provided.

The basic problem was, when pystar is disabled:
 * PyRuntimeInfo is the rules_python defined provider
* py_binary is `native.py_binary`, which provides only the builtin
PyRuntimeInfo provider.

Thus, when users loaded the rules_python PyRuntimeInfo symbol, it was
referring to a provider that the underlying py_binary didn't actually
provide. Pystar is always disabled on Bazel 6.4,
and enabling it for Bazel 7 will happen eminently.

This typically showed up when users had a custom rule with an attribute
definition that used the rules_python PyRuntimeInfo.

To fix, only use the rules_python define PyRuntimeInfo if pystar is
enabled. This ensures the providers the underlying rules are providing
match the symbols that rules_python is exported.

* Also fixes `py_binary` and `py_test` to also return the builtin
PyRuntimeInfo. This
should make the transition from the builtin symbols to the rules_python
symbols a bit
  easier.

Fixes #1732
  • Loading branch information
rickeylev authored Feb 10, 2024
1 parent 677fb53 commit a5e17e6
Show file tree
Hide file tree
Showing 4 changed files with 55 additions and 3 deletions.
5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,11 @@ A brief description of the categories of changes:
* (toolchain) Changed the `host_toolchain` to symlink all files to support
Windows host environments without symlink support.

* (PyRuntimeInfo) Switch back to builtin PyRuntimeInfo for Bazel 6.4 and when
pystar is disabled. This fixes an error about `target ... does not have ...
PyRuntimeInfo`.
([#1732](https://github.com/bazelbuild/rules_python/issues/1732))

### Added

* (py_wheel) Added `requires_file` and `extra_requires_files` attributes.
Expand Down
24 changes: 23 additions & 1 deletion python/private/common/py_executable.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
"""Common functionality between test/binary executables."""

load("@bazel_skylib//lib:dicts.bzl", "dicts")
load("//python/private:reexports.bzl", "BuiltinPyRuntimeInfo")
load(
":attributes.bzl",
"AGNOSTIC_EXECUTABLE_ATTRS",
Expand Down Expand Up @@ -771,7 +772,28 @@ def _create_providers(
# TODO(b/265840007): Make this non-conditional once Google enables
# --incompatible_use_python_toolchains.
if runtime_details.toolchain_runtime:
providers.append(runtime_details.toolchain_runtime)
py_runtime_info = runtime_details.toolchain_runtime
providers.append(py_runtime_info)

# Re-add the builtin PyRuntimeInfo for compatibility to make
# transitioning easier, but only if it isn't already added because
# returning the same provider type multiple times is an error.
# NOTE: The PyRuntimeInfo from the toolchain could be a rules_python
# PyRuntimeInfo or a builtin PyRuntimeInfo -- a user could have used the
# builtin py_runtime rule or defined their own. We can't directly detect
# the type of the provider object, but the rules_python PyRuntimeInfo
# object has an extra attribute that the builtin one doesn't.
if hasattr(py_runtime_info, "interpreter_version_info"):
providers.append(BuiltinPyRuntimeInfo(
interpreter_path = py_runtime_info.interpreter_path,
interpreter = py_runtime_info.interpreter,
files = py_runtime_info.files,
coverage_tool = py_runtime_info.coverage_tool,
coverage_files = py_runtime_info.coverage_files,
python_version = py_runtime_info.python_version,
stub_shebang = py_runtime_info.stub_shebang,
bootstrap_template = py_runtime_info.bootstrap_template,
))

# TODO(b/163083591): Remove the PyCcLinkParamsProvider once binaries-in-deps
# are cleaned up.
Expand Down
4 changes: 2 additions & 2 deletions python/py_runtime_info.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@

"""Public entry point for PyRuntimeInfo."""

load("@rules_python_internal//:rules_python_config.bzl", "config")
load("//python/private:reexports.bzl", "BuiltinPyRuntimeInfo")
load("//python/private:util.bzl", "IS_BAZEL_6_OR_HIGHER")
load("//python/private/common:providers.bzl", _starlark_PyRuntimeInfo = "PyRuntimeInfo")

PyRuntimeInfo = _starlark_PyRuntimeInfo if IS_BAZEL_6_OR_HIGHER else BuiltinPyRuntimeInfo
PyRuntimeInfo = _starlark_PyRuntimeInfo if config.enable_pystar else BuiltinPyRuntimeInfo
25 changes: 25 additions & 0 deletions tests/base_rules/py_executable_base_tests.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
# limitations under the License.
"""Tests common to py_binary and py_test (executable rules)."""

load("@rules_python//python:py_runtime_info.bzl", RulesPythonPyRuntimeInfo = "PyRuntimeInfo")
load("@rules_python_internal//:rules_python_config.bzl", rp_config = "config")
load("@rules_testing//lib:analysis_test.bzl", "analysis_test")
load("@rules_testing//lib:truth.bzl", "matching")
Expand All @@ -21,6 +22,8 @@ load("//tests/base_rules:base_tests.bzl", "create_base_tests")
load("//tests/base_rules:util.bzl", "WINDOWS_ATTR", pt_util = "util")
load("//tests/support:test_platforms.bzl", "WINDOWS")

_BuiltinPyRuntimeInfo = PyRuntimeInfo

_tests = []

def _test_basic_windows(name, config):
Expand Down Expand Up @@ -275,6 +278,28 @@ def _test_name_cannot_end_in_py_impl(env, target):
matching.str_matches("name must not end in*.py"),
)

def _test_py_runtime_info_provided(name, config):
rt_util.helper_target(
config.rule,
name = name + "_subject",
srcs = [name + "_subject.py"],
)
analysis_test(
name = name,
impl = _test_py_runtime_info_provided_impl,
target = name + "_subject",
)

def _test_py_runtime_info_provided_impl(env, target):
# Make sure that the rules_python loaded symbol is provided.
env.expect.that_target(target).has_provider(RulesPythonPyRuntimeInfo)

# For compatibility during the transition, the builtin PyRuntimeInfo should
# also be provided.
env.expect.that_target(target).has_provider(_BuiltinPyRuntimeInfo)

_tests.append(_test_py_runtime_info_provided)

# Can't test this -- mandatory validation happens before analysis test
# can intercept it
# TODO(#1069): Once re-implemented in Starlark, modify rule logic to make this
Expand Down

0 comments on commit a5e17e6

Please sign in to comment.