No-op transition causes dependencies to be rebuilt separately from default configuration

[jayconrod]

Description of the problem / feature request:

Suppose we have an x_binary rule with an incoming edge transition, which can change build settings based on attributes. Most of the time the attributes are not set, so the transition makes no changes (output settings are identical to inputs).

In cases where the transition makes no changes, Bazel should act as if no transition were applied. That is, if an x_binary depends on a file created by another target (x_library), the file should only be generated once, whether the x_library is a dependency of x_binary or the x_library is built directly in the default configuration.

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

Here's a standalone workspace that reproduces this. x_library runs date and writes the output to a file. x_binary just collects files.

-- 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(":def.bzl", "x_binary", "x_library")
load("@bazel_skylib//rules:common_settings.bzl", "string_setting")

x_binary(
    name = "bin",
    deps = ["lib"],
)

x_library(
    name = "lib",
)

string_setting(
    name = "x_setting",
    build_setting_default = "auto",
)

-- def.bzl --
def _x_transition_impl(settings, attr):
    return settings

x_transition = transition(
    implementation = _x_transition_impl,
    inputs = ["//:x_setting"],
    outputs = ["//:x_setting"],
)

def _x_binary_impl(ctx):
    return [DefaultInfo(files = depset(ctx.files.deps))]

x_binary = rule(
    implementation = _x_binary_impl,
    attrs = {
        "deps": attr.label_list(),
        "_whitelist_function_transition": attr.label(
            default = "@bazel_tools//tools/whitelists/function_transition_whitelist",
        ),
    },
    cfg = x_transition,
)

def _x_library_impl(ctx):
    f = ctx.actions.declare_file(ctx.label.name + ".txt")
    ctx.actions.run_shell(
        outputs = [f],
        command = 'date > "$1"',
        arguments = [f.path],
    )
    return [DefaultInfo(files = depset([f]))]

x_library = rule(
    implementation = _x_library_impl,
)

Building the binary produces one file:

$ bazel build :bin
Target //:bin up-to-date:
  bazel-out/darwin-fastbuild-ST-a8da75533bb21808e51b80cc48e0fbd4a22ad8179cfa2e2acc3ed4c1a7d740fe/bin/lib.txt

$ cat bazel-out/darwin-fastbuild-ST-a8da75533bb21808e51b80cc48e0fbd4a22ad8179cfa2e2acc3ed4c1a7d740fe/bin/lib.txt
Wed Apr 22 14:10:01 EDT 2020

Building the library produces a different file:

$ bazel build :lib
Target //:lib up-to-date:
  bazel-bin/lib.txt

$ cat bazel-bin/lib.txt
Wed Apr 22 14:04:00 EDT 2020

What operating system are you running Bazel on?

macOS 10.15.4

What's the output of bazel info release?

release 3.0.0

[jayconrod]

cc @katre @gregestren @steeve @achew22

[gregestren]

I'll look into it. The code that adds the ST-<hash> prefixes is a bit conservative and sometimes applies "if the configuration might have changed" criteria instead of "if the configuration did change". The only way to know this transition is a no-op is to look at its execution, which the directory-naming code may not have access to.

I'll confirm the current functionality in detail as the next step.

[gregestren]

If we can easily do this, I think it's possible by not calling

bazel/src/main/java/com/google/devtools/build/lib/analysis/skylark/FunctionTransitionUtil.java

Line 306 in 3262af8

updateOutputDirectoryNameFragment(convertedNewValues.keySet(), optionInfoMap, buildOptions);

when we can determine either a) buildOptionsToTransition.equals(buildOptions) or b) newValues is empty (if that's known).

CC @juliexxia if there's subtleties I'm missing.

[juliexxia]

newValues is just the returned dict from the transition implementation function and it doesn't know about its originating build options so we'd need to go through constructing the new build options and then do an equals - I think we should be able to just add an if to where the hash is constructed/used.

The only other thing I'll mention is I have some memory of BuildOptions.equals being an expensive operation and that causing slow downs in the past, relevant to verifying starlark transitions. I think in thatscenario (couldn't find the bug) multiple .equals were being called on every target, starlark transitioned or not, or something like that and this should only happen after transitions so it should be fine, but something to think about.

[aiuto]

cc @bazelbuild/configurability

[gregestren]

BuildOptions.equals caches its result, so it doesn't have to intrinsically be slow. But good point - it does deserve some profiling.

If this is basically realistic this would be a great feature for us to implement.

[gregestren]

GitHub issue review: lowering this to P3 not because it doesn't matter but because no one is working on it at this moment.

I still agree this is worth doing. And should offer nice value for a small amount of effort. We can up-prioritize this once we've figured out who can own doing it.

[juliexxia]

I'm actively working on fixing this now

[stefanbucur]

Not sure if this is the right issue to comment on. When our transition becomes "no-op" (outputs simply forward the inputs), I'm getting the following type of errors:

ERROR: file 'external/com_github_grpc_grpc/_objs/gpr_base/fork.pic.o' is generated by these conflicting actions:
Label: @com_github_grpc_grpc//:gpr_base
RuleClass: cc_library rule
Configuration: 2f8696270c3712ea5075f7cfecbfdf9d2fb51a1f8244da96130057076aba4cca, b2b76f9335e12e6679b98fca3c9cc68e3f7a26cfbca7041c637a729d7877270e
Mnemonic: CppCompile
Action key: aad2ff2a1e3fdb79fb3ba2b36a642a0c5dbb891655dd25ecd4cec4b82f6dc630
Progress message: Compiling com_github_grpc_grpc/src/core/lib/gprpp/fork.cc
PrimaryInput: File:[/usr/local/google/home/sbucur/.cache/bazel/_bazel_sbucur/4e2ed66b5d3aec1195331fcaa64efd63/external[source]]com_github_grpc_grpc/src/core/lib/gprpp/fork.cc
PrimaryOutput: File:[[<execution_root>]bazel-out/k8-fastbuild/bin]external/com_github_grpc_grpc/_objs/gpr_base/fork.pic.o
Owner information: ConfiguredTargetKey{label=@com_github_grpc_grpc//:gpr_base, config=BuildConfigurationValue.Key[2f8696270c3712ea5075f7cfecbfdf9d2fb51a1f8244da96130057076aba4cca]}, ConfiguredTargetKey{label=@com_github_grpc_grpc//:gpr_base, config=BuildConfigurationValue.Key[b2b76f9335e12e6679b98fca3c9cc68e3f7a26cfbca7041c637a729d7877270e]}
MandatoryInputs: are equal
Outputs: are equal
WARNING: errors encountered while analyzing target '@com_github_grpc_grpc//:gpr_base': it will not be built

I believe what is particular about our transition is that is has both scalar and list inputs (the C++ options). Do you think list inputs might be problematic here?

Also let me know if I should file a separate issue for this case.