module.is_root is turning bzlmod into yet another WORKSPACE

[EdSchouten]

Description of the bug:

Some time last week I migrated Buildbarn from WORKSPACE to MODULE.bazel. This process went smoother than I had expected up front, so kudos to the folks implementing this.

Unfortunately, I came to the conclusion that the MODULE.bazel approach still isn't as 'modular' as I had hoped.

Which category does this issue belong to?

No response

What's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

For example, let's create a toy project that uses rules_rust:

$ cat > MODULE.bazel << 'EOF'
module(name = "foo")
bazel_dep(name = "rules_rust", version = "0.41.1")
rust = use_extension("@rules_rust//rust:extensions.bzl", "rust")
rust.toolchain(edition = "2021")
EOF
$ cat > BUILD.bazel << 'EOF'
load("@rules_rust//rust:defs.bzl", "rust_binary")
rust_binary(
    name = "hello",
    srcs = ["hello.rs"],
)
EOF
cat > hello.rs << 'EOF'
fn main() {
    println!("Hello World!");
}
EOF
$ bazel run //:hello
...
Hello World!

Now let's consider that some user wants to integrate this project into their organisation in the form of a tarball, so they create a Bazel project that looks like this:

cat > MODULE.bazel << 'EOF'
module(name = "bar")
bazel_dep(name = "foo", version = "0.0.0")
bazel_dep(name = "rules_pkg", version = "0.10.1")
# git_override, archive_override, or local_path_override for "foo" goes here.
EOF
cat > BUILD.bazel << 'EOF'
load("@rules_pkg//:pkg.bzl", "pkg_tar")
pkg_tar(
    name = "hello_tar",
    srcs = ["@foo//:hello"],
)
EOF

When they try to build their tarball, they get presented with the following error message:

$ bazel build //...  
...
ERROR: Traceback (most recent call last):
...
Error in fail: 
Your transitive dependency foo is using rules_rust, so you need to define a rust toolchain.
To do so, you will need to add the following to your root MODULE.bazel. For example:

bazel_dep(name = "rules_rust", version = "<rules_rust version>")

rust = use_extension("@rules_rust//rust:extensions.bzl", "rust")
rust.toolchain(
    edition = "2021",
    versions = ["1.70.2"],
)
use_repo(rust, "rust_toolchains")
register_toolchains("@rust_toolchains//:all")
...

So now the user, who couldn't care less that my tool is written in Rust, needs to copy-paste code from the original MODULE.bazel file into their own project, and make sure that that code remains up to date. In my opinion this completely undermines the problem that MODULE.bazel was trying to solve. It adds unnecessary complexity when people try to use Bazel as a meta-build system for tying multiple projects together.

rules_rust isn't the only set of rules that has such weird restrictions. I have also observed this to be an issue with toolchains_llvm.

Which operating system are you running Bazel on?

All of them

What is the output of bazel info release?

7.1.1

If bazel info release returns development version or (@non-git), tell us how you built Bazel.

No response

What's the output of git remote get-url origin; git rev-parse HEAD ?

No response

Is this a regression? If yes, please try to identify the Bazel commit where the bug was introduced.

is_root was introduced here: #15815

Have you found anything relevant by searching the web?

No response

Any other information, logs, or outputs that you want to share?

In my opinion either one of the two following things needs to be done:

1. Non-intrusive: change is_root to return True if and only if it is the root of all modules that depend on a given module instead of the literal root of the project.
2. Deprecate is_root and introduce a way where rules authors can compare modules along a partial order based on parent/child relationships.

In my opinion archive_override and git_override should be given a similar treatment. Namely, it's completely valid for them to get ignored when declared in a child module when the parent also has a dependency on it. But ignoring it if none of the parents/siblings/... depend on it either? That's just cruel.

[fmeum]

Thanks for raising this discussion. I do see great potential for Bazel working well as a meta build system in this way, but I think that this kind of modularity can only be realized if we properly design the domain-specific interactions.

I think that the ultimate root cause of the type of problem you are seeing is that "toolchains are like WORKSPACE": Most toolchains are highly configurable, making different versions hard to reconcile, and registrations have "first registered wins" semantics. Many module extensions, apparently including rules_rust and also toolchains_llvm, have taken the if not module.is_root: fail or ignore approach - most likely because they don't want to commit to any particular alternative approach for toolchain management yet. This seems prudent to me since it allows for improving modularity later without potentially backwards incompatible changes.

