Dependency cycle involving label_flag and configuration transition

[jayconrod]

Description of the problem / feature request:

In rules_go, we have a customizable static analysis system called nogo. There's a nogo rule that generates a binary that is run alongside the Go compiler when building each go_library. Developers use it to define a binary with a customizable set of static analyzers.

I'd like to migrate nogo to the new Starlark Build Configuration system. Ideally, there should be a label_flag that points to a nogo target to use, and that could be set on the command-line. Currently, each go_binary and go_library implicitly depends on a default nogo target, which is set using some hacks involving repository rules. To break the dependency cycle, nogo targets may only depend on targets defined with go_tool_library, a bootstrap rule similar to go_library except it doesn't depend on nogo in order to break the dependency cycle.

Unfortunately, I'm seeing a different kind of dependency cycle. The workspace below is a minimal, standalone example of what I want to do with nogo.

• x_binary is analogous to go_binary.
• x_checker is analogous to nogo.
• Every x_binary implicitly depends on a label_flag (with cfg = "exec") that points to an x_checker that depends on an x_binary.
• The x_checker has an incoming transition that sets the flag to a special target with no dependencies to break the cycle.

Any build involving the label_flag reports a dependency cycle. It doesn't seem like the current value of the flag is taken into account when the dependency graph is loaded.

This works if checker_flag defaults to //:null_checker and is set to another value. It just doesn't work when checker_flag defaults to a real x_checker target.

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

Run bazel build //:x in this workspace:

-- WORKSPACE --
# empty file

-- BUILD.bazel --
load(":def.bzl", "null_checker", "x_binary", "x_checker")

x_binary(
    name = "x",
)

label_flag(
    name = "checker_flag",
    build_setting_default = "//:checker",
)

x_checker(
    name = "checker",
    dep = ":checker_bin",
)

x_binary(
    name = "checker_bin",
)

null_checker(
    name = "null_checker",
)

-- def.bzl --
def _x_binary_impl(ctx):
    pass

x_binary = rule(
    implementation = _x_binary_impl,
    attrs = {
        "_checker": attr.label(
            default = "//:checker_flag",
            cfg = "exec",
        ),
    },
)

def _x_checker_transition(settings, attr):
    settings = dict(settings)
    settings["//:checker_flag"] = "//:null_checker"
    return settings

x_checker_transition = transition(
    implementation = _x_checker_transition,
    inputs = ["//:checker_flag"],
    outputs = ["//:checker_flag"],
)

def _x_checker_impl(ctx):
    pass

x_checker = rule(
    implementation = _x_checker_impl,
    attrs = {
        "dep": attr.label(mandatory = True),
        "_whitelist_function_transition": attr.label(
            default = "@bazel_tools//tools/whitelists/function_transition_whitelist",
        ),
    },
    cfg = x_checker_transition,
)

def _null_checker_impl(ctx):
    pass

null_checker = rule(
    implementation = _null_checker_impl,
)

What operating system are you running Bazel on?

Darwin / amd64 (macOS 10.15.4)

What's the output of bazel info release?

release 3.1.0

cc @juliexxia @gregestren @katre

[katre]

My suspicion is that for some reason the rule-level transition isn't being used. I'll debug and take a look.

[katre]

No, it's being called. Still trying to figure out what's happening.

[katre]

@juliexxia, Do you know how to debug this further?

[jayconrod]

I think I have a workaround.

• I've added a bool_setting called need_checker_flag. It defaults to True.
• x_checker_transition sets need_checker_flag to "False".
• Instead of depending directly on checker_flag, x_binary rules now depend on checker_alias, which uses a select: it only depends on checker_flag if need_checker_flag is True.

-- WORKSPACE --
load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")

http_archive(
    name = "bazel_skylib",
    sha256 = "97e70364e9249702246c0e9444bccdc4b847bed1eb03c5a3ece4f83dfe6abc44",
    urls = [
        "https://mirror.bazel.build/github.com/bazelbuild/bazel-skylib/releases/download/1.0.2/bazel-skylib-1.0.2.tar.gz",
        "https://github.com/bazelbuild/bazel-skylib/releases/download/1.0.2/bazel-skylib-1.0.2.tar.gz",
    ],
)

load("@bazel_skylib//:workspace.bzl", "bazel_skylib_workspace")

bazel_skylib_workspace()

-- BUILD.bazel --
load("@bazel_skylib//rules:common_settings.bzl", "bool_setting")
load(":def.bzl", "null_checker", "x_binary", "x_checker")

x_binary(
    name = "x",
)

alias(
    name = "checker_alias",
    actual = select({
        "//:need_checker": ":checker_flag",
        "//conditions:default": ":null_checker",
    }),
)

label_flag(
    name = "checker_flag",
    build_setting_default = "//:checker",
)

bool_setting(
    name = "need_checker_flag",
    build_setting_default = True,
)

