From 0f39e4c3e7ce3d89bc1bf5955cec3c9ceda8ba78 Mon Sep 17 00:00:00 2001 From: Richard Levasseur Date: Mon, 5 Jun 2023 21:42:56 +0000 Subject: [PATCH 1/4] fix(bzlmod)!: Remove ability to specify toolchain repo name. The main reasons this is removed is because if modules choose different names for the same toolchain, only one of the two toolchains (which are, hopefully, identical) will be used. Which toolchain is used depends on the module graph dependency ordering. Furthermore, as of #1238, only one repo per version is created; others are ignored. This means a module doesn't know if the name it chooses will result in a repo being created with that name. Instead, the toolchain repos are named by rules_python: `python_{major}_{minor}`. These repo names are currently part of the public API, since they end up referenced in MODULE config (to wire the toolchain interpreter to pip). BREAKING CHANGES This removes the `name` arg from `python.toolchain()`. Users will need to remove such usages from their `MODULE.bazel` and update their `use_repo()` statements. If keeping the custom repo name is necessary, then repo mappings can be used. See #1232 for additional migration steps, commands, and information. --- .bazelignore | 2 + examples/bzlmod/BUILD.bazel | 2 +- examples/bzlmod/MODULE.bazel | 9 +--- examples/bzlmod/other_module/MODULE.bazel | 12 +---- .../other_module/other_module/pkg/BUILD.bazel | 2 +- .../bzlmod_build_file_generation/MODULE.bazel | 3 +- examples/py_proto_library/MODULE.bazel | 3 +- python/extensions/interpreter.bzl | 2 +- python/extensions/python.bzl | 53 +++---------------- 9 files changed, 17 insertions(+), 71 deletions(-) diff --git a/.bazelignore b/.bazelignore index a603a7bd8a..ec504da988 100644 --- a/.bazelignore +++ b/.bazelignore @@ -6,3 +6,5 @@ bazel-rules_python bazel-bin bazel-out bazel-testlogs +examples/bzlmod/bazel-bzlmod +examples/bzlmod_build_file_generation/bazel-bzlmod_build_file_generation diff --git a/examples/bzlmod/BUILD.bazel b/examples/bzlmod/BUILD.bazel index 0a068ce640..3bff20836d 100644 --- a/examples/bzlmod/BUILD.bazel +++ b/examples/bzlmod/BUILD.bazel @@ -7,7 +7,7 @@ # names. Those names are defined in the MODULES.bazel file. load("@bazel_skylib//rules:build_test.bzl", "build_test") load("@pip//:requirements.bzl", "all_requirements", "all_whl_requirements", "requirement") -load("@python_39//:defs.bzl", py_test_with_transition = "py_test") +load("@python_3_9//:defs.bzl", py_test_with_transition = "py_test") load("@rules_python//python:defs.bzl", "py_binary", "py_library", "py_test") load("@rules_python//python:pip.bzl", "compile_pip_requirements") diff --git a/examples/bzlmod/MODULE.bazel b/examples/bzlmod/MODULE.bazel index 24bb4581f4..33cb905bee 100644 --- a/examples/bzlmod/MODULE.bazel +++ b/examples/bzlmod/MODULE.bazel @@ -13,11 +13,11 @@ local_path_override( # This name is passed into python.toolchain and it's use_repo statement. # We also use the same value in the python.host_python_interpreter call. -PYTHON_NAME_39 = "python_39" +PYTHON_NAME_39 = "python_3_9" INTERPRETER_NAME_39 = "interpreter_39" -PYTHON_NAME_310 = "python_310" +PYTHON_NAME_310 = "python_3_10" INTERPRETER_NAME_310 = "interpreter_310" @@ -25,10 +25,6 @@ INTERPRETER_NAME_310 = "interpreter_310" # You can set different Python versions in this block. python = use_extension("@rules_python//python/extensions:python.bzl", "python") python.toolchain( - # This name is used in the various use_repo statements - # below, and in the local extension that is in this - # example. - name = PYTHON_NAME_39, configure_coverage_tool = True, # Only set when you have mulitple toolchain versions. is_default = True, @@ -41,7 +37,6 @@ python.toolchain( # Note: we do not supporting using multiple pip extensions, this is # work in progress. python.toolchain( - name = PYTHON_NAME_310, configure_coverage_tool = True, python_version = "3.10", ) diff --git a/examples/bzlmod/other_module/MODULE.bazel b/examples/bzlmod/other_module/MODULE.bazel index 5fb745266f..cc23a51601 100644 --- a/examples/bzlmod/other_module/MODULE.bazel +++ b/examples/bzlmod/other_module/MODULE.bazel @@ -10,24 +10,16 @@ bazel_dep(name = "rules_python", version = "") # a submodule. This code only exists to test that # we support doing this. This code is only for rules_python # testing purposes. -PYTHON_NAME_39 = "python_39" +PYTHON_NAME_39 = "python_3_9" -PYTHON_NAME_311 = "python_311" +PYTHON_NAME_311 = "python_3_11" python = use_extension("@rules_python//python/extensions:python.bzl", "python") python.toolchain( - # This name is used in the various use_repo statements - # below, and in the local extension that is in this - # example. - name = PYTHON_NAME_39, configure_coverage_tool = True, python_version = "3.9", ) python.toolchain( - # This name is used in the various use_repo statements - # below, and in the local extension that is in this - # example. - name = PYTHON_NAME_311, configure_coverage_tool = True, # In a submodule this is ignored is_default = True, diff --git a/examples/bzlmod/other_module/other_module/pkg/BUILD.bazel b/examples/bzlmod/other_module/other_module/pkg/BUILD.bazel index 952a674d48..6e37df8233 100644 --- a/examples/bzlmod/other_module/other_module/pkg/BUILD.bazel +++ b/examples/bzlmod/other_module/other_module/pkg/BUILD.bazel @@ -1,4 +1,4 @@ -load("@python_311//:defs.bzl", py_binary_311 = "py_binary") +load("@python_3_11//:defs.bzl", py_binary_311 = "py_binary") load("@rules_python//python:defs.bzl", "py_library") py_library( diff --git a/examples/bzlmod_build_file_generation/MODULE.bazel b/examples/bzlmod_build_file_generation/MODULE.bazel index d69dd7da48..fab2a26a83 100644 --- a/examples/bzlmod_build_file_generation/MODULE.bazel +++ b/examples/bzlmod_build_file_generation/MODULE.bazel @@ -46,14 +46,13 @@ python = use_extension("@rules_python//python/extensions:python.bzl", "python") # This name is passed into python.toolchain and it's use_repo statement. # We also use the same name for python.host_python_interpreter. -PYTHON_NAME = "python" +PYTHON_NAME = "python_3_9" INTERPRETER_NAME = "interpreter" # We next initialize the python toolchain using the extension. # You can set different Python versions in this block. python.toolchain( - name = PYTHON_NAME, configure_coverage_tool = True, is_default = True, python_version = "3.9", diff --git a/examples/py_proto_library/MODULE.bazel b/examples/py_proto_library/MODULE.bazel index 3116c40b2d..feb938da5c 100644 --- a/examples/py_proto_library/MODULE.bazel +++ b/examples/py_proto_library/MODULE.bazel @@ -14,11 +14,10 @@ local_path_override( python = use_extension("@rules_python//python/extensions:python.bzl", "python") python.toolchain( - name = "python3_9", configure_coverage_tool = True, python_version = "3.9", ) -use_repo(python, "python3_9") +use_repo(python, "python_3_9") # We are using rules_proto to define rules_proto targets to be consumed by py_proto_library. bazel_dep(name = "rules_proto", version = "5.3.0-21.7") diff --git a/python/extensions/interpreter.bzl b/python/extensions/interpreter.bzl index b9afe1abda..bbeadc26b8 100644 --- a/python/extensions/interpreter.bzl +++ b/python/extensions/interpreter.bzl @@ -53,7 +53,7 @@ def _interpreter_repo_impl(rctx): actual_interpreter_label = INTERPRETER_LABELS.get(rctx.attr.python_name) if actual_interpreter_label == None: - fail("Unable to find interpreter with name {}".format(rctx.attr.python_name)) + fail("Unable to find interpreter with name '{}'".format(rctx.attr.python_name)) rctx.symlink(actual_interpreter_label, "python") diff --git a/python/extensions/python.bzl b/python/extensions/python.bzl index a604df6ca3..73efeb31de 100644 --- a/python/extensions/python.bzl +++ b/python/extensions/python.bzl @@ -43,11 +43,11 @@ def _left_pad_zero(index, length): def _print_warn(msg): print("WARNING:", msg) -def _python_register_toolchains(toolchain_attr, version_constraint): +def _python_register_toolchains(name, toolchain_attr, version_constraint): """Calls python_register_toolchains and returns a struct used to collect the toolchains. """ python_register_toolchains( - name = toolchain_attr.name, + name = name, python_version = toolchain_attr.python_version, register_coverage_tool = toolchain_attr.configure_coverage_tool, ignore_root_user_error = toolchain_attr.ignore_root_user_error, @@ -56,7 +56,7 @@ def _python_register_toolchains(toolchain_attr, version_constraint): return struct( python_version = toolchain_attr.python_version, set_python_version_constraint = str(version_constraint), - name = toolchain_attr.name, + name = name, ) def _python_impl(module_ctx): @@ -67,38 +67,17 @@ def _python_impl(module_ctx): # toolchain added to toolchains. default_toolchain = None - # Map of toolchain name to registering module - global_toolchain_names = {} - # Map of string Major.Minor to the toolchain name and module name global_toolchain_versions = {} for mod in module_ctx.modules: - module_toolchain_names = [] module_toolchain_versions = [] for toolchain_attr in mod.tags.toolchain: - toolchain_name = toolchain_attr.name - - # Duplicate names within a module indicate a misconfigured module. - if toolchain_name in module_toolchain_names: - _fail_duplicate_module_toolchain_name(mod.name, toolchain_name) - module_toolchain_names.append(toolchain_name) - - # Ignore name collisions in the global scope because there isn't - # much else that can be done. Modules don't know and can't control - # what other modules do, so the first in the dependency graph wins. - if toolchain_name in global_toolchain_names: - _warn_duplicate_global_toolchain_name( - toolchain_name, - first_module = global_toolchain_names[toolchain_name], - second_module = mod.name, - ) - continue - global_toolchain_names[toolchain_name] = mod.name + toolchain_version = toolchain_attr.python_version + toolchain_name = "python_" + toolchain_version.replace(".", "_") # Duplicate versions within a module indicate a misconfigured module. - toolchain_version = toolchain_attr.python_version if toolchain_version in module_toolchain_versions: _fail_duplicate_module_toolchain_version(toolchain_version, mod.name) module_toolchain_versions.append(toolchain_version) @@ -137,6 +116,7 @@ def _python_impl(module_ctx): ) toolchain_info = _python_register_toolchains( + toolchain_name, toolchain_attr, version_constraint = not is_default, ) @@ -182,23 +162,6 @@ def _python_impl(module_ctx): }, ) -def _fail_duplicate_module_toolchain_name(module_name, toolchain_name): - fail(("Duplicate module toolchain name: module '{module}' attempted " + - "to use the name '{toolchain}' multiple times in itself").format( - toolchain = toolchain_name, - module = module_name, - )) - -def _warn_duplicate_global_toolchain_name(name, first_module, second_module): - _print_warn(( - "Ignoring toolchain '{name}' from module '{second_module}': " + - "Toolchain with the same name from module '{first_module}' has precedence" - ).format( - name = name, - first_module = first_module, - second_module = second_module, - )) - def _fail_duplicate_module_toolchain_version(version, module): fail(("Duplicate module toolchain version: module '{module}' attempted " + "to use version '{version}' multiple times in itself").format( @@ -271,10 +234,6 @@ is set as the default toolchain. mandatory = False, doc = "Whether the toolchain is the default version", ), - "name": attr.string( - mandatory = True, - doc = "Name of the toolchain", - ), "python_version": attr.string( mandatory = True, doc = "The Python version that we are creating the toolchain for.", From 7ce22e60ab6b83417562454bd1e3db8aed5e5587 Mon Sep 17 00:00:00 2001 From: Richard Levasseur Date: Wed, 7 Jun 2023 23:49:26 +0000 Subject: [PATCH 2/4] fixup! fix(bzlmod)!: Remove ability to specify toolchain repo name. --- python/extensions/python.bzl | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/python/extensions/python.bzl b/python/extensions/python.bzl index 73efeb31de..9e65ecb72c 100644 --- a/python/extensions/python.bzl +++ b/python/extensions/python.bzl @@ -219,6 +219,12 @@ if the sub module toolchain is marked as the default version. If you have more than one toolchain in your root module, you need to set one of the toolchains as the default version. If there is only one toolchain it is set as the default toolchain. + +Toolchain repository name + +A toolchain's repository name uses the format `python_{major}_{minor}`, e.g. +`python_3_10`. The `major` and `minor` components are +`major` and `minor` are the Python version from the `python_version` attribute. """, attrs = { "configure_coverage_tool": attr.bool( @@ -236,7 +242,7 @@ is set as the default toolchain. ), "python_version": attr.string( mandatory = True, - doc = "The Python version that we are creating the toolchain for.", + doc = "The Python version, in `major.minor` format, e.g "3.12", to create a toolchain for.", ), }, ), From 266f86ef73dc353bfe3dee977949f8c4211e068e Mon Sep 17 00:00:00 2001 From: Richard Levasseur Date: Wed, 7 Jun 2023 23:56:33 +0000 Subject: [PATCH 3/4] fixup! fixup! fix(bzlmod)!: Remove ability to specify toolchain repo name. --- examples/bzlmod/MODULE.bazel | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/examples/bzlmod/MODULE.bazel b/examples/bzlmod/MODULE.bazel index 33cb905bee..40dfb6aed1 100644 --- a/examples/bzlmod/MODULE.bazel +++ b/examples/bzlmod/MODULE.bazel @@ -11,8 +11,8 @@ local_path_override( path = "../..", ) -# This name is passed into python.toolchain and it's use_repo statement. -# We also use the same value in the python.host_python_interpreter call. +# This name is generated by python.toolchain(), and is later passed +# to use_repo() and interpreter.install(). PYTHON_NAME_39 = "python_3_9" INTERPRETER_NAME_39 = "interpreter_39" From bd82b3191b0160664bcb28e9c596ffd03d7cd48d Mon Sep 17 00:00:00 2001 From: Richard Levasseur Date: Thu, 8 Jun 2023 00:57:16 +0000 Subject: [PATCH 4/4] fixup! fixup! fixup! fix(bzlmod)!: Remove ability to specify toolchain repo name. --- .bazelignore | 1 + python/extensions/python.bzl | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/.bazelignore b/.bazelignore index ec504da988..135f709824 100644 --- a/.bazelignore +++ b/.bazelignore @@ -8,3 +8,4 @@ bazel-out bazel-testlogs examples/bzlmod/bazel-bzlmod examples/bzlmod_build_file_generation/bazel-bzlmod_build_file_generation +examples/py_proto_library/bazel-py_proto_library diff --git a/python/extensions/python.bzl b/python/extensions/python.bzl index 9e65ecb72c..bed62305de 100644 --- a/python/extensions/python.bzl +++ b/python/extensions/python.bzl @@ -242,7 +242,7 @@ A toolchain's repository name uses the format `python_{major}_{minor}`, e.g. ), "python_version": attr.string( mandatory = True, - doc = "The Python version, in `major.minor` format, e.g "3.12", to create a toolchain for.", + doc = "The Python version, in `major.minor` format, e.g '3.12', to create a toolchain for.", ), }, ),