Allow "go_deps.module_override" tag in non-root Bazel modules

[aaronmondal]

Gazelle Commit 23226de.
rules_go version 0.39.1 from bzlmod
Bazel 6.2 from nixpkgs
x86_64 Gentoo/Clang

Does this issue reproduce with the latest releases of all the above?

• Module override for circl breaks downstream users eomii/rules_ll#149

Non-minimal reproducer:

git clone [email protected]/eomii/rules_ll
cd rules_ll/examples
nix develop
bazel build @rules_ll//devtools:cluster

What did you do?

After #1521 we have something like this in our MODULE.bazel:

bazel_dep(name = "rules_go", version = "0.39.1")
bazel_dep(name = "gazelle", version = "23226de")
go_deps = use_extension("@gazelle//:extensions.bzl", "go_deps")
go_deps.from_file(go_mod = "//:go.mod")
go_deps.module_override(
    path = "github.com/cloudflare/circl",
    patches = [
        "@rules_ll//patches:go_circl_header_fix.diff",
    ],
    patch_strip = 1,
)
use_repo(
    go_deps,
    "com_github_pulumi_pulumi_kubernetes_sdk_v3",
    "com_github_pulumi_pulumi_sdk_v3",
    "io_k8s_sigs_kind",
)

Unfortunately this causes errors for importers of the module:

ERROR: Traceback (most recent call last):
        File "external/gazelle~23226de/internal/bzlmod/go_deps.bzl", line 189, column 36, in _go_deps_i
mpl
                _fail_on_non_root_overrides(module, "module_override")
        File "external/gazelle~23226de/internal/bzlmod/go_deps.bzl", line 68, column 17, in _fail_on_no
n_root_overrides
                fail(_report_forbidden_override(module, tag_class))
Error in fail: Using the "go_deps.module_override" tag in a non-root Bazel module is forbidden, but module "rules_ll" requests it.

If you need this override for a Bazel module that will be available in a public registry (such as the Bazel Central Registry), please file an issue at https://github.com/baze
lbuild/bazel-gazelle/issues/new or submit a PR adding the required directives to the "directives.bzl" file at https://github.com/bazelbuild/bazel-gazelle/tree/master/internal
/bzlmod/directives.bzl.

This looks like the behavior of something like local_path_override applies to go_deps.module_override which seems wrong. The patch not being allowed to propagate to importers means that importers can't build targets the same way that upstream builds them.

I think it should be fine (and probably default behavior) to allow patches to propagate to importers or somehow bind them to the bazel module that declares them.

I've had similar issues with LLVM where we need to patch the repo but also have downstream users that need the ability to "repatch" it. We currently append the importer patches to the existing ones, which has worked fine so far (https://github.com/eomii/bazel-eomii-registry/blob/11e289ec5b9c2ed9c4ebb0455133ff8455831233/modules/llvm-project-overlay/17-init-bcr.3/patches/00_add_bzlmod_support.patch#L141).

That said, it also seems like a viable solution to just remove the _fail_on_non_root_overrides here:

https://github.com/bazelbuild/bazel-gazelle/blob/31064705f5a481149d060a94adb48cd2c03347b7/internal/bzlmod/go_deps.bzl#L230-L237

cc @seh @fmeum @tyler-french

[fmeum]

All Go modules (transitively) declared in a Bazel project participate in Minimum Version Resolution, so there can only ever be one Go module per module path (in this case github.com/cloudflare/circl). This prevents errors due to duplicated packages and also ensures that Gazelle will know which repo to reference in deps.

All of this breaks if two different modules have a dep on the same Go module, but with different patches applied, which is why we made this a hard failure.

Could you explain your use case in more detail? What are you using the module for - in particular, will it be exposed to other modules as part of a go_library target? Could the patch be submitted upstream? Depending on the answers to these questions, we could come up with alternative approaches to make this work for you.

[aaronmondal]

All of this breaks if two different modules have a dep on the same Go module, but with different patches applied, which is why we made this a hard failure.

For LLVM in rules_ll we apply all patches in order through importers, but that's usually a situation where users know exactly what patches and what external deps are being used in all places and there is little chance for clashing patches. This isn't the case for go dependencies though where one quickly "imports the world".

I wonder whether it would be an option to separate patched go modules in a way that they are only used by the bazel module that declares the patch. Something like "usually everything is reused, but if you patch something you get a duplicate dep". AFAIK this is the approach used by nix. This can make build graphs larger if misconfigured (similar to toolchain transitions), but allows for different variations of deps when needed.

This is originally what I wanted for LLVM, but it wasn't a viable approach there since rebuilds are gigantic. Go has very low compile times though, so duplicating deps might be viable here. It's unclear to me how one would work around the uniqueness of go module paths though.

Could you explain your use case in more detail? What are you using the module for - in particular, will it be exposed to other modules as part of a go_library target?

The target is a go binary to create and configure a local Kubernetes development cluster. It's exposed via nix scripts that invoke bazel run @rules_ll//devtools:cluster. The circl dependency is a transitive dependency that's somewhere in the dependency graph 😂. The breaking binary is a go_binary. This is not intended as dev_dependency but as a dependency that is actually usable (buildable) by importers. At the moment there is no go_library target associated with it, but that might change in the future. There is also nothing depending directly on it in our build files.

It could be the case that we "libraryrize" this binary at some point since importers might want to extend/customize it.

Could the patch be submitted upstream?

I'm not not sure. The patch fixes a cgo dependency that isn't detected by gazelle. It just adds some missing cc_libraries to the build files and add those ad cgo deps. It might be possible to upstream a fix that makes gazelle autodetect the cgo deps better, but I don't know where gazelles limits here are.

However, this might also be a good thing since the actual repo isn't patched at all. It's just the generated build files. There might be more room for "flexibility" for such cases. Maybe a difference between "patching before gazelle" and "patching after gazelle"?

[fmeum]

Thanks for the context, this information is very helpful.

I would propose either of the following, in order of preference:

1. Figure out why Gazelle doesn't pick up the cgo deps and fix that. I can help debug this.
2. Add Gazelle directives that help Gazelle do this if it can't be done fully automatically and add these directives to the registry of defaults that the error message references.
3. Given that the dependency isn't exposed as a library to users by rules_ll, work around this issue by adding the dependency as an http_archive with all BUILD files patched in, plus some Gazelle resolve directives for local development.
4. Think about how to properly handle namespaced drops with patches. I actually expect this to be the most difficult path forward.

What do you think?

[aaronmondal]

Sounds very good! I also agree on your order of preference.

1. Figure out why Gazelle doesn't pick up the cgo deps and fix that. I can help debug this.

Yes. This would be nice if it completely removes the need for any manual patching. I believe the error is raised by this include (and another similar one). There might be some magic go flag that might help gazelle to detect the missing header. I can set up smaller reproducer if that would help. (Though it's basically just invoking gazelle on this go.mod file).

2. Add Gazelle directives that help Gazelle do this if it can't be done fully automatically and add these directives to the registry of defaults that the error message references.

Tbh I'm not familiar enough with gazelle yet to determine how this works, but sounds reasonable. (Edit: Ah do you mean replace in go.mod? Sounds nice.)

3. Given that the dependency isn't exposed as a library to users by rules_ll, work around this issue by adding the dependency as an http_archive with all BUILD files patched in, plus some Gazelle resolve directives for local development.

I've already played around with generating buildfiles with gazelle and patching them in via a bazel module so that gazelle doesn't actually run on module import. That seemed fairly hacky though and required the deps.bzl pattern which didn't seem idiomatic with bzlmod. It also didn't seem nice to maintain.

So this works, but isn't pretty. It's still an option if 1 and 2 don't work out though.

Think about how to properly handle namespaced drops with patches. I actually expect this to be the most difficult path forward.

I can see that changing (or "liberating") the current behavior might have far reaching consequences. This stuff is also notoriously hard to work on since one would probably need (several?) "demo" bazel registries to test it properly.

[fmeum]

That looks sufficiently reproducible that I won't need a separate reproducer. I will take a look.

I've already played around with generating buildfiles with gazelle and patching them in via a bazel module so that gazelle doesn't actually run on module import. That seemed fairly hacky though and required the deps.bzl pattern which didn't seem idiomatic with bzlmod. It also didn't seem nice to maintain.

I had something much simpler in mind: Check out circl locally, run Gazelle on it and make the necessary changes to the BUILD files. Generate a patch out of all of this (including the changes Gazelle made) and load the original archive with this patch as an http_archive from a module extension (similar to https://github.com/bazelbuild/rules_go/blob/f2dcbd619792d9fd6e41bcc6bcb05ff4ce521e3a/MODULE.bazel#L13). If you ignore version resolution, that's essentially what remains of go_deps anyway.

[fmeum]

I looked into this in more detail and don't think that making Gazelle aware of this setup automatically is going to be feasible in the short term - we would need to either export files or generate cc_library targets in other packages, which seems difficult.

Instead, I think that #1526 may help with this. After that functionality has become available, circl could become a proper Bazel module checked into the registry with the patches applied while still providing its Go module to go_deps. @aaronmondal What do you think?

[aaronmondal]

@fmeum Thanks for pointing out #1526. Overriding a gazelle dep with a bazel_dep seems like a good solution.

In the meantime (since I really need to "unbreak" rules_ll for the next release 😅) I'll try overriding @com_github_cloudflare_circl with a http_archive as you mentioned earlier.

[fmeum]

#1526 now has an example of what depending on circl would look like.