config_setting(
    name = "need_checker",
    flag_values = {
        "//:need_checker_flag": "True",
    },
)

x_checker(
    name = "checker",
    dep = ":checker_bin",
)

x_binary(
    name = "checker_bin",
)

null_checker(
    name = "null_checker",
)

-- def.bzl --
def _x_binary_impl(ctx):
    pass

x_binary = rule(
    implementation = _x_binary_impl,
    attrs = {
        "_checker": attr.label(
            default = "//:checker_alias",
            cfg = "exec",
        ),
    },
)

def _x_checker_transition(settings, attr):
    settings = dict(settings)
    settings["//:checker_flag"] = "//:null_checker"
    settings["//:need_checker_flag"] = False
    return settings

x_checker_transition = transition(
    implementation = _x_checker_transition,
    inputs = [
        "//:checker_flag",
        "//:need_checker_flag",
    ],
    outputs = [
        "//:checker_flag",
        "//:need_checker_flag",
    ],
)

def _x_checker_impl(ctx):
    pass

x_checker = rule(
    implementation = _x_checker_impl,
    attrs = {
        "dep": attr.label(mandatory = True),
        "_whitelist_function_transition": attr.label(
            default = "@bazel_tools//tools/whitelists/function_transition_whitelist",
        ),
    },
    cfg = x_checker_transition,
)

def _null_checker_impl(ctx):
    pass

null_checker = rule(
    implementation = _null_checker_impl,
)

[jayconrod]

No, turns out the workaround doesn't work. Bazel still reports a dependency cycle when a binary is built in the host configuration.

To reproduce, try building the //:x_runner target below.

-- WORKSPACE --
load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")
http_archive(

name = "bazel_skylib",

sha256 = "97e70364e9249702246c0e9444bccdc4b847bed1eb03c5a3ece4f83dfe6abc44",

urls = [

"https://mirror.bazel.build/github.com/bazelbuild/bazel-skylib/releases/download/1.0.2/bazel-skylib-1.0.2.tar.gz",

"https://github.com/bazelbuild/bazel-skylib/releases/download/1.0.2/bazel-skylib-1.0.2.tar.gz",

],

)
load("@bazel_skylib//:workspace.bzl", "bazel_skylib_workspace")
bazel_skylib_workspace()
-- BUILD.bazel --

load("@bazel_skylib//rules:common_settings.bzl", "bool_setting")

load(":def.bzl", "null_checker", "x_binary", "x_checker", "x_runner")
x_runner(

name = "x_runner",

dep = ":x",

)
x_binary(

name = "x",

)
alias(

name = "checker_alias",

actual = select({

"//:need_checker": ":checker_flag",

"//conditions:default": ":null_checker",

}),

)
label_flag(

name = "checker_flag",

build_setting_default = "//:checker",

)
bool_setting(

name = "need_checker_flag",

build_setting_default = True,

)
config_setting(

name = "need_checker",

flag_values = {

"//:need_checker_flag": "True",

},

)
x_checker(

name = "checker",

dep = ":checker_bin",

)
x_binary(

name = "checker_bin",

)
null_checker(

name = "null_checker",

)
-- def.bzl --

def _x_binary_impl(ctx):

pass
x_binary = rule(

implementation = _x_binary_impl,

attrs = {

"_checker": attr.label(

default = "//:checker_alias",

cfg = "exec",

),

},

)
def _x_checker_transition(settings, attr):

settings = dict(settings)

settings["//:checker_flag"] = "//:null_checker"

settings["//:need_checker_flag"] = False

return settings
x_checker_transition = transition(

implementation = _x_checker_transition,

inputs = [

"//:checker_flag",

"//:need_checker_flag",

],

outputs = [

"//:checker_flag",

"//:need_checker_flag",

],

)
def _x_checker_impl(ctx):

pass
x_checker = rule(

implementation = _x_checker_impl,

attrs = {

"dep": attr.label(mandatory = True),

"_whitelist_function_transition": attr.label(

default = "@bazel_tools//tools/whitelists/function_transition_whitelist",

),

},

cfg = x_checker_transition,

)
def _null_checker_impl(ctx):

pass
null_checker = rule(

implementation = _null_checker_impl,

)
def _x_runner_impl(ctx):

pass
x_runner = rule(

implementation = _x_runner_impl,

attrs = {

"dep": attr.label(

mandatory = True,

cfg = "host",

),

},

)

It doesn't seem like there's another way around this, so I think I'll have to roll back the change in rules_go. Too bad. It simplified a lot of things.

[katre]

The problem with the workaround is just that you can't transition in the host configuration, so the flag can't be changed. We don't yet have a way to set the values of Starlark flags in the host configuration specifically, unfortunately.

@gregestren and @juliexxia we should figure this out.

[juliexxia]

