Language implementations cannot configure loads based on config

[illicitonion]

Background / Feature Request

Currently, gazelle has exactly one place where config is allowed to affect per-build-file generated loads, and that's using map_kind.

There are use-cases in practice for being able to do this more flexibly from arbitrary language plugins. For instance, say in Java you wanted to generate java_test targets most of the time, but if a test is marked as flaky (perhaps by annotating its class with an @Flaky annotation, as suggested in https://github.com/bazelbuild/bazel-gazelle/pull/1293#issuecomment-1171525421), you want to generate a flaky_java_test target, or if a test is marked as requiring network access (again, noted by annotation), you want to either generate network_using_java_test target.

In particular, this may not be a static global property of a plugin; for a shared common Java plugin, different teams may have different conventions requiring custom mappings (e.g. they may define their own target types they want to generate for). In a multi-team monorepo, these may even be per-directory conventions.

In the Java gazelle plugin, we wish to make a flexible configuration mechanism, where someone can in a BUILD file write something like:

# gazelle:java_annotation_custom_rule com.example.annotations.Flaky @my_repo//rules:defs.bzl flaky_java_test

And to be able to customise what rules are generated as a result.

The Problem

Unfortunately, a language.Language must statically declare up-front all of the kinds/loads it may generate. Languages can be configured globally using flags, but don't have any way of varying the kinds/loads they generate based on the contents of BUILD files, which means teams can't make their own local choices.

Proposed Solution

Ideally, language.Language.Kinds() and language.Language.Loads() would take arguments: c *config.Config, rel *string to allow per-directory configuring to affect the output of Kinds() and Loads().

But this is a breaking change! There are a couple of approaches I can reasonably imagine:

1. Just add the arguments. The functions are actually called in very few places (all of which in Gazelle have the required information to hand), and the migration of "adding two arguments you ignore" for each language.Language implementation isn't terribly complex. But it is churn.
2. Add a second interface, and where we call Loads()/Kinds(), sniff whether the other interface is implemented, and if so, call that instead.

I'd prefer 1, but understand if it's too drastic. But in the long-term, I think we should converge on passing Config to these functions.

I'd also be open to any other ways of achieving this same aim, but this was the cleanest I could work out.

Sample implementation

I put together a sample implementation in https://github.com/illicitonion/bazel-gazelle/commits/config-dependent-loader (just the top commit) which works for my aims; it includes an integration test showing the key behaviour in tests/diff_mode_with_config. I'd be happy to put this together as a formal PR if there's no better approach.

[illicitonion]

/cc @achew22 @sluongng @linzhp

[linzhp]

I saw you are experimenting different approaches to skip flaky tests. Uber did quite a bit too with Go. If I understand correctly, you want to skip some tests in a package, but don't want to skip the whole test target, right? So you would mark the tests that you want to skip with @Flaky. Then you want Gazelle put those classes in flaky_java_test and the rest in java_test. Is that right?

[illicitonion]

Yeah, that's exactly right @linzhp.

[sluongng]

I think (1) is definitely a bit too disruptive.

I think after https://github.com/bazelbuild/bazel-gazelle/commit/622d8889c63227a71d6b393ba5e3a8b8a6761466, option (2) is feasible.

We would want to advise folks to compose BaseLang into their own struct implementation. BaseLang should contain a sensible default implementation for LoadsWithConfig(...) and KindsWithConfig(...) (actual names pending™️) so that on Gazelle version upgrade, folks would just need to add BaseLang into their struct for an easy, quick fix.

 type MyAwesomeLang {
+   language.BazeLang
 }

We can then mark Loads() and Kinds() as deprecated, rename them in the subsequent patches to OldLoads() and OldKinds() before eventual deletion. This migration would require some engineering hours to drive within Gazelle code base plus release notes documentation, but definitely not that much 🤔

Implementing a new interface definitely would be a more backward compatible approach, but I think that would have resulted in a fragmented end state where end user would have more interfaces to worry about during implementation for no apparent benefits.

Gazelle is still before 1.0.0 so breaking change could be accepted, though we definitely should not abuse that ability. 👀

[illicitonion]

Thanks for the thoughts!

BaseLang should contain a sensible default implementation for LoadsWithConfig(...) and KindsWithConfig(...)

This generally sounds reasonable, except that I'm not sure what that sensible default implementation would look like...

I tried implementing this, but because Go doesn't have overridable virtual functions, just composable structs, I don't think there's a way to have BaseLang.KindsWithConfig call goLang.Kinds, unless BaseLang has a self pointer to a language.Language it can use to do the virtual dispatch on (and there's no automatic way to make that happen, we'd need to have a fiddly BaseLang.Init(Language) function we'd require folks call)...

Do you know if a way to have BaseLang's functions be overridable, when called from inside other functions in BaseLang?

This migration would require some engineering hours to drive within Gazelle code base plus release notes documentation, but definitely not that much 🤔

I'm happy to put in the hours, as long as we can work out how to make it work!

Gazelle is still before 1.0.0 so breaking change could be accepted, though we definitely should not abuse that ability. 👀

