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.

[matts1]

I don't think that's necessarily the right choice

Your points here seem pretty reasonable to me. My main concern here, however, is spooky action at a distance. When I suddenly include rules_jsonnet, I have significant concerns if that randomly decided to modify my toolchains.

Here's a few proposals from me on how we might deal with it.

Option 1: Fail if root_module != MVS(modules)

# root_module/MODULE.bazel
rust.toolchain(
    features = ["foo"]
)

# rules_rust/MODULE.bazel
rust.toolchain(
   version = "1.70",
)

# rules_jsonnet/MODULE.bazel

rust.toolchain(
    features = ["bar"],
    version = "1.80",

When you add a bazel_dep on rules_jsonnet, it would:

1. Perform MVS, discovering that we need toolchain(version = "1.80", features = ["foo", "bar"])
2. Discover that the root module specifies toolchain(version = "1.70", features = ["bar"]) (version number would be inherited from rules_rust).
3. It would then compare the two, and output an error along the lines of:

Problems with your rust toolchain:
* rules_jsonnet requires a rust toolchain with version 1.80
* rules_jsonnet requires the features ["bar"]

We recommend adding the following to your root MODULE.bazel:
rust.toolchain(
    version = "1.80",
    features = ["foo", "bar"],
)

Option 2: Warning if root_module != MVS(modules)

Or perhaps instead, we take the approach that bzlmod itself took. We do the same as above, but instead of outputting an error, we actually apply the MVS and then give the user that error, but give it as a warning.

WARNING: For repository 'bazel_skylib', the root module requires module version bazel_skylib@1.4.2, but got bazel_skylib@1.5.0 in the resolved dependency graph.

Option 3: Apply MVS, no warning

I don't really like this option due to spooky action at a distance

Option 4: No MVS

I think this will always need to be an option - sometimes a user will want to disable it.

Option 5: Toolchain tag to decide what to do

rust.toolchain(
    version = "1.80",
    # options could be:
    # * mvs_or_fail (option 1)
    # * mvs (option 2)
    # * mvs_no_warning (option 3)
    # * disable (option 4)
    toolchain_merge = "mvs_or_fail"

How to merge

All that being said, merging toolchains with MVS gets very confusing / difficult when you can have multiple toolchains. I suspect it's relatively common to have different toolchain configurations for different platforms, but it may be quite reasonable to even do so for a single platform.

rust.toolchain(
    version = "1.70",
   # Some packages are using the old toolchain, and others are using the new.
   target_settings = ["//:use_old_toolchain"],
)

rust.toolchain(
   version = "1.80",
)

Maybe we just take the approach where if the user defines multiple toolchains, we just don't merge them?

[fmeum]

I think it's fine to do MVS to obtain a default toolchain, but if the root module defines a toolchain, that one should always take priority (and possibly emit a warning if it lacks some features transitive modules request). What do you think about that?

[matts1]

I think that would address my concerns. So the policy would be, for a given toolchain defined by rules_foo which has the tag foo.toolchain:

• If the root module defines a toolchain tag, the toolchain configuration will be:
  • Configured based on the root MODULE's foo.toolchain tag
    • Any unspecified fields may be inherited from rules_foo's foo.toolchain
    • Any specified fields must not be affected by rules_foo's foo.toolchain
    • The toolchain configuration must not be affected by any module other than the root module and rules_foo
  • If MVS is supported, rules_foo may choose to compare the root MODULE's foo.toolchain with MVS(all modules' foo.toolchain) in order to output warnings
• If the root module doesn't define a toolchain, the toolchain is defined by:
  • If MVS is supported, MVS(all modules' foo.toolchain invocations)
  • Otherwise, rules_foo's foo.toolchain
• Non-leaf modules should specify the minimum possible versions and minimal set of features in their foo.toolchain

That leaves you open to spooky action at a distance if you don't define your own toolchain, but I personally think that's ok - if a user never explicitly specified toolchain configuration, it probably means they don't care enough about it.

[fmeum]

That leaves you open to spooky action at a distance if you don't define your own toolchain, but I personally think that's ok - if a user never explicitly specified toolchain configuration, it probably means they don't care enough about it.

"Reasonable defaults" and "spooky action at a distance" are just different ways of expressing the same behavior in this case, so I tend to agree.

[meteorcloudy]

I think those best practices could be implemented as an example in https://github.com/bazel-contrib/rules-template, and we should link to it from Bzlmod documentation. /cc @Wyverald

[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.)