Hey Jay, not totally sure what's going on here but I can believe we have a bug with our label_flags and transitions. Is this bug blocking you? This week is a bazel fixit so I'd like to come back and investigate next week if this isn't totally keeping you from getting things done.

[jayconrod]

Thanks Julie, it would be amazing if this could be fixed.

This blocks migration of our static analysis framework (nogo) to build settings and transitions. Essentially we have a nogo rule that creates a binary that runs alongside Go compile actions.

Currently, every package the nogo binary depends on has to be specified with a go_tool_library rule, which is like go_library but doesn't build with nogo to avoid a cyclic dependency. So we end up with a parallel graph of targets that need to be written out in build files.

If we can fix this dependency cycle, then nogo can transition on the incoming edge to a configuration that doesn't require nogo. Then we could get rid of go_tool_library and just use regular go_library rule. It would make the framework much easier to use and maintain.

This seems to work everywhere except the host configuration. We do have a couple binaries that need to run on hosts like Gazelle.

(I've migrated everything else to build settings and transitions by the way, and I'm about to tag a new release with that. I'm quite happy with the simplifications that's enabled. Thanks for working on this!)

[katre]

What are you using the host configuration for at this point? I had hoped that at this point you'd be entirely into execution transitions.

[jayconrod]

The main thing is the gazelle rule. It depends on @bazel_gazelle//cmd/gazelle in the host configuration. That binary runs on the system where Bazel is invoked and is intended to be used as bazel run //:gazelle.

Is there another way to express that with execution transitions?

[katre]

Hmm, bazel run should probably set the target platform to the host platform, which would handle that case. Then, you wouldn't need any special handling for gazelle, just leave it in the "target" configuration.

Does that make sense for you?

[gregestren]

Triaged inline with Julie's comments above.

[jayconrod]

Just tried changing cfg = "host" to cfg = "target" and then cfg = "exec" for the gazelle attribute in the _gazelle_runner rule. Unfortunately, the dependency cycle error still comes up in both cases.

[katre]

if you try cfg = "target" and bazel run --platforms=@local_config_platforms//:host gazelle_target, what happens?

[jayconrod]

I see this error. Do I need to define the repository local_config_platforms somewhere? From the documentation it sounds like it should be built-in. This is on Bazel 3.1.0.

ERROR: While resolving toolchains for target //:gazelle: com.google.devtools.build.lib.packages.BuildFileNotFoundException: no such package '@local_config_platforms//': The repository '@local_config_platforms' could not be resolved
ERROR: Analysis of target '//:gazelle' failed; build aborted: com.google.devtools.build.lib.packages.BuildFileNotFoundException: no such package '@local_config_platforms//': The repository '@local_config_platforms' could not be resolved

[katre]

Sorry, I meant @local_config_platform//:host, I just mis-typed it.

[jayconrod]

Ah, ok. In that case, it's the same dependency cycle error:

ERROR: /private/var/tmp/_bazel_jayconrod/60e122df6a2a16ab6af86d77e9a71ec5/external/io_bazel_rules_go/BUILD.bazel:71:1: in go_context_data rule @io_bazel_rules_go//:go_context_data: cycle in dependency graph:
    //:gazelle
    //:gazelle-runner
    @bazel_gazelle//cmd/gazelle:gazelle
    @bazel_gazelle//language/go:go_default_library
    @bazel_gazelle//language/go:std_package_list.go
    @bazel_gazelle//language/go:std_package_list
    @bazel_gazelle//language/go/gen_std_package_list:gen_std_package_list (host)
.-> @io_bazel_rules_go//:go_context_data (host)
|   @io_bazel_rules_go//:nogo_alias (host)
|   @io_bazel_rules_go//go/config:nogo (host)
|   @io_bazel_rules_nogo//:nogo (host)
|   @io_bazel_rules_go//:tools_nogo (host)
|   @org_golang_x_tools//go/analysis/passes/unreachable:go_default_library (host)
`-- @io_bazel_rules_go//:go_context_data (host)
This cycle occurred because of a configuration option

[katre]

Okay, looks like go:std_package_list has a host dependency on gen_std_package_list:gen_std_package_list.

[katre]

Actually, in testing it looks like bazel run does use the host platform for the target already.

[katre]

@jayconrod This hasn't been updated in a while. Is it still an issue?

[jayconrod]

Yes I think so. The example still reproduces the error in Bazel 4.0.0. I tried making x_binary produce an executable, then running that with bazel run //:x, but that gives the same error.

[katre]

Good news: I have a fix out in #13372.

For posterity: The transition was working correctly, and was in fact converting to use //:null_checker in the second instance of //:check_flag. However, all instances of label_flag actually had two dependencies: one on the value being used, and another one on the default value. It was this default value dependency that wasn't changing (and wasn't changeable) that was causing the cycle.

The fix is that the default should be a NODEP_LABEL, since it's just used to hold a label to be used later.