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.

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

fix(toolchain): restrict coverage tool visibility under bzlmod #1252

Merged
merged 2 commits into from
Jun 23, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
17 changes: 0 additions & 17 deletions MODULE.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -29,23 +29,6 @@ use_repo(
"pypi__tomli",
"pypi__wheel",
"pypi__zipp",
# coverage_deps managed by running ./tools/update_coverage_deps.py <version>
"pypi__coverage_cp310_aarch64-apple-darwin",
"pypi__coverage_cp310_aarch64-unknown-linux-gnu",
"pypi__coverage_cp310_x86_64-apple-darwin",
"pypi__coverage_cp310_x86_64-unknown-linux-gnu",
"pypi__coverage_cp311_aarch64-apple-darwin",
"pypi__coverage_cp311_aarch64-unknown-linux-gnu",
"pypi__coverage_cp311_x86_64-apple-darwin",
"pypi__coverage_cp311_x86_64-unknown-linux-gnu",
"pypi__coverage_cp38_aarch64-apple-darwin",
"pypi__coverage_cp38_aarch64-unknown-linux-gnu",
"pypi__coverage_cp38_x86_64-apple-darwin",
"pypi__coverage_cp38_x86_64-unknown-linux-gnu",
"pypi__coverage_cp39_aarch64-apple-darwin",
"pypi__coverage_cp39_aarch64-unknown-linux-gnu",
"pypi__coverage_cp39_x86_64-apple-darwin",
"pypi__coverage_cp39_x86_64-unknown-linux-gnu",
)

# We need to do another use_extension call to expose the "pythons_hub"
Expand Down
10 changes: 10 additions & 0 deletions examples/bzlmod/description.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
Before this PR the `coverage_tool` automatically registered by `rules_python`
was visible outside the toolchain repository. This fixes it to be consistent
with `non-bzlmod` setups and ensures that the default `coverage_tool` is not
visible outside the toolchain repos.

This means that the `MODULE.bazel` file can be cleaned-up at the expense of
relaxing the `coverage_tool` attribute for the `python_repository` to be a
simple string as the label would be evaluated within the context of
`rules_python` which may not necessarily resolve correctly without the
`use_repo` statement in our `MODULE.bazel`.
2 changes: 1 addition & 1 deletion examples/bzlmod/test.py
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ def test_coverage_sys_path(self):
if os.environ.get("COVERAGE_MANIFEST"):
# we are running under the 'bazel coverage :test'
self.assertTrue(
"pypi__coverage_cp" in last_item,
"_coverage" in last_item,
f"Expected {last_item} to be related to coverage",
)
self.assertEqual(pathlib.Path(last_item).name, "coverage")
Expand Down
2 changes: 0 additions & 2 deletions python/extensions/private/internal_deps.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,10 @@
"Python toolchain module extension for internal rule use"

load("@rules_python//python/pip_install:repositories.bzl", "pip_install_dependencies")
load("@rules_python//python/private:coverage_deps.bzl", "install_coverage_deps")

# buildifier: disable=unused-variable
def _internal_deps_impl(module_ctx):
pip_install_dependencies()
install_coverage_deps()

internal_deps = module_extension(
doc = "This extension to register internal rules_python dependecies.",
Expand Down
44 changes: 2 additions & 42 deletions python/private/coverage_deps.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,6 @@

load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")
load("@bazel_tools//tools/build_defs/repo:utils.bzl", "maybe")
load(
"//python:versions.bzl",
"MINOR_MAPPING",
"PLATFORMS",
)

# Update with './tools/update_coverage_deps.py <version>'
#START: managed by update_coverage_deps.py script
Expand Down Expand Up @@ -103,16 +98,14 @@ _coverage_deps = {

_coverage_patch = Label("//python/private:coverage.patch")

def coverage_dep(name, python_version, platform, visibility, install = True):
def coverage_dep(name, python_version, platform, visibility):
"""Register a singe coverage dependency based on the python version and platform.

Args:
name: The name of the registered repository.
python_version: The full python version.
platform: The platform, which can be found in //python:versions.bzl PLATFORMS dict.
visibility: The visibility of the coverage tool.
install: should we install the dependency with a given name or generate the label
of the bzlmod dependency fallback, which is hard-coded in MODULE.bazel?

Returns:
The label of the coverage tool if the platform is supported, otherwise - None.
Expand All @@ -131,17 +124,6 @@ def coverage_dep(name, python_version, platform, visibility, install = True):
# Some wheels are not present for some builds, so let's silently ignore those.
return None

if not install:
# FIXME @aignas 2023-01-19: right now we use globally installed coverage
# which has visibility set to public, but is hidden due to repo remapping.
#
# The name of the toolchain is not known when registering the coverage tooling,
# so we use this as a workaround for now.
return Label("@pypi__coverage_{abi}_{platform}//:coverage".format(
abi = abi,
platform = platform,
))

maybe(
http_archive,
name = name,
Expand All @@ -162,26 +144,4 @@ filegroup(
urls = [url],
)

return Label("@@{name}//:coverage".format(name = name))

def install_coverage_deps():
"""Register the dependency for the coverage dep.

This is only used under bzlmod.
"""

for python_version in MINOR_MAPPING.values():
for platform in PLATFORMS.keys():
if "windows" in platform:
continue

coverage_dep(
name = "pypi__coverage_cp{version_no_dot}_{platform}".format(
version_no_dot = python_version.rpartition(".")[0].replace(".", ""),
platform = platform,
),
python_version = python_version,
platform = platform,
visibility = ["//visibility:public"],
install = True,
)
return "@{name}//:coverage".format(name = name)
9 changes: 5 additions & 4 deletions python/repositories.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -373,10 +373,9 @@ python_repository = repository_rule(
_python_repository_impl,
doc = "Fetches the external tools needed for the Python toolchain.",
attrs = {
"coverage_tool": attr.label(
"coverage_tool": attr.string(
# Mirrors the definition at
# https://github.com/bazelbuild/bazel/blob/master/src/main/starlark/builtins_bzl/common/python/py_runtime_rule.bzl
allow_files = False,
doc = """
This is a target to use for collecting code coverage information from `py_binary`
and `py_test` targets.
Expand All @@ -391,6 +390,9 @@ The entry point for the tool must be loadable by a Python interpreter (e.g. a
of coverage.py (https://coverage.readthedocs.io), at least including
the `run` and `lcov` subcommands.

The target is accepted as a string by the python_repository and evaluated within
the context of the toolchain repository.

For more information see the official bazel docs
(https://bazel.build/reference/be/python#py_runtime.coverage_tool).
""",
Expand Down Expand Up @@ -534,11 +536,10 @@ def python_register_toolchains(
),
python_version = python_version,
platform = platform,
visibility = ["@@{name}_{platform}//:__subpackages__".format(
visibility = ["@{name}_{platform}//:__subpackages__".format(
name = name,
platform = platform,
)],
install = not bzlmod,
)

python_repository(
Expand Down
10 changes: 0 additions & 10 deletions tools/update_coverage_deps.py
Original file line number Diff line number Diff line change
Expand Up @@ -241,16 +241,6 @@ def main():
dry_run=args.dry_run,
)

# Update the MODULE.bazel, which needs to expose the dependencies to the toolchain
# repositories
_update_file(
path=rules_python / "MODULE.bazel",
snippet="".join(sorted([f' "{u.repo_name}",\n' for u in urls])),
start_marker=" # coverage_deps managed by running",
end_marker=")",
dry_run=args.dry_run,
)

return


Expand Down