From 80826d31071b406a3a45d8912fae76be70f8fff1 Mon Sep 17 00:00:00 2001 From: Richard Levasseur Date: Mon, 1 May 2023 03:50:07 +0000 Subject: [PATCH 1/2] fix: Allow passing a tuple to the `tags` attribute. Starlark rules allow giving the tags as a tuple. The helper function that added the special migration tag assumed tags was always a list, resulting in an error when it tried to concatenate a list and tuple. To fix, check if tags is a tuple and concatenate a tuple if so. The input type of the tags attribute is preserved so that a test verifying tags can be passed to the underlying rule can be implemented (this test is to verify there isn't a regression during the rewrite to Starlark). --- python/private/BUILD.bazel | 1 + python/private/util.bzl | 19 +++++++++++++++- tools/build_defs/python/tests/base_tests.bzl | 23 +++++++++++++++++++- 3 files changed, 41 insertions(+), 2 deletions(-) diff --git a/python/private/BUILD.bazel b/python/private/BUILD.bazel index 4068ea480b..abf230309b 100644 --- a/python/private/BUILD.bazel +++ b/python/private/BUILD.bazel @@ -45,6 +45,7 @@ bzl_library( name = "util_bzl", srcs = ["util.bzl"], visibility = ["//python:__subpackages__"], + deps = ["@bazel_skylib//lib:types"], ) # @bazel_tools can't define bzl_library itself, so we just put a wrapper around it. diff --git a/python/private/util.bzl b/python/private/util.bzl index 25a50aac6a..f0d43737a0 100644 --- a/python/private/util.bzl +++ b/python/private/util.bzl @@ -1,5 +1,7 @@ """Functionality shared by multiple pieces of code.""" +load("@bazel_skylib//lib:types.bzl", "types") + def copy_propagating_kwargs(from_kwargs, into_kwargs = None): """Copies args that must be compatible between two targets with a dependency relationship. @@ -36,8 +38,23 @@ def copy_propagating_kwargs(from_kwargs, into_kwargs = None): _MIGRATION_TAG = "__PYTHON_RULES_MIGRATION_DO_NOT_USE_WILL_BREAK__" def add_migration_tag(attrs): + """Add a special tag to `attrs` to aid migration off native rles. + + Args: + attrs: dict of keyword args. The `tags` key will be modified in-place. + + Returns: + The same `attrs` object, but modified. + """ if "tags" in attrs and attrs["tags"] != None: - attrs["tags"] = attrs["tags"] + [_MIGRATION_TAG] + tags = attrs["tags"] + + # Preserve the input type: this allows a test verifying the underlying + # rule can accept the tuple for the tags argument. + if types.is_tuple(tags): + attrs["tags"] = tags + (_MIGRATION_TAG,) + else: + attrs["tags"] = tags + [_MIGRATION_TAG] else: attrs["tags"] = [_MIGRATION_TAG] return attrs diff --git a/tools/build_defs/python/tests/base_tests.bzl b/tools/build_defs/python/tests/base_tests.bzl index 715aea7fde..467611fcd8 100644 --- a/tools/build_defs/python/tests/base_tests.bzl +++ b/tools/build_defs/python/tests/base_tests.bzl @@ -15,7 +15,7 @@ load("@rules_testing//lib:analysis_test.bzl", "analysis_test") load("@rules_testing//lib:truth.bzl", "matching") -load("@rules_testing//lib:util.bzl", rt_util = "util") +load("@rules_testing//lib:util.bzl", "PREVENT_IMPLICIT_BUILDING_TAGS", rt_util = "util") load("//python:defs.bzl", "PyInfo") load("//tools/build_defs/python/tests:py_info_subject.bzl", "py_info_subject") load("//tools/build_defs/python/tests:util.bzl", pt_util = "util") @@ -99,5 +99,26 @@ def _test_data_sets_uses_shared_library_impl(env, target): _tests.append(_test_data_sets_uses_shared_library) +def _test_tags_can_be_tuple(name, config): + # We don't use a helper because we want to ensure that value passed is + # a tuple. + config.base_test_rule( + name = name + "_subject", + tags = ("one", "two") + tuple(PREVENT_IMPLICIT_BUILDING_TAGS), + ) + analysis_test( + name = name, + target = name + "_subject", + impl = _test_tags_can_be_tuple_impl, + ) + +def _test_tags_can_be_tuple_impl(env, target): + env.expect.that_target(target).tags().contains_at_least([ + "one", + "two", + ]) + +_tests.append(_test_tags_can_be_tuple) + def create_base_tests(config): return pt_util.create_tests(_tests, config = config) From 13678833e7282592263c4b35a7c70bc3ed00c608 Mon Sep 17 00:00:00 2001 From: Richard Levasseur Date: Mon, 1 May 2023 16:32:13 +0000 Subject: [PATCH 2/2] fixup! fix: Allow passing a tuple to the `tags` attribute. --- docs/BUILD.bazel | 3 +++ python/private/BUILD.bazel | 5 ++++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/docs/BUILD.bazel b/docs/BUILD.bazel index 27af3e7ed5..938ba85dd5 100644 --- a/docs/BUILD.bazel +++ b/docs/BUILD.bazel @@ -80,6 +80,9 @@ bzl_library( "//python/private:stamp.bzl", "//python/private:util.bzl", ], + deps = [ + "//python/private:util_bzl", + ], ) # TODO: Stardoc does not guarantee consistent outputs accross platforms (Unix/Windows). diff --git a/python/private/BUILD.bazel b/python/private/BUILD.bazel index abf230309b..f454f42cf3 100644 --- a/python/private/BUILD.bazel +++ b/python/private/BUILD.bazel @@ -44,7 +44,10 @@ bzl_library( bzl_library( name = "util_bzl", srcs = ["util.bzl"], - visibility = ["//python:__subpackages__"], + visibility = [ + "//docs:__subpackages__", + "//python:__subpackages__", + ], deps = ["@bazel_skylib//lib:types"], )