Conflicting actions for cc_proto_library with transition in Starlark rule dependency

[fmeum]

Description of the problem / feature request:

As of 7acf9ea, Bazel generates conflicting actions when a custom Starlark rule depends on a cc_proto_library target in the exec configuration if there is a transition applied to the library.

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

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

http_archive(
    name = "com_google_protobuf",
    sha256 = "7892a35d979304a404400a101c46ce90e85ec9e2a766a86041bb361f626247f5",
    strip_prefix = "protobuf-3.16.0",
    url = "https://github.com/protocolbuffers/protobuf/archive/v3.16.0.tar.gz",
)

load("@com_google_protobuf//:protobuf_deps.bzl", "protobuf_deps")

protobuf_deps()

$ cat BUILD.bazel
load(":rules.bzl", "exec_with_library_dep", "apply_transition")

proto_library(
    name = "proto",
    srcs = ["empty.proto"],
    deps = [],
)

cc_proto_library(
    name = "cc_proto",
    deps = [":proto"],
)

apply_transition(
    name = "cc_proto_with_transition",
    native_target = ":cc_proto",
)

exec_with_library_dep(
    name = "error",
    out = "dummy.gen",
    library = ":cc_proto_with_transition",
)

$ touch empty.proto

$ cat rules.bzl
_COPT_BUILD_SETTING = "//command_line_option:copt"

def _set_foobar_copt(settings, attr):
    return {_COPT_BUILD_SETTING: ["-DFOOBAR"]}

set_foobar_copt = transition(
    implementation = _set_foobar_copt,
    inputs = [],
    outputs = [_COPT_BUILD_SETTING],
)

def _apply_transition_impl(ctx):
    pass

apply_transition = rule(
        implementation = _apply_transition_impl,
        attrs = {
            "native_target": attr.label(
                cfg = set_foobar_copt,
            ),
            "_allowlist_function_transition": attr.label(
                default = "@bazel_tools//tools/whitelists/function_transition_whitelist",
            ),
        },
    )


def _exec_with_library_dep_impl(ctx):
    ctx.actions.write(ctx.outputs.out, "")

exec_with_library_dep = rule(
    _exec_with_library_dep_impl,
    attrs = {
        "library": attr.label(
            mandatory = True,
            cfg = "exec",
        ),
        "out": attr.output(
            mandatory = True,
        ),
    },
)

$ bazel build //:error

You can also clone https://github.com/fmeum/cc_proto_library_transition_issue.

The command will generate the following error:

ERROR: file 'external/com_google_protobuf/_objs/protobuf_lite/int128.o' is generated by these conflicting actions:
Label: @com_google_protobuf//:protobuf_lite
RuleClass: cc_library rule
Configuration: 8d1a3355bd0201255e646e7ddf342ec528414ebbf2f1feab6e6b20aced50487e, 64b9564990ccaa73f2844f556c40131bd590d44ee9288aea2e3f3beb0c1a956b
Mnemonic: CppCompile
Action key: 4a432e1690a4da20c2fe249ae6554cf201d9792e4375c5a945c9962a8f2ec395, 9dc03c391082ba0e9a3e692ac5cc7257e5abab2a8de3eb5226db9bc4a4ba9f50
Progress message: Compiling src/google/protobuf/stubs/int128.cc
PrimaryInput: File:[/home/fhenneke/.cache/bazel/_bazel_fhenneke/3665225be4dbfed855cbc8b2bd45f83a/external/com_google_protobuf[source]]src/google/protobuf/stubs/int128.cc
PrimaryOutput: File:[[<execution_root>]bazel-out/k8-opt-exec-2B5CBBC6-ST-cbcaac683f1b/bin]external/com_google_protobuf/_objs/protobuf_lite/int128.o
Owner information: ConfiguredTargetKey{label=@com_google_protobuf//:protobuf_lite, config=BuildConfigurationValue.Key[8d1a3355bd0201255e646e7ddf342ec528414ebbf2f1feab6e6b20aced50487e]}, ConfiguredTargetKey{label=@com_google_protobuf//:protobuf_lite, config=BuildConfigurationValue.Key[64b9564990ccaa73f2844f556c40131bd590d44ee9288aea2e3f3beb0c1a956b]}
MandatoryInputs: are equal
Outputs: are equal
ERROR: com.google.devtools.build.lib.skyframe.ArtifactConflictFinder$ConflictException: com.google.devtools.build.lib.actions.MutableActionGraph$ActionConflictException: for external/com_google_protobuf/_objs/protobuf_lite/int128.o, previous action: action 'Compiling src/google/protobuf/stubs/int128.cc', attempted action: action 'Compiling src/google/protobuf/stubs/int128.cc'
INFO: Elapsed time: 0.121s
INFO: 0 processes.
FAILED: Build did NOT complete successfully (0 packages loaded, 0 targets configured)

What operating system are you running Bazel on?

Ubuntu 20.10

What's the output of bazel info release?

The issue reproduces with release 4.0.0 as well as master @ 7caa01f, but not with release 3.7.2.

Using bazel-bisect.sh, I have identified the breaking commit as 7acf9ea, which enabled exec transitions on proto rules.

[fmeum]

For context: This does break our build and blocks us from moving to Bazel 4. Since the breaking commit has been identified, could it be reverted?

[fmeum]

I debugged this a bit more and have some more information:

• Output of bazel config with the example workspace above:

Displaying diff between configs 7b34a8fa4e1b2383020edce66c2acf41b3c12499affb782277374d6c9efd6950 and 95f1321061780b75d432048aaa0f36c9ed3f6d9b17ab714cb5095331ca133766
FragmentOptions com.google.devtools.build.lib.rules.cpp.CppOptions {
  copt: ["-DFOOBAR"], [-g0]
}

• Not fixed by making CppCompileAction shareable (Action conflicts when new Python version transition semantics are enabled #7655 (comment))

[gregestren]

@sdtwigg will offer some guidance on what's going on for your build.

[fmeum]

@sdtwigg Friendly ping

[fmeum]

@gregestren Could you offer some guidance? Our current workaround is to use the target rather than the exec config, but that will no longer be feasible with cross-compilation.

[gregestren]

I'll up the visibility on this and we'll get you some kind of a response.

[sdtwigg]

There seems to be a broad class of issues related to action conflict detection when the owning 'rule' is an Aspect.

Unfortunately, the actual mechanism of this has been tricky to track down since the systems for both aspects and action conflict detection are a bit obscured.

[fmeum]

I may have identified the root cause of this issue. The cc_proto_library rule depends on @com_google_protobuf//:protobuf in two incompatible ways: Once through protoc (via the exec transition since 7acf9ea) and once through @com_google_protobuf//:cc_toolchain via the toolchain attr in CcProtoAspect, which is missing the configuration required for the exec transition. Adding it in the latter place fixes the issue for me.

@sdtwigg Would you review a PR? If so, would you also like me to include the reproducer as a test?

[fmeum]

After a first misguided attempt to fix the issue, I now took a deeper look and produced a Starlark-only reproducer that does not rely on aspects (see below). The cause of the issue rather appears to be the particular chain of transitions in which the target, in this case //:fake_protobuf, is built:

1. Through the library attribute of fake_mock, the build transitions into the exec configuration, so far without any Starlark transitions attached (output path is bazel-out/k8-opt-exec-2B5CBBC6).
2. Through the target attribute of apply_copt_transition, a Starlark transition setting a distinctive copt is applied to the rest of the build (output path is bazel-out/k8-opt-exec-2B5CBBC6-ST-cbcaac683f1b). This copt -DFOOBAR directly applies to the build of //:fake_protobuf when it is built to satisfy the _runtime prerequisite of fake_cc_proto_library.
3. Through the _compiler attribute of fake_cc_proto_library, the build of //:fake_protoc transitions again into the exec configuration, which via a transitive dependency ends up building //:fake_protobuf. Here, the copts have been reset by https://github.com/bazelbuild/bazel/blob/2e723f228efee008bcfd62ceb74a176a357c4c32/src/main/java/com/google/devtools/build/lib/analysis/config/ExecutionTransitionFactory.java#L124 to those of the host config so that this build of the cc_library does not define FOOBAR. However, since 99f8a8f#diff-85e08f9833efccc5033b847d466a7d3b6b7ed82c41755fcc2d1d14b3da3b8c55 the output path is again bazel-out/k8-opt-exec-2B5CBBC6-ST-cbcaac683f1b, which results in the action conflict.

Based on this analysis, the root cause of this issue appears to be that the logic that updates transitionDirectoryNameFragment in ExecutionTransitionFactory is not correct in general: It simply copies over the existing hash to the new configuration, even though flags (in this case --copt) might have changed.
It seems to me that this issue can be resolved by replicating (or reusing) parts of the logic in FunctionTransitionUtils to recompute the correct hash to put in the new configuration's transitionDirectoryNameFragment. However, I'm not yet sure which settings (native or Starlark) need to be included in the hash as e.g. the Starlark transition might have changed flags such as host_copt, which due to the exec transition have now become regular copts and would thus need to be tracked.

@gregestren @sdtwigg If you could provide some guidance, I would happily research this further and see whether I can prepare a fix.

The reproducer:

#/usr/bin/env sh
touch WORKSPACE

cat << EOF > lib.h
int lib();
EOF

cat << EOF > lib.cc
#include "lib.h"

int lib() {
    return 0;
}
EOF

cat << EOF > main.cc
#include "lib.h"

int main() {
  return lib();
}
EOF

cat << EOF > rules.bzl
_COPT_BUILD_SETTING = "//command_line_option:copt"

def _set_copt_impl(settings, attr):
    return {_COPT_BUILD_SETTING: ["-DFOOBAR"]}

_set_copt = transition(
    implementation = _set_copt_impl,
    inputs = [],
    outputs = [_COPT_BUILD_SETTING],
)

def _apply_copt_transition_impl(ctx):
    pass

apply_copt_transition = rule(
    implementation = _apply_copt_transition_impl,
    attrs = {
        "target": attr.label(
            cfg = _set_copt,
        ),
        "_allowlist_function_transition": attr.label(
            default = "@bazel_tools//tools/allowlists/function_transition_allowlist",
        ),
    },
)

def _fake_cc_proto_library_impl(ctx):
    return [
        ctx.attr._runtime[DefaultInfo],
        ctx.attr._runtime[CcInfo],
    ]

fake_cc_proto_library = rule(
    _fake_cc_proto_library_impl,
    attrs = {
        "_compiler": attr.label(
            default = "//:fake_protoc",
            cfg = "exec",
            executable = True,
        ),
        "_runtime": attr.label(
            default = "//:fake_protobuf",
        ),
    },
)

def _fake_mock_impl(ctx):
    ctx.actions.write(ctx.outputs.out, "")

fake_mock = rule(
    _fake_mock_impl,
    attrs = {
        "library": attr.label(
            cfg = "exec",
        ),
        "out": attr.output(
            mandatory = True,
        ),
    },
)
EOF

cat << EOF > BUILD
load(":rules.bzl", "apply_copt_transition", "fake_cc_proto_library", "fake_mock")

cc_library(
    name = "fake_protobuf",
    srcs = [
        "lib.cc",
    ],
    hdrs = [
        "lib.h",
    ],
)

cc_binary(
    name = "fake_protoc",
    srcs = [
        "main.cc",
    ],
    deps = [
        ":fake_protobuf",
    ],
)

fake_cc_proto_library(
    name = "cc_proto_lib",
)

apply_copt_transition(
    name = "cc_proto_lib_with_copt_transition",
    target = ":cc_proto_lib",
)

fake_mock(
    name = "mock",
    out = "empty.gen",
    library = ":cc_proto_lib_with_copt_transition",
)
EOF

bazel build //:mock

[fmeum]

I prepared #13915, which resolves both this issue and the analogous one with --host_copt instead of --copt. It updates the transitionDirectoryNameFragment also taking into account that --host_copt becomes --copt under the exec transition.

[sdtwigg]

I will take a look at your PR today. It is surprising because we had just (late last week) identified a separate issue with AspectFunction failing to ask for a proper, post-transition value but this seems like a different bug entirely.

…

On Sun, Aug 29, 2021 at 3:13 AM Fabian Meumertzheim < ***@***.***> wrote: I prepared #13915 <#13915>, which resolves both this issue and the analogous one with --host_copt instead of --copt. It updates the transitionDirectoryNameFragment also taking into account that --host_copt becomes --copt under the exec transition. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#13464 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHX76GMOTYW7X4N4YGILT3T7HMZ5ANCNFSM44UMY4LA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[sdtwigg]

Why would users be modifying the host_* values in a transitions?
To my understanding, the host configuration is built once at the top-level based on what the host_* values are there and future changes to host_* values have no effect to the host configuration.

Edit: Oh, I think I misread your description and see now that you are also introducing a mechanism to modify the exec transition via host options. I absolutely have to discuss this with jcater before proceeding.

Edit2: I might have further misread and instead that behavior already exists but is not accounted for in the exec transition renaming. I still want to double check that this behavior is expected and not an artifact of the exec transition being built off infrastructure for the host transition.

[fmeum]

Re your edit2: In fact, I also only learned about this being possible only while writing the tests for this PR. I'm not sure whether its intended or not.

I could see the following use cases:

1. If a custom tool run during the build requires C++17, but the project itself does not want to add the required flag to its .bazelrc, it can be brought in via a transition on --host_cxxopt prior to switching to the exec config. This could also be useful with sanitizers or optimization flags.
2. If a third-party tool run during the build on an Apple M1 Mac is not yet compatible with the new architecture, a transition on --host_platform to some darwin x86_64 platform can help fix the build.

If it turns out that exec transitions should not behave this way, I could remove the host flag propagation from the PR. The call to updateTransitionDirectoryNameFragment would remain.

[gregestren]

@katre, @sdtwigg, and I are definitely discussing this further soon (we've scheduled time to talk about this). Thanks for the input.

[gregestren]

@sdtwigg and I discussed this more today.

Thanks particularly for your spot-on summary at #13464 (comment).

Based on this analysis, the root cause of this issue appears to be that the logic that updates transitionDirectoryNameFragment in ExecutionTransitionFactory is not correct in general: It simply copies over the existing hash to the new configuration, even though flags (in this case --copt) might have changed.

I agree completely. 99f8a8f#diff-85e08f9833efccc5033b847d466a7d3b6b7ed82c41755fcc2d1d14b3da3b8c55 was not the most rigorous patch and you're clearly showing its holes.

It seems to me that this issue can be resolved by replicating (or reusing) parts of the logic in FunctionTransitionUtils to recompute the correct hash to put in the new configuration's transitionDirectoryNameFragment.

This makes sense. More broadly, I believe the biggest problem is that native transitions (transitions written directly in Java) have their own, independent algorithms for determining output paths vs. Starlark transitions. So when you interleave them you get the possibility of corner cases like you've described.

Also see

bazel/src/main/java/com/google/devtools/build/lib/analysis/config/OutputDirectories.java

Line 172 in 09c621e

private static String buildMnemonic(

and

bazel/src/main/java/com/google/devtools/build/lib/analysis/config/Fragment.java

Line 65 in 09c621e

public String getOutputDirectoryName() {

if you haven't already. This is the primary code native transitions use to set output paths.

So I agree with your intuition. I'm just unclear on the right way to merge them. Calling into FunctionTransitionUtils from ExecutionTransitionFactory is one approach, but that's also muddling up a bit of the sequencing of buildMnemonic.

However, I'm not yet sure which settings (native or Starlark) need to be included in the hash as e.g. the Starlark transition might have changed flags such as host_copt, which due to the exec transition have now become regular copts and would thus need to be tracked.

I remain confused about this too. --host_ flags are indeed unique, and I'm not sure Starlark transitions are "smart" enough to know how to properly integrate them. Perhaps your current approach really is the best short-term approach, even though we'd really like to have something more pure and consolidated over the long-term. Possibly with writing better logic for rule writers to declare what flags propagate to the host or not, which could eliminate the need for --host_ flags.

Also see #13580. It doesn't fix this issue but it changes and improves how transitionDirectoryNameFragment is computed, so FYI.

[fmeum]

Thanks for providing the context.

More broadly, I believe the biggest problem is that native transitions (transitions written directly in Java) have their own, independent algorithms for determining output paths vs. Starlark transitions. So when you interleave them you get the possibility of corner cases like you've described.

Also see

bazel/src/main/java/com/google/devtools/build/lib/analysis/config/OutputDirectories.java

Line 172 in 09c621e

private static String buildMnemonic(

and

bazel/src/main/java/com/google/devtools/build/lib/analysis/config/Fragment.java

Line 65 in 09c621e

public String getOutputDirectoryName() {

if you haven't already. This is the primary code native transitions use to set output paths.
So I agree with your intuition. I'm just unclear on the right way to merge them. Calling into FunctionTransitionUtils from ExecutionTransitionFactory is one approach, but that's also muddling up a bit of the sequencing of buildMnemonic.

I think that there is a trade-off to be made here:

1. Currently, transitionDirectoryNameFragment is only recomputed when it could possibly have changed. This may help performance, but means that the function that recomputes it needs to be called from a number of different places (including ExecutionTransitionFactory) and if that is forgotten, action conflicts ensue.
2. Conceptually, recomputing transitionDirectoryNameFragment should probably be part of some Fragment's getOutputDirectoryName. Since Starlark settings live in CoreOptions, which is not a Fragment, it could make sense to add a getOutputDirectoryName method to CoreOptions, which would handle the logic in
   

   bazel/src/main/java/com/google/devtools/build/lib/analysis/config/OutputDirectories.java

   Lines 194 to 211 in 09c621e

   	String platformSuffix = (options.platformSuffix != null) ? options.platformSuffix : "";
   	try {
   	PathFragment.checkSeparators(platformSuffix);
   	} catch (InvalidBaseNameException e) {
   	throw new InvalidMnemonicException("Platform suffix '" + platformSuffix + "'", e);
   	}
   	String shortString = options.compilationMode + platformSuffix;
   	nameParts.add(shortString);
   	if (options.transitionDirectoryNameFragment != null) {
   	try {
   	PathFragment.checkSeparators(options.transitionDirectoryNameFragment);
   	} catch (InvalidBaseNameException e) {
   	throw new InvalidMnemonicException(
   	"Transition directory name fragment '" + options.transitionDirectoryNameFragment + "'",
   	e);
   	}
   	nameParts.add(options.transitionDirectoryNameFragment);
   	}

   
   as well as call updateTransitionDirectoryNameFragment.

If potentially wasteful recomputations of transitionDirectoryNameFragment are not a problem, I think that approach 2 could greatly improve the clarity here and may even prevent action conflicts that we just haven't identified yet. What do you think?

I remain confused about this too. --host_ flags are indeed unique, and I'm not sure Starlark transitions are "smart" enough to know how to properly integrate them. Perhaps your current approach really is the best short-term approach, even though we'd really like to have something more pure and consolidated over the long-term. Possibly with writing better logic for rule writers to declare what flags propagate to the host or not, which could eliminate the need for --host_ flags.

I agree that added rule metadata would help here. It would first need to be clarified though whether separate --exec_ flags (respectively the behavior of transitioned host_ flags under exec transitions) are desired or not.

Also see #13580. It doesn't fix this issue but it changes and improves how transitionDirectoryNameFragment is computed, so FYI.

Thanks, this PR is indeed very important for us as we encounter frequent rebuilds that would be prevented by it.

[gregestren]

We may be arguing for the same thing.

I can't remember where I mentioned this, but I'd like to prioritize a refactoring that computes transitionDirectoryNameFragment directly in BuildConfiguration (or in CoreOptions as you describe - it's the same concept). transitionDirectoryNameFragment would therefore purely be a function of the current configuration's BuildOptions (it's current native + Starlark options). With no regard for what sequence of transitions got you there.

There are some technical complications to realize that but I believe they're manageable. I should update in more detail here.

I share your intuition that this would clean up a large class of performance degradations and action conflicts that we experience today. If that works I'd prefer it over #13915, although it's a deeper effort. Re: 5.0 - it may be possible to get a short-term cherrypick in but I don't want to rush something we're not fully comfortable with.

I also don't think approach 2 above completely eliminates action conflict risk. There should still be ways, I believe, for native + Starlark transitions to sabotage each others' invariants. I can trace up an example demonstrating.

I'm not worried about approach 2 wastefully recomputing work, particularly if it's computed in BuildConfiguration instantiation. Not many BuildConfiguration instances are created in any practical build - existing caching ensures that.

[sdtwigg]

This is slated to be resolved imminently when transitionDirectoryNameFragment computation is moved to being part of BuildConfigurationFunction rather than being (redundant state) in BuildOptions.

I'll leave this open until that CL is submitted.

[gregestren]

I believe @sdtwigg is centralizing discussion and next steps at #14023.

Let's consolidate there, for focus, and only use other issues for distinctly different concerns.

[fmeum]

Fixed by 711c44e.