feat(bzlmod): Allowing multiple python.toolchain extension calls

[chrislovecnm]

We do this work for two reasons.

First, we must support Module dependencies and sub-modules using python.toolchain. There are already two known instances of sub-modules setting up a Python toolchain and colliding with another module (nanobind and rules_testing both run into this).

Second, the upcoming multi-version support is going to work by having each python.toolchain() call register its particular version with the extra toolchain constraint. This also helps unify the version-aware and non-version-aware code paths (the non-version aware paths are just version-aware with a single version registered as the default)

This commit implements various business logic in the toolchain class.

Toolchains in Sub Modules

It will create a toolchain in a sub-module if the toolchain
of the same name does not exist in the root module. The extension stops name
clashing between toolchains in the root module and sub-modules.
You cannot configure more than one toolchain as the default toolchain.

Toolchain set as the default version.

This extension will not create a toolchain in a sub-module
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.

See #1229 for more information

[rickeylev]

The order the repos are created in doesn't matter, it's the order they are seen when register_toolchains() encounters them. i.e. the order of the toolchain_names list needs to put the default toolchain at the end of the list. This doesn't matter right now because of who calls register_toolchains, so I'm fine with just a TODO comment about this for now, too.

Something for another PR: The repo for the default toolchain is essentially a copy of one of the others. I think we can avoid this by just having the hub generate extra toolchain() calls that point to the same targets in the backing repo.

[chrislovecnm]

Please explain more

[rickeylev]

There's two aspects to this: ordering, and compatiblity.

Usually the order targets are defined in doesn't matter, but toolchains are an exception. Basically, all the native.toolchain targets that are found via register_toolchains() calls are searched in the order they're encountered.

For example, say we had this build file:

toolchain(name="foo", ...)
toolchain(name="bar", ...)

And this module:

register_toolchains("//:all")

This will record a list of [foo, bar]. When someone asks for a toolchain, it'll first check foo, and, if it's compatible, will use it. Otherwise it moves onto the next item in the list.

If we change the BUILD file to put "bar" before "foo", bar will be checked first. If we change the register_toolchains call to explicitly name the targets, e.g. register_toolchains("//:bar", "//:foo"), that will also make bar checked first.

Next, compatibility: when iterating over the toolchains, the first compatible toolchain is used, where "compatible" means "are all of the toolchain's constraints satisfied?". For example, say we had this build file:

toolchain(name="default_linux", compatible_with=["//os:linux"])
toolchain(name="py38_linux", compatible_with=["//os:linux", "//py:py38"])

And we do register_toolchains("//:all"), so the ordering is [default_linux, py38_linux]. Note how the first toolchain has fewer constraints than the second -- it just cares about matching linux.

When a multi-version aware rule is used, it's constraints are (os=linux, py=py38). We want it to use the py38_linux toolchain. But, default_linux is checked first and will be matched because all its constraints (it just needs linux) are satisfied, and thus it uses the wrong toolchain.

The toolchain docs explain the algorithm in detail: https://bazel.build/extending/toolchains#toolchain-resolution

[fmeum]

I think that targets within a package are ordered lexicographically, not in declaration order. That's why rules_go appends a prefix such as 0001_ to all toolchain definitions.

[rickeylev]

You're right! Is that documented somewhere? I couldn't find a mention of :all or the lexicographic ordering it uses.

[fmeum]

I don't think so, @Wyverald dug this out of the code when I asked him about it.

[Wyverald]

https://cs.opensource.google/bazel/bazel/+/refs/heads/master:src/main/java/com/google/devtools/build/lib/packages/Package.java;drc=665a3241f4acf2291d157c76a413d872ae59241e;l=704

All the target pattern resolution stuff eventually go to Package.getTargets(), and it's an ImmutableSortedSet on the target name.

[chrislovecnm]

@Wyverald @fmeum @rickeylev Are these changes that I should have in this PR? If so can someone do a code review?

[rickeylev]

@chrislovecnm No, we don't need to address all of this in this PR because there isn't a hub for rules_python to call register_toolchains on.

The changes for this PR are:

1. Remove L99, the python_register_toolchains() call for the default toolchain.
2. Remove the skip-if-default logic up around L56 (so that the repo for the default version is still created)

When we do the PR to automatically register toolchains, then we'll need to worry about what this comment thread is about.

[rickeylev]

Mostly LGTM.

This extension will not create a toolchain in a sub-module if the sub-module toolchain is marked as the default version.
TODO: should we create the toolchain anyways,
but set the default version to False?

Yeah, I'm not sure about this. My main concern is, because the repo will sometimes exist and sometimes not, it'll cause headaches. There are two cases I have in mind:

Case 1: Leaky development-only dependencies. I'm doing development. I add python.toolchain(X, default=True); use_repo(X). I run my tests, things work. I deploy. My users break, because my use_repo(X) call fails -- X was never created. I should have marked something dev_only=True, but forgot; instead of some extra work being done, things have broken.

Case 2: Some non-dev-only reason you want the repo, but don't need it to be registered as a toolchain? I don't know what this reason would be....Aha -- won't pip.parse stop working? For example, my module might have a tool, and my tool has dependencies, which it gets through pip.parse() and python_interpreter_target, so the toolchain repo has to be available.

Ok, yeah, I think we need the repo to be created. Just don't let it be registered as a toolchain when we move the register_toolchains() call into rules_python itself. So in this PR, create the repo. In the PR when we refactor where register_toolchains() is called, update the logic to respect this.

This does expose another issue, though: the submodule will use pip.parse() with one interpreter, but its py_binary targets might end up using a different one through toolchain resolution. I think the best we can do here is provide a "default interpreter" repo, so that the sub-module has a way to pass the same interpreter the default toolchain will use into its pip.parse calls. (Something for another PR, though). A default-interpreter repo might also simplify how much config the user has to do in their MODULE file? Haven't thought deeply here.