A pattern that I could see working here is to:

• define and register a "good enough" default toolchain in rules_rust that is updated with rules_rust versions and can be relied upon by all modules without them having to register any toolchain of their own and
• allow the root module to register and finetune rust toolchains as needed.

This sidesteps the issue of reconciling toolchain requirements by multiple transitive dependencies while still allowing rulesets to use Rust tools without manual setup required from the root module. Would that work in your case? @illicitonion @UebelAndre in case they want to provide their input on this.

Non-intrusive: change is_root to return True if and only if it is the root of all modules that depend on a given module instead of the literal root of the project.

While that works in your particular example, it looks to me as if this would stop working in the typical diamond situation with dependency chains A -> B -> rules_rust and A -> C -> rules_rust. This is an example of what I described above: If B and C have opinions about Rust toolchains, it's essentially impossible to reconcile them automatically. Instead, either A is tasked with defining a toolchain (the current situation) or rules_rust provides a toolchain good enough for B and C and tunable by A if desired (the "improved" approach described above).

Deprecate is_root and introduce a way where rules authors can compare modules along a partial order based on parent/child relationships.

While it's true that module extensions can currently only query the BFS order of modules using them and not the dependency edges between them, I'm not sure whether this extra information is what extensions are missing to become more modular: It already seems difficult to reconcile the fully featured toolchain requirements of two module that aren't comparable with respect to this partial order. I would rather think that domain-specific information or restrictions (say, editions/language versions/etc. instead of arbitrary toolchain features) could help do better here (say use Minimum Version Selection on the toolchain versions).

[illicitonion]

I think that the ultimate root cause of the type of problem you are seeing is that "toolchains are like WORKSPACE": Most toolchains are highly configurable, making different versions hard to reconcile, and registrations have "first registered wins" semantics. Many module extensions, apparently including rules_rust and also toolchains_llvm, have taken the if not module.is_root: fail or ignore approach - most likely because they don't want to commit to any particular alternative approach for toolchain management yet. This seems prudent to me since it allows for improving modularity later without potentially backwards incompatible changes.

A pattern that I could see working here is to:

• define and register a "good enough" default toolchain in rules_rust that is updated with rules_rust versions and can be relied upon by all modules without them having to register any toolchain of their own and
• allow the root module to register and finetune rust toolchains as needed.

This sidesteps the issue of reconciling toolchain requirements by multiple transitive dependencies while still allowing rulesets to use Rust tools without manual setup required from the root module. Would that work in your case? @illicitonion @UebelAndre in case they want to provide their input on this.

cc @matts1 who probably has the most context here, and introduced this requirement in bazelbuild/rules_rust#2021 including this snippet:

https://github.com/bazelbuild/rules_rust/blob/1a053cfbf9a28ef09a5594bf69a9360c8e59bc61/rust/extensions.bzl#L42-L49

    # Toolchain configuration is only allowed in the root module.
    # It would be very confusing (and a security concern) if I was using the
    # default rust toolchains, then when I added a module built on rust, I was
    # suddenly using a custom rustc.
    root = _find_root(module_ctx)

    if not root:
        fail(_TRANSITIVE_DEP_ERR % module_ctx.modules[0].name)

[matts1]

I definitely understand your issues with it. I do also take issue with it, but I don't currently have any ideas that can improve on it - I'm open to suggestions.

I believe that the biggest problem with WORKSPACE.bazel is that it didn't work very well when you had collisions, and implementing your idea as-is would simply make those collisions come back.

To me, there were pros and cons of doing it either way. I'll outline a few of them:

Pros of explicitly doing it in root workspace:

• Can easily make implicit default toolchains implicit later down the line if we decide it's better
• Completely unambiguous - if you have multiple packages depending on rules_rust, you know that there is still only a single registered toolchain.

Pros of implicitly using the toolchain from the modules that you depend on:

• Simpler to write / less verbose
• Don't have to think about your transitive dependencies

The minimum version selection on the toolchain versions is an interesting idea, and might work for simpler use cases, it starts failing when you do anything more complex.

