Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: fold per-target python version into base rules #2541

Merged
merged 5 commits into from
Dec 31, 2024
Merged

refactor: fold per-target python version into base rules #2541

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
535be2f
fold per-target python version into base rule
rickeylev Dec 29, 2024
7b0620b
make aliases wrappers with deprecation set
rickeylev Dec 31, 2024
bcf2882
fix deprecation message
rickeylev Dec 31, 2024
8e42447
undo bazelversion change
rickeylev Dec 31, 2024
aaab257
Merge branch 'main' into refactor.consolidate.pyversion.transition
rickeylev Dec 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,14 @@ Unreleased changes template.
tested by CI, so functionality cannot be guaranteed.
* ({bzl:obj}`pip.parse`) Only query SimpleAPI for packages that have
sha values in the `requirements.txt` file.
* (rules) The version-aware rules have been folded into the base rules and
the version-aware rules are now simply aliases for the base rules. The
`python_version` attribute is still used to specify the Python version.

{#v0-0-0-deprecations}
#### Deprecations
* `//python/config_settings:transitions.bzl` and its `py_binary` and `py_test`
wrappers are deprecated. Use the regular rules instead.

{#v0-0-0-fixed}
### Fixed
Expand Down
265 changes: 7 additions & 258 deletions python/config_settings/transition.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,266 +14,15 @@

"""The transition module contains the rule definitions to wrap py_binary and py_test and transition
them to the desired target platform.
:::{versionchanged} VERSION_NEXT_PATCH
The `py_binary` and `py_test` symbols are aliases to the regular rules. Usages
of them should be changed to load the regular rules directly.
:::
"""

load("@bazel_skylib//lib:dicts.bzl", "dicts")
load("//python:py_binary.bzl", _py_binary = "py_binary")
load("//python:py_info.bzl", "PyInfo")
load("//python:py_runtime_info.bzl", "PyRuntimeInfo")
load("//python:py_test.bzl", _py_test = "py_test")
load("//python/config_settings/private:py_args.bzl", "py_args")
load("//python/private:reexports.bzl", "BuiltinPyInfo", "BuiltinPyRuntimeInfo")

def _transition_python_version_impl(_, attr):
return {"//python/config_settings:python_version": str(attr.python_version)}

_transition_python_version = transition(
implementation = _transition_python_version_impl,
inputs = [],
outputs = ["//python/config_settings:python_version"],
)

def _transition_py_impl(ctx):
target = ctx.attr.target
windows_constraint = ctx.attr._windows_constraint[platform_common.ConstraintValueInfo]
target_is_windows = ctx.target_platform_has_constraint(windows_constraint)
executable = ctx.actions.declare_file(ctx.attr.name + (".exe" if target_is_windows else ""))
ctx.actions.symlink(
is_executable = True,
output = executable,
target_file = target[DefaultInfo].files_to_run.executable,
)
default_outputs = []
if target_is_windows:
# NOTE: Bazel 6 + host=linux + target=windows results in the .exe extension missing
inner_bootstrap_path = _strip_suffix(target[DefaultInfo].files_to_run.executable.short_path, ".exe")
inner_bootstrap = None
inner_zip_file_path = inner_bootstrap_path + ".zip"
inner_zip_file = None
for file in target[DefaultInfo].files.to_list():
if file.short_path == inner_bootstrap_path:
inner_bootstrap = file
elif file.short_path == inner_zip_file_path:
inner_zip_file = file

# TODO: Use `fragments.py.build_python_zip` once Bazel 6 support is dropped.
# Which file the Windows .exe looks for depends on the --build_python_zip file.
# Bazel 7+ has APIs to know the effective value of that flag, but not Bazel 6.
# To work around this, we treat the existence of a .zip in the default outputs
# to mean --build_python_zip=true.
if inner_zip_file:
suffix = ".zip"
underlying_launched_file = inner_zip_file
else:
suffix = ""
underlying_launched_file = inner_bootstrap

if underlying_launched_file:
launched_file_symlink = ctx.actions.declare_file(ctx.attr.name + suffix)
ctx.actions.symlink(
is_executable = True,
output = launched_file_symlink,
target_file = underlying_launched_file,
)
default_outputs.append(launched_file_symlink)

env = {}
for k, v in ctx.attr.env.items():
env[k] = ctx.expand_location(v)

providers = [
DefaultInfo(
executable = executable,
files = depset(default_outputs, transitive = [target[DefaultInfo].files]),
runfiles = ctx.runfiles(default_outputs).merge(target[DefaultInfo].default_runfiles),
),
# Ensure that the binary we're wrapping is included in code coverage.
coverage_common.instrumented_files_info(
ctx,
dependency_attributes = ["target"],
),
target[OutputGroupInfo],
# TODO(f0rmiga): testing.TestEnvironment is deprecated in favour of RunEnvironmentInfo but
# RunEnvironmentInfo is not exposed in Bazel < 5.3.
# https://github.com/bazelbuild/rules_python/issues/901
# https://github.com/bazelbuild/bazel/commit/dbdfa07e92f99497be9c14265611ad2920161483
testing.TestEnvironment(env),
]
if PyInfo in target:
providers.append(target[PyInfo])
if BuiltinPyInfo != None and BuiltinPyInfo in target and PyInfo != BuiltinPyInfo:
providers.append(target[BuiltinPyInfo])

if PyRuntimeInfo in target:
providers.append(target[PyRuntimeInfo])
if BuiltinPyRuntimeInfo != None and BuiltinPyRuntimeInfo in target and PyRuntimeInfo != BuiltinPyRuntimeInfo:
providers.append(target[BuiltinPyRuntimeInfo])
return providers

_COMMON_ATTRS = {
"deps": attr.label_list(
mandatory = False,
),
"env": attr.string_dict(
mandatory = False,
),
"python_version": attr.string(
mandatory = True,
),
"srcs": attr.label_list(
allow_files = True,
mandatory = False,
),
"target": attr.label(
executable = True,
cfg = "target",
mandatory = True,
providers = [PyInfo],
),
# "tools" is a hack here. It should be "data" but "data" is not included by default in the
# location expansion in the same way it is in the native Python rules. The difference on how
# the Bazel deals with those special attributes differ on the LocationExpander, e.g.:
# https://github.com/bazelbuild/bazel/blob/ce611646/src/main/java/com/google/devtools/build/lib/analysis/LocationExpander.java#L415-L429
#
# Since the default LocationExpander used by ctx.expand_location is not the same as the native
# rules (it doesn't set "allowDataAttributeEntriesInLabel"), we use "tools" temporarily while a
# proper fix in Bazel happens.
#
# A fix for this was proposed in https://github.com/bazelbuild/bazel/pull/16381.
"tools": attr.label_list(
allow_files = True,
mandatory = False,
),
# Required to Opt-in to the transitions feature.
"_allowlist_function_transition": attr.label(
default = "@bazel_tools//tools/allowlists/function_transition_allowlist",
),
"_windows_constraint": attr.label(
default = "@platforms//os:windows",
),
}

_PY_TEST_ATTRS = {
# Magic attribute to help C++ coverage work. There's no
# docs about this; see TestActionBuilder.java
"_collect_cc_coverage": attr.label(
default = "@bazel_tools//tools/test:collect_cc_coverage",
executable = True,
cfg = "exec",
),
# Magic attribute to make coverage work. There's no
# docs about this; see TestActionBuilder.java
"_lcov_merger": attr.label(
default = configuration_field(fragment = "coverage", name = "output_generator"),
executable = True,
cfg = "exec",
),
}

_transition_py_binary = rule(
_transition_py_impl,
attrs = _COMMON_ATTRS | _PY_TEST_ATTRS,
cfg = _transition_python_version,
executable = True,
fragments = ["py"],
)

_transition_py_test = rule(
_transition_py_impl,
attrs = _COMMON_ATTRS | _PY_TEST_ATTRS,
cfg = _transition_python_version,
test = True,
fragments = ["py"],
)

def _py_rule(rule_impl, transition_rule, name, python_version, **kwargs):
pyargs = py_args(name, kwargs)
args = pyargs["args"]
data = pyargs["data"]
env = pyargs["env"]
srcs = pyargs["srcs"]
deps = pyargs["deps"]
main = pyargs["main"]

# Attributes common to all build rules.
# https://bazel.build/reference/be/common-definitions#common-attributes
compatible_with = kwargs.pop("compatible_with", None)
deprecation = kwargs.pop("deprecation", None)
exec_compatible_with = kwargs.pop("exec_compatible_with", None)
exec_properties = kwargs.pop("exec_properties", None)
features = kwargs.pop("features", None)
restricted_to = kwargs.pop("restricted_to", None)
tags = kwargs.pop("tags", None)
target_compatible_with = kwargs.pop("target_compatible_with", None)
testonly = kwargs.pop("testonly", None)
toolchains = kwargs.pop("toolchains", None)
visibility = kwargs.pop("visibility", None)

common_attrs = {
"compatible_with": compatible_with,
"deprecation": deprecation,
"exec_compatible_with": exec_compatible_with,
"exec_properties": exec_properties,
"features": features,
"restricted_to": restricted_to,
"target_compatible_with": target_compatible_with,
"testonly": testonly,
"toolchains": toolchains,
}

# Test-specific extra attributes.
if "env_inherit" in kwargs:
common_attrs["env_inherit"] = kwargs.pop("env_inherit")
if "size" in kwargs:
common_attrs["size"] = kwargs.pop("size")
if "timeout" in kwargs:
common_attrs["timeout"] = kwargs.pop("timeout")
if "flaky" in kwargs:
common_attrs["flaky"] = kwargs.pop("flaky")
if "shard_count" in kwargs:
common_attrs["shard_count"] = kwargs.pop("shard_count")
if "local" in kwargs:
common_attrs["local"] = kwargs.pop("local")

# Binary-specific extra attributes.
if "output_licenses" in kwargs:
common_attrs["output_licenses"] = kwargs.pop("output_licenses")

rule_impl(
rickeylev marked this conversation as resolved.
Show resolved Hide resolved
name = "_" + name,
args = args,
data = data,
deps = deps,
env = env,
srcs = srcs,
main = main,
tags = ["manual"] + (tags if tags else []),
visibility = ["//visibility:private"],
**dicts.add(common_attrs, kwargs)
)

return transition_rule(
name = name,
args = args,
deps = deps,
env = env,
python_version = python_version,
srcs = srcs,
tags = tags,
target = ":_" + name,
tools = data,
visibility = visibility,
**common_attrs
)

def py_binary(name, python_version, **kwargs):
return _py_rule(_py_binary, _transition_py_binary, name, python_version, **kwargs)

def py_test(name, python_version, **kwargs):
return _py_rule(_py_test, _transition_py_test, name, python_version, **kwargs)

def _strip_suffix(s, suffix):
if s.endswith(suffix):
return s[:-len(suffix)]
else:
return s
py_binary = _py_binary
rickeylev marked this conversation as resolved.
Show resolved Hide resolved
py_test = _py_test
55 changes: 46 additions & 9 deletions python/private/py_executable.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ load(
_py_builtins = py_internal
_EXTERNAL_PATH_PREFIX = "external"
_ZIP_RUNFILES_DIRECTORY_NAME = "runfiles"
_PYTHON_VERSION_FLAG = str(Label("//python/config_settings:python_version"))

# Bazel 5.4 doesn't have config_common.toolchain_type
_CC_TOOLCHAINS = [config_common.toolchain_type(
Expand Down Expand Up @@ -132,16 +133,34 @@ Valid values are:
target level.
""",
),
# TODO(b/203567235): In Google, this attribute is deprecated, and can
# only effectively be PY3. Externally, with Bazel, this attribute has
# a separate story.
"python_version": attr.string(
# TODO(b/203567235): In the Java impl, the default comes from
# --python_version. Not clear what the Starlark equivalent is.
default = "PY3",
# NOTE: Some tests care about the order of these values.
values = ["PY2", "PY3"],
doc = "Defunct, unused, does nothing.",
doc = """
The Python version this target should use.

The value should be in `X.Y` or `X.Y.Z` (or compatible) format. If empty or
unspecified, the incoming configuration's {obj}`--python_version` flag is
inherited. For backwards compatibility, the values `PY2` and `PY3` are
accepted, but treated as an empty/unspecified value.

:::{note}
In order for the requested version to be used, there must be a
toolchain configured to match the Python version. If there isn't, then it
may be silently ignored, or an error may occur, depending on the toolchain
configuration.
:::

:::{versionchanged} VERSION_NEXT_PATCH

This attribute was changed from only accepting `PY2` and `PY3` values to
accepting arbitrary Python versions.
:::
""",
),
# Required to opt-in to the transition feature.
"_allowlist_function_transition": attr.label(
default = "@bazel_tools//tools/allowlists/function_transition_allowlist",
),
"_bootstrap_impl_flag": attr.label(
default = "//python/config_settings:bootstrap_impl",
Expand Down Expand Up @@ -1008,7 +1027,7 @@ def _get_build_info(ctx, cc_toolchain):
return build_info_files.redacted_build_info_files.to_list()

def _validate_executable(ctx):
if ctx.attr.python_version != "PY3":
if ctx.attr.python_version == "PY2":
fail("It is not allowed to use Python 2")

def _declare_executable_file(ctx):
Expand Down Expand Up @@ -1691,9 +1710,26 @@ def _create_run_environment_info(ctx, inherited_environment):
inherited_environment = inherited_environment,
)

def _transition_executable_impl(input_settings, attr):
settings = {
_PYTHON_VERSION_FLAG: input_settings[_PYTHON_VERSION_FLAG],
}
if attr.python_version and attr.python_version not in ("PY2", "PY3"):
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 = dicts.add(EXECUTABLE_ATTRS, attrs),
attrs = attrs,
fragments = ["py", "bazel_py"],
**kwargs
Expand All @@ -1715,6 +1751,7 @@ def create_base_executable_rule(*, attrs, fragments = [], **kwargs):
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),
Expand Down
Loading