cc @fmeum as a second pair of eyes on my thinking here.

If you have more than one toolchain in your root module, you need to set one of the
toolchains as the default version.

For posterity, recap of the convo Chris and I had:

Having a dependency allowed to change your Python version is bad because that dependency might be far away. A consumer might unexpectedly change their version updating a seemingly unrelated dependency. A producer might inadvertently break someone changing their python version. Neither has a good insight into the transitive effects.

We talked about the idea of taking the highest version of any toolchains marked default -- we rejected this idea for two reasons. The first is the same rationale as above. The second is: if you're introducing a new python version to a large project, it's beneficial to get the toolchain setup and installed, but leave it in an unused-by-default state. This allows work to progress to test and validate things incrementally. When you're finally ready, you can switch the default.

[chrislovecnm]

Need to update the example.

[rickeylev]

Mostly LGTM; a couple doc nits. Per chat: one of those CI failures looks legit. I've retried it 4 times now and it keeps failing. It's odd that its only failing on windows, though. Our guess is due to the multiple versions in the example; lets just pull that out for now.

I'll give approval so you can merge when ready.

Two closing thoughts; these don't need to be addressed in this PR. I'm just posting them for posterity.

First: I think the creation of the default repo is sort of wrong, but things end up working out anyways. Basically, because it's not being created with version_constraint=True, there's no toolchain that is multi-version aware. However, all the others are, and the default ends up last. The net effect is looking for the same version as the default will fail to match all the versioned toolchains, then land on the default, and end up with the proper toolchain. This is fine for now -- no need to address it in this PR; when we create the toolchain repo, we can address it in that PR.

Second: I'm think we should have a way for the default interpreter to be referenced by the interpreter extension. Because: lets say you're a sub module and you're OK with using the default python version. How do you configure your pip.parse() call to use it? You can't -- you don't know what the root module is using for the default. So we should address that somehow.

[aignas]

nit: I find the usage of constants here making it harder to read. Even though the definition and the usage of the constants is nearby, my eyes need to jump to lines 14 and 16 when reading line 28. Having the text verbatim would be better at the expense of very little duplication.

[chrislovecnm]

If you look at the example now, we now have multiple versions, and I find that having variables causes me less grief because of typos. Do you have a compromise?

[aignas]

nit: consider inverting the if statement and using continue statement to reduce the indentation for the code that matters.

[chrislovecnm]

Please provide more context.

[aignas]

minor: having _interpreter_hub_repo = "hub_repo" here would make the code below more readable.

[chrislovecnm]

The naming of all of this may need to change. The interpreter is just one part of the hub. We will have repos for all of the interpreter symlinks, and also we will have a hub for all of the toolchains. So I am having a problem with the naming in general. @rickeylev thoughts?

[rickeylev]

I think "interpreter hub" and "toolchain hub" are sufficiently clear.

"interpreter hub" isn't great, but we couldn't think of a better name for it. The interpreter hub name is internal, so we can rename it if we think of a better name.

[aignas]

Left some comments on readability. Thanks for the PR, in general LGTM other than the gazelle example modifications which you commented on yourself.

[chrislovecnm]

So this PR is passing CI now, besides a known OSX flake. For some reason, I am getting an error locally on Windows that I saw in a flake earlier. Moreover, the example/bzlmod/.bazelrc did not enable Windows symlinks, so it should not have passed CI. I am confused. cc: @rickeylev @aignas

I have tried:

bazelisk.exe --output_user_root=C:/b build ...

and

 bazelisk.exe --output_user_root=C:/b build --flaky_test_attempts=3 --local_test_jobs=8 --show_progress_rate_limit=5 --curses=yes --color=yes --terminal_columns=143 --show_timestamps --verbose_failures --jobs=30 --announce_rc --experimental_repository_cache_hardlinks --disk_cache= --sandbox_debug --subcommands=pretty_print ...

And I am getting:

ERROR: An error occurred during the fetch of repository 'rules_python~override~pip~pip_requests':
   Traceback (most recent call last):
        File "C:/b/r4du22do/external/rules_python~override/python/pip_install/pip_repository.bzl", line 705, column 13, in _whl_library_impl
                fail("whl_library %s failed: %s (%s)" % (rctx.attr.name, result.stdout, result.stderr))
Error in fail: whl_library rules_python~override~pip~pip_requests failed:  ()
ERROR: <builtin>: fetching whl_library rule @rules_python~override//python/extensions:rules_python~override~pip~pip_requests: Traceback (most recent call last):
        File "C:/b/r4du22do/external/rules_python~override/python/pip_install/pip_repository.bzl", line 705, column 13, in _whl_library_impl
                fail("whl_library %s failed: %s (%s)" % (rctx.attr.name, result.stdout, result.stderr))
Error in fail: whl_library rules_python~override~pip~pip_requests failed:  ()
ERROR: C:/b/r4du22do/external/rules_python~override~pip~pip/BUILD.bazel:286:6: @rules_python~override~pip~pip//:requests_pkg depends on @rules_python~override~pip~pip_requests//:pkg in repository @rules_python~override~pip~pip_requests which failed to fetch. no such package '@rules_python~override~pip~pip_requests//': whl_library rules_python~override~pip~pip_requests failed:  ()
ERROR: Analysis of target '//:all_requirements_14__deps' failed; build aborted:

[rickeylev]

Thanks for the review, @aignas. We're going to leave the aesthetics stuff for a separate PR so we can get this base functionality merged. There's still several functional pieces we need to get done to make the bzlmod support actually usable.