Skip to content

Commit

Permalink
fix(bzlmod)!: Remove ability to specify toolchain repo name. (#1258)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
rickeylev authored Jun 8, 2023
1 parent 9374021 commit b228f60
Show file tree
Hide file tree
Showing 9 changed files with 27 additions and 74 deletions.
3 changes: 3 additions & 0 deletions .bazelignore
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,6 @@ bazel-rules_python
bazel-bin
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
2 changes: 1 addition & 1 deletion examples/bzlmod/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -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")

Expand Down
13 changes: 4 additions & 9 deletions examples/bzlmod/MODULE.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -11,24 +11,20 @@ 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.
PYTHON_NAME_39 = "python_39"
# 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"

PYTHON_NAME_310 = "python_310"
PYTHON_NAME_310 = "python_3_10"

INTERPRETER_NAME_310 = "interpreter_310"

# We next initialize the python toolchain using the extension.
# 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,
Expand All @@ -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",
)
Expand Down
12 changes: 2 additions & 10 deletions examples/bzlmod/other_module/MODULE.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
2 changes: 1 addition & 1 deletion examples/bzlmod/other_module/other_module/pkg/BUILD.bazel
Original file line number Diff line number Diff line change
@@ -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(
Expand Down
3 changes: 1 addition & 2 deletions examples/bzlmod_build_file_generation/MODULE.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
3 changes: 1 addition & 2 deletions examples/py_proto_library/MODULE.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -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")
2 changes: 1 addition & 1 deletion python/extensions/interpreter.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -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")

Expand Down
61 changes: 13 additions & 48 deletions python/extensions/python.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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):
Expand All @@ -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)
Expand Down Expand Up @@ -137,6 +116,7 @@ def _python_impl(module_ctx):
)

toolchain_info = _python_register_toolchains(
toolchain_name,
toolchain_attr,
version_constraint = not is_default,
)
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -256,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(
Expand All @@ -271,13 +240,9 @@ 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.",
doc = "The Python version, in `major.minor` format, e.g '3.12', to create a toolchain for.",
),
},
),
Expand Down

0 comments on commit b228f60

Please sign in to comment.