One additional piece of fun here is the bzl gazelle implementation in skylib, which effectively is a circular dependency, but we can work out these details!

[linzhp]

I think changing the build graph to skip some flaky tests is quite disruptive and yet not flexible enough. The granularity is only at file/class level. You have to disable all test cases in a file even though some of them are not flaky. In addition, the build cache for the test targets are invalidated whenever you want to disable or enable some of the test classes.

Ideally, skipping flaky tests should be a runtime configuration, not a build time configuration: we build the same test binary, and conditionally run some tests out of it. For example, in Uber's Go monorepo, land-blocking jobs skips all unit tests, and a separate non-blocking job run flaky tests. There is also a post land job that run all tests to detect flaky tests. These jobs share the same test binary fetched from remote cache. They read a configuration file at runtime that contains all known flaky test cases and a flag to decide whether to skip them or not.

We did this by patching rules_go right now, but eventually when https://github.com/bazelbuild/rules_go/issues/2785 is implemented, we can use Bazel --test_filter to do that. I think you should also implement --test_filter for Java to skip test cases. With that, you CI system can read some configuration file build a --test_filter. No changes to source code or build graph is needed to skip or enable flaky tests.

[illicitonion]

Sorry, I simplified our use case to the example of flaky tests (which I agree doesn't necessarily pull its weight as motivation) - a better example for us than flaky tests is tests that require external network, because that requires encoding specific data in the remote execution protocol when running the tests - let's focus on that one:

We have some Java tests which are annotated as requiring network:

@RequiresNetwork
public class ScaryIntegrationTest {
    // ...
}

We need to be able to express to our CI system that when communicating with a remote execution system, caching needs to be disabled, and a platform property needs to be passed to enable routing the actions to network-enabled workers.

We currently express this through a modification in the BUILD file - effectively we have a macro which modifies the exec_properties attribute of a rule, and may modify its tags attribute.

a separate non-blocking job run flaky tests. There is also a post land job that run all tests to detect flaky tests. These jobs share the same test binary fetched from remote cache. They read a configuration file at runtime that contains all known flaky test cases and a flag to decide whether to skip them or not.

This sounds like you need to make many separate systems aware of this metadata, which also needs storing somewhere (or needs to be generated externally of bazel), which is what we're trying to avoid. We want the test portion our CI pipeline to be a simple bazel test ... (or some subset of ...), the same as a user would run on their machine.

If we don't encode in a BUILD file that a test requires access to the network, we lose the ability to bazel test our targets any more.

Instead we'd need some separate tool needs to be aware of the RequiresNetwork annotation. And that tool would either need to run bazel multiple times with different flags (which is problematic for parallelism), or we would need to bazel build our targets and then run them outside of bazel (which is a lot of work, e.g. it would require us to implement the remote execution protocol).

I can't really see a world where we can both bazel test these tests, and not have this information noted in the BUILD file.

---

But even without this particular use-case, the core of this feature request is effectively to not treat map_kind as a special directive. Right now, map_kind can be used anywhere in the repo, but only at a BUILD file granularity - this feature request is to allow any language implementation to have the power of map_kind based on its own criteria, rather than requiring effectively a global override. A team being able to configure that, say, half of the targets generated in a BUILD file should use a legacy rule type for backwards compatibility, and half should use a non-legacy rule type, seems like a generally reasonable request, but currently requires global configuration of a plugin. This feature request is really about being able to make a more flexible building-block than map_kind.

[linzhp]

Is it possible to write multiple Gazelle extensions: one for RequiresNetwork, one for Flaky, and one for the rest. So you can have something like:

gazelle_binary(
    name = "gazelle_bin",
    # Gazelle runs the extensions below in order. Behavior may change if the order is changed.
    languages = [
        "//gazelle-extensions/java_flaky",
        "//gazelle-extensions/java_network",
        "//gazelle-extensions/java",
    ],
    visibility = ["//visibility:public"],
)

//gazelle-extensions/java_flaky and //gazelle-extensions/java_network will generate specific kinds of Java targets when they see @Flaky or @ RequiresNetwork. Then //gazelle-extensions/java can skip the Java files that have been consumed by previous extensions, and only include the rest to regular Java targets, like this: https://github.com/bazelbuild/bazel-gazelle/blob/2b30bc443ea4494629527d6d72cd14243b22af5e/language/go/generate.go#L246-L258

[illicitonion]

//gazelle-extensions/java_flaky and //gazelle-extensions/java_network will generate specific kinds of Java targets when they see @Flaky or @ RequiresNetwork. Then //gazelle-extensions/java can skip the Java files that have been consumed by previous extensions

Is there a way for these to interplay nicely so that //gazelle-extensions/java is still generating the deps list for the targets identified by //gazelle-extensions/java_network though?

It's not that we want to skip these targets come java generation, it's that we want to customise how they're rendered...

[vardaro]

FWIW this limitation led us to writing/maintaining a lot of our own Gazelle plugins for Python, Proto, and many more. We didn't have the flexibility with pre-baked directives to make Gazelle generate our own rules properly.

[linzhp]

@illicitonion Can you mock up a BUILD.bazel file for Java files with RequiresNetwork. I wan to make sure I understand correctly

[illicitonion]

Absolutely:

load("//some:custom.bzl", "requires_network")

java_test(
    name = "OrdinaryTest",
    srcs = ["OrdinaryTest.java"],
    deps = [
        ":helper",
        "//junit",
    ],
)

requires_network(
    java_test,
    name = "RequiresNetworkTest",
    srcs = ["RequiresNetworkTest.java"],
    deps = [
        ":helper",
        "//junit",
    ],
)

java_library(
    name = "helper",
    srcs = ["Helper.java"],
)

(We happen to use requires_network(java_test, ...) but java_network_test(...) would work equivalently)

[linzhp]

Is there a way for these to interplay nicely so that //gazelle-extensions/java is still generating the deps list for the targets identified by //gazelle-extensions/java_network though?

Do you want OrdinaryTest to have deps for RequiresNetworkTest, even though OrdinaryTest.java doesn't import them?

[illicitonion]

Do you want OrdinaryTest to have deps for RequiresNetworkTest, even though OrdinaryTest.java doesn't import them?

Ideally they would have independently gathered deps. But the logic for gathering the deps, and providing a resolver for them, lives firmly in the remit of the java extension.

[linzhp]

//gazelle-extensions/java_network can call functions in //gazelle-extensions/java to gather and resolve deps, right?

[illicitonion]

I'm about to head out for the weekend, but will give a go at implementing these two tightly-coupled extensions shortly - thanks for the feedback!

(I'd also be curious as to @vardaro's use cases - maybe they could describe their use-cases some?)

[illicitonion]

I put together an example of two tightly coupled languages, and the result really wasn't very nice; some of the edge-cases that I ran into that are super fiddly:

1. For the network plugin to perform resolution via another language, that language's Resolver needs to be a static global somewhere, because when language.NewLanguage() is called, it has no context passed to it. (The Java resolver, for instance, is stateful, so we can't just make a new one, we need to share one with the Java backend). This works fine, but is not a nice pattern to be putting in place. And if we end up doing this for multiple languages, our plugin would need to multiplex based on generated target kind (original target kind though - map_kind would make this harder) to one of several resolvers depending on what plugin should have generated the original target.
2. The Java plugin allows you to express "All subdirectories of this directory should be merged into this directory, as far as BUILD files are concerned" - the helper plugin would need to be aware of this config, and re-implement the logic to decide whether to merge directories or not when generating rules.
3. A blunt "was this src covered by another generator" actually isn't sufficient - we also have other gazelle extensions (e.g. one which generates a filegroup per package for release bundling purposes) which generate targets that cover files too. We can work around this by checking whether the kind of the target is one generated by the extension we expect, but again, this is a lot of coupling.