• We have a few rust toolchains that we use, some of which define custom sets of rustc flags, and so merging the toolchains would be bad, and having them fight and have one win would result in spooky action at a distance based on the order in which you declared your bazel deps.
• If we have the diamond dependency root -> foo, bar -> rules_rust, I could see a world where we want to have a transition to foo's rust toolchain whenever we depend on a target in foo, and a transition to bar's rust toolchain whenever we depend on a target in bar.

I think my conclusion is that what you're suggesting would indeed make things slightly simpler for the simple use cases, but it'd make things much harder for the more complex use cases. I personally don't think the tradeoff is worth it, but if anyone has suggestions that make this easier.

FWIW, in rules_rust, we only allow the root module to configure the toolchain, but IIRC there is a default configuration. So you should just need to register that default toolchain. I haven't ever tried your use case, so I'm not actually too familiar with how it currently works.

[Kevintjeb]

We hit this issue in rules_jsonnet in the following PR.

Our goal is to allow users to use either a Go, Cpp or Rust based Jsonnet
compiler. Depending on which compiler the user picks, Bazel will then correctly
compile it for them. This has been working nicely while rules-jsonnet only
supported Go and Cpp implementations.

The PR introduced Rust, and thus also rules_rust to be able to compile it.
To my surprise I observed the following behaviour for the users of this patch:

• Users of the module rules_jsonnet must now always include a rules_rust
  toolchain. Regardless if they're using Rust or not.
• Including a rules_rust dependency and toolchain in rules_jsonnet is not
  enough, as the rules_rust implementation requires the root module to define the
  toolchain.
• Not using anything related to Rust requires a toolchain to be specified. A
  bazel_dep(name = "rules_rust") is enough to trigger the error.
• A great error message from rules_rust on what to add and where. That was nice!

Unfortunately this behaviour blocks us from merging the PR, because we don't
want to introduce a breaking change / make users aware of our dependency
(transitive for them) on rules_rust if they only use the defaults of
rules_jsonnet with the Go compiler.

I understand that the current setup in rules_rust is a sane default, as this
allows for improvements down the line without breaking backwards compatibility.
However, I wonder what we can do to improve the setup for e.g. rules_jsonnet,
where it is similar to rules_go for example:

• rules_jsonnet has a dependency on rules_go, because there is a Go Jsonnet
  compiler. If the user registers the intent to use the Go compiler, then
  rules_go will use the default toolchain (unless specified otherwise by the
  user) to compile it. When the user does not use Go but the Cpp implementation,
  the rules_go module does not throw an error.

We're open to a temporary solution that allows us to move forward with
rules_jsonnet and not put pressure on rules_rust to redefine how toolchains are
supposed to work with Bzlmod. I'd also be keen to chat with the rules_rust
maintainers as a user and see how we can improve the Bzlmod setup as a whole.

[meteorcloudy]

I converted this to a discussion since it's not really a Bazel issue.

[matts1]

So I've been thinking about this, and here's what I think could be a set of reasonable policies:

Every module that defines a new toolchain type must also register a non-dev default toolchain for that toolchain

Rules_rust introduces toolchain types for rust, rust_proto, rust_bindgen, and one or two others. Thus, it must then call
register_toolchain("@rust_default_toolchains//:all") or similar.

If a project defines a new toolchain for which the toolchain type was defined by another module, it must register it as a dev toolchain

If my project uses rules_rust, it may register its own custom rust toolchain. However, it must do so by invoking register_toolchains(my_rust_toolchain, dev_dependency = True).
If a project such as rules_jsonnet wanted to define their own custom rust toolchain, they could do so, and then if the root module wanted to use it, they would then write register_toolchain("@rules_jsonnet//:custom_rust_toolchain", dev_dependency = True).

The toolchain configuration of a toolchain may only be affected by the module defining said toolchain type, and the root module

That is, if a user in rules_jsonnet writes the following (which modifies the default toolchain), then any user of rules_jsonnet should not be affected by it (it should only apply when rules_jsonnet is the root module).

rust.toolchain(edition = "2021", version = "1.70.0")

In practice, this means that any toolchain module extension should look like:

def _toolchain_impl(module_ctx):
    # Maybe we should add module_ctx.root_module
    root_module = [module for module in module_ctx.modules if module.is_root][0]
    # define toolchain based on root_module alone, grabbing defaults, ignore all other module_ctx.modules

_toolchain = tag_class(
    attrs = {
        "version": attr.string(default = DEFAULT_VERSION),
        ...
    }
)

toolchain = module_extension(
    implementation = _toolchain_impl,
    tag_classes = {"toolchain": _toolchain},
)

Enforcement

@meteorcloudy How would you feel about adding a --incompatible_<something> flag to enforce the first two policies? Can you think of any scenarious where one might not want to do such a thing.

The third one seems difficult to enforce.

[groodt]

I think it would be a great and necessary addition. Who would be best placed to raise a PR? I would do it myself, but I feel I would probably miss some of the depth and detail that others in this discussion would provide.

[rickeylev]

FWIW, in rules_python, we came to a similar conclusion as what matts1 described a couple posts up.

The rules_python module registers a "default" python toolchain. The "default" toolchain is special: it has lowest priority (i.e. sorts last in :all) and lacks python version constraints (i.e. target_compatible_with doesn't include the python_version flag).

(IMHO, every language module should register some default toolchain. If not, then the transitive affects show up, as shown by the rules_rust situation).

Only the root module can override what the default is.

Intermediate modules can still call python.toolchain(python_version=...), and they are registered as toolchains, but they are aren't registered as the default toolchain. Instead, these toolchains are registered with the python version constraints set. If a module wants to use those toolchains, they have to specify e.g. python_version=... on their target so it matches the toolchain's settings.

No MVS is performed, for two reasons. The first is there isn't really a need to in this setup. If an intermediate module wants to use a specific version, it ties itself to that using its targets. (we might do MVS over patch-level specificity in the future, IDK).

The second reason is there's incomplete information to perform MVS. Anyone, anywhere, could register a toolchain. A bzlmod extension can only see what went through its tags. So you can perform MVS, but you don't have a strong guarantee that you'll actually get one of those. The root module, another module earlier in BFS, etc could have performed its own registrations, and you'd never know.

I used to fret a lot about different modules calling python.toolchain() and what to do until I realize that last point. Toolchains are, by design, weak and late bound abstractions, precisely so they can be customized. If a module wants to register a toolchain, and will accidentally muck up everyone downstream, you can't really stop it from doing so.

[tpudlik]

Did the best practices proposed here ever get documented somewhere?

[tpudlik]

There's a question behind my question :). We might want to amend the best practices to specify that the root module should be capable of disabling the default toolchain registration carried out by modules that define new toolchain types. See bazelbuild/rules_rust#2818.

[meteorcloudy]

I believe adding a new section next to https://bazel.build/extending/toolchains#registering-building-toolchains would be nice, if anyone can summarize this into a PR, we can also continue the discussion and reviewing there!

[jacky8hyf]

Hello, what is the conclusion here?

Android kernel build is also using rust on android15-6.6, and we found it difficult to use if all users of the kernel build tools must also put a rust.toolchains() declaration in their MODULE.bazel file. What's the best way to avoid users of our module (that has a dependency on rules_rust) to also call rust.toolchains()?

[fmeum]

Cc @illicitonion

[matts1]

Is this still an issue for you with rules_rust 0.44.0?

It should have been solved with bazelbuild/rules_rust@29ec07a

[matts1]

FYI, if you define a custom toolchain in android kernel build, users won't need to use rust.toolchains, but they will need to add the following to their MODULE.bazel file:

register_toolchains("@android_kernel//path/to:rust_toolchain")

I don't think we're gonna ever give you anything more convenient than that, since that would leave us open to the "spooky action at a distance" problem described earlier.

[jacky8hyf]

Thanks for the suggestion. rules_rust 0.44.0 introduced numerous new bazel_dep()'s that we need to properly locally vendor by importing them to the source tree:

@rules_go
@aspect_bazel_lib
@rules_nodejs
@rules_buf
@bazel_features 

Notably, bazel_features. I'll revisit this once we can get bazel_features properly imported. (The internal bug number is 340944112.)

[armandomontanez]

It sounds like part of the conclusion here is that register_toolchains() should default to dev_dependency=True.