From f5b1d08b8ae65435c3201cdcc0a75c168f130822 Mon Sep 17 00:00:00 2001 From: aignas <240938+aignas@users.noreply.github.com> Date: Fri, 16 Feb 2024 16:53:38 +0900 Subject: [PATCH 1/2] internal(render_pkg_aliases): allow using default config setting This allows the code outside to decide what is default and what is not. This is needed when we have the same version but different constraint values as the logic where we use the 'default_version' breaks. --- python/private/render_pkg_aliases.bzl | 25 ++++++-- .../render_pkg_aliases_test.bzl | 64 +++++++++++++++++++ 2 files changed, 85 insertions(+), 4 deletions(-) diff --git a/python/private/render_pkg_aliases.bzl b/python/private/render_pkg_aliases.bzl index e38f13321d..aad4c73c80 100644 --- a/python/private/render_pkg_aliases.bzl +++ b/python/private/render_pkg_aliases.bzl @@ -38,6 +38,8 @@ If the value is missing, then the "default" Python version is being used, which has a "null" version value and will not match version constraints. """ +_CONDITIONS_DEFAULT = "//conditions:default" + def _render_whl_library_alias( *, name, @@ -60,12 +62,13 @@ def _render_whl_library_alias( for alias in sorted(aliases, key = lambda x: x.version): actual = "@{repo}//:{name}".format(repo = alias.repo, name = name) selects[alias.config_setting] = actual - if alias.version == default_version: + if alias.version == default_version or alias.config_setting == _CONDITIONS_DEFAULT: default = actual no_match_error = None if default: - selects["//conditions:default"] = default + # Attempt setting it but continue if already exists + selects.setdefault("//conditions:default", default) return render.alias( name = name, @@ -81,10 +84,24 @@ def _render_common_aliases(*, name, aliases, default_version = None): ] versions = None + has_default = False if aliases: - versions = sorted([v.version for v in aliases if v.version]) + versions = sorted([a.version for a in aliases if a.version]) + has_default = len([a.config_setting for a in aliases if a.config_setting == _CONDITIONS_DEFAULT]) == 1 + default_version_aliases = [a for a in aliases if a.version == default_version] + if not has_default and len(default_version_aliases) > 1: + fail( + ( + "BUG: expected to have a single alias for the default version, but got multiple: '{}'. " + + "Add the 'whl_alias(config_setting = \"//conditions:default\", ...)' setting explicitly." + ).format(default_version_aliases), + ) + + if has_default: + # Zero this out as it is not useful anymore. + default_version = None - if not versions or default_version in versions: + if not versions or default_version in versions or has_default: pass else: error_msg = NO_MATCH_ERROR_MESSAGE_TEMPLATE.format( diff --git a/tests/pip_hub_repository/render_pkg_aliases/render_pkg_aliases_test.bzl b/tests/pip_hub_repository/render_pkg_aliases/render_pkg_aliases_test.bzl index c61e5ef9b6..08739ee0be 100644 --- a/tests/pip_hub_repository/render_pkg_aliases/render_pkg_aliases_test.bzl +++ b/tests/pip_hub_repository/render_pkg_aliases/render_pkg_aliases_test.bzl @@ -160,6 +160,70 @@ alias( _tests.append(_test_bzlmod_aliases) +def _test_bzlmod_aliases_with_default(env): + actual = render_pkg_aliases( + aliases = { + "bar-baz": [ + whl_alias(version = "3.2", repo = "pypi_32_bar_baz", config_setting = "//:my_config_setting"), + whl_alias(version = "3.2", repo = "pypi_32_bar_baz", config_setting = "//conditions:default"), + ], + }, + ) + + want_key = "bar_baz/BUILD.bazel" + want_content = """\ +package(default_visibility = ["//visibility:public"]) + +alias( + name = "bar_baz", + actual = ":pkg", +) + +alias( + name = "pkg", + actual = select( + { + "//:my_config_setting": "@pypi_32_bar_baz//:pkg", + "//conditions:default": "@pypi_32_bar_baz//:pkg", + }, + ), +) + +alias( + name = "whl", + actual = select( + { + "//:my_config_setting": "@pypi_32_bar_baz//:whl", + "//conditions:default": "@pypi_32_bar_baz//:whl", + }, + ), +) + +alias( + name = "data", + actual = select( + { + "//:my_config_setting": "@pypi_32_bar_baz//:data", + "//conditions:default": "@pypi_32_bar_baz//:data", + }, + ), +) + +alias( + name = "dist_info", + actual = select( + { + "//:my_config_setting": "@pypi_32_bar_baz//:dist_info", + "//conditions:default": "@pypi_32_bar_baz//:dist_info", + }, + ), +)""" + + env.expect.that_collection(actual.keys()).contains_exactly([want_key]) + env.expect.that_str(actual[want_key]).equals(want_content) + +_tests.append(_test_bzlmod_aliases_with_default) + def _test_bzlmod_aliases_with_no_default_version(env): actual = render_pkg_aliases( default_version = None, From 1abac11dee004c2b0423dc3c2c434b99f3aca751 Mon Sep 17 00:00:00 2001 From: aignas <240938+aignas@users.noreply.github.com> Date: Thu, 29 Feb 2024 17:59:51 +0900 Subject: [PATCH 2/2] harden a little bit --- python/private/bzlmod/pip.bzl | 9 +++++++++ python/private/bzlmod/pip_repository.bzl | 1 - python/private/render_pkg_aliases.bzl | 8 ++++---- .../render_pkg_aliases_test.bzl | 15 ++++++++++----- 4 files changed, 23 insertions(+), 10 deletions(-) diff --git a/python/private/bzlmod/pip.bzl b/python/private/bzlmod/pip.bzl index a017089803..797e0d1648 100644 --- a/python/private/bzlmod/pip.bzl +++ b/python/private/bzlmod/pip.bzl @@ -208,6 +208,7 @@ def _create_whl_repos(module_ctx, pip_attr, whl_map, whl_overrides): ) major_minor = _major_minor_version(pip_attr.python_version) + default_major_minor = _major_minor_version(DEFAULT_PYTHON_VERSION) whl_map[hub_name].setdefault(whl_name, []).append( whl_alias( repo = repo_name, @@ -216,6 +217,14 @@ def _create_whl_repos(module_ctx, pip_attr, whl_map, whl_overrides): config_setting = Label("//python/config_settings:is_python_" + major_minor), ), ) + if default_major_minor == major_minor: + whl_map[hub_name][whl_name].append( + whl_alias( + repo = repo_name, + version = major_minor, + config_setting = "//conditions:default", + ), + ) def _pip_impl(module_ctx): """Implementation of a class tag that creates the pip hub and corresponding pip spoke whl repositories. diff --git a/python/private/bzlmod/pip_repository.bzl b/python/private/bzlmod/pip_repository.bzl index d96131dad7..8566514e60 100644 --- a/python/private/bzlmod/pip_repository.bzl +++ b/python/private/bzlmod/pip_repository.bzl @@ -31,7 +31,6 @@ def _pip_repository_impl(rctx): key: [whl_alias(**v) for v in json.decode(values)] for key, values in rctx.attr.whl_map.items() }, - default_version = rctx.attr.default_version, ) for path, contents in aliases.items(): rctx.file(path, contents) diff --git a/python/private/render_pkg_aliases.bzl b/python/private/render_pkg_aliases.bzl index aad4c73c80..2a262ce726 100644 --- a/python/private/render_pkg_aliases.bzl +++ b/python/private/render_pkg_aliases.bzl @@ -68,7 +68,7 @@ def _render_whl_library_alias( if default: # Attempt setting it but continue if already exists - selects.setdefault("//conditions:default", default) + selects.setdefault(_CONDITIONS_DEFAULT, default) return render.alias( name = name, @@ -88,13 +88,13 @@ def _render_common_aliases(*, name, aliases, default_version = None): if aliases: versions = sorted([a.version for a in aliases if a.version]) has_default = len([a.config_setting for a in aliases if a.config_setting == _CONDITIONS_DEFAULT]) == 1 - default_version_aliases = [a for a in aliases if a.version == default_version] + default_version_aliases = [a for a in aliases if a.version == default_version or a.config_setting == _CONDITIONS_DEFAULT] if not has_default and len(default_version_aliases) > 1: fail( ( "BUG: expected to have a single alias for the default version, but got multiple: '{}'. " + - "Add the 'whl_alias(config_setting = \"//conditions:default\", ...)' setting explicitly." - ).format(default_version_aliases), + "Add the 'whl_alias(config_setting = {}, ...)' setting explicitly." + ).format(default_version_aliases, repr(_CONDITIONS_DEFAULT)), ) if has_default: diff --git a/tests/pip_hub_repository/render_pkg_aliases/render_pkg_aliases_test.bzl b/tests/pip_hub_repository/render_pkg_aliases/render_pkg_aliases_test.bzl index 08739ee0be..be00118ed4 100644 --- a/tests/pip_hub_repository/render_pkg_aliases/render_pkg_aliases_test.bzl +++ b/tests/pip_hub_repository/render_pkg_aliases/render_pkg_aliases_test.bzl @@ -164,7 +164,8 @@ def _test_bzlmod_aliases_with_default(env): actual = render_pkg_aliases( aliases = { "bar-baz": [ - whl_alias(version = "3.2", repo = "pypi_32_bar_baz", config_setting = "//:my_config_setting"), + whl_alias(version = "3.2", repo = "pypi_32_bar_baz", config_setting = "//:my_config_setting_32"), + whl_alias(version = "3.1", repo = "pypi_31_bar_baz", config_setting = "//:my_config_setting_31"), whl_alias(version = "3.2", repo = "pypi_32_bar_baz", config_setting = "//conditions:default"), ], }, @@ -183,7 +184,8 @@ alias( name = "pkg", actual = select( { - "//:my_config_setting": "@pypi_32_bar_baz//:pkg", + "//:my_config_setting_31": "@pypi_31_bar_baz//:pkg", + "//:my_config_setting_32": "@pypi_32_bar_baz//:pkg", "//conditions:default": "@pypi_32_bar_baz//:pkg", }, ), @@ -193,7 +195,8 @@ alias( name = "whl", actual = select( { - "//:my_config_setting": "@pypi_32_bar_baz//:whl", + "//:my_config_setting_31": "@pypi_31_bar_baz//:whl", + "//:my_config_setting_32": "@pypi_32_bar_baz//:whl", "//conditions:default": "@pypi_32_bar_baz//:whl", }, ), @@ -203,7 +206,8 @@ alias( name = "data", actual = select( { - "//:my_config_setting": "@pypi_32_bar_baz//:data", + "//:my_config_setting_31": "@pypi_31_bar_baz//:data", + "//:my_config_setting_32": "@pypi_32_bar_baz//:data", "//conditions:default": "@pypi_32_bar_baz//:data", }, ), @@ -213,7 +217,8 @@ alias( name = "dist_info", actual = select( { - "//:my_config_setting": "@pypi_32_bar_baz//:dist_info", + "//:my_config_setting_31": "@pypi_31_bar_baz//:dist_info", + "//:my_config_setting_32": "@pypi_32_bar_baz//:dist_info", "//conditions:default": "@pypi_32_bar_baz//:dist_info", }, ),