Also, needing to parse each Java file twice (which is relatively expensive), or build a Java indexer cache shared by multiple processes, rather than just indexing once, isn't ideal.

---

For now, I'll just implement the extension I need using flags (to be repo-global) rather than with config directives (which would be more flexible), but I really do think we should make loads be able to refer to config here.

[illicitonion]

For now, I'll just implement the extension I need using flags (to be repo-global) rather than with config directives

Oh actually, this doesn't quite work out because we don't have https://github.com/bazelbuild/bazel-gazelle/pull/1274 - still, that at least is hopefully a simple PR to get merged! I'll try to put together some test for it soon.

[linzhp]

when language.NewLanguage() is called

Is it possible for the Java plugin to implement a stateless function like ResolveGo: https://github.com/bazelbuild/bazel-gazelle/blob/2b30bc443ea4494629527d6d72cd14243b22af5e/language/go/resolve.go#L120

That way, you can call it without creating a Language instance.

re-implement the logic to decide whether to merge directories or not when generating rules.

Can you put the logic in a stateless function too?

[illicitonion]

Is it possible for the Java plugin to implement a stateless function

The Java plugin discovers packages by indexing, so fundamentally needs to store that indexing state somewhere so that the resolver can be driven by it. We could only make this stateless by removing some of the flexibility/functionality of the plugin, which would make it much less appealing to use (particularly as a migration tool).

Can you put the logic in a stateless function too?

We could, but at that point we're making an incredibly tight coupling between these plugins - that's going to make it fairly hard to evolve functionality.

---

Can I step back and check where the objection to adding Config to these functions is? Is it just because changing the API is inconvenient (which I grant it is!), or is there also some technical objection to these functions having access to Config? I'm not personally seeing the reason that adding Config here would be bad, but I do definitely see that the API break is inconvenient. Is there a technical reason I'm missing?

[linzhp]

so fundamentally needs to store that indexing state somewhere

ResolveGo takes the index as an argument

Is it just because changing the API is inconvenient

Yeah, it means all Gazelle plugins are broken, but I think per directory configuration for Kinds() and Loads() is a useful feature. @achew22 @jayconrod What do you think?