0.25 gratuitously breaks cc_common.compile/link

[shahms]

Description of the problem / feature request:

Commit 976876d changed the API for cc_common.compile and cc_common.link in a backwards incompatible way (fine), but also entirely removed the implementation. This would be sort of fine had that deleted implementation not persisted into a release version, but it has done so.

This has broken a number of Kythe tests with no recourse but to blacklist 0.25.

Feature requests: what underlying problem are you trying to solve with this feature?

Not gratuitously breaking users of older in-the-progress-of-changing APIs with no recourse but to blacklist that release.

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

Take a previously working use of the old API (say https://github.com/kythe/kythe/blob/8cd2434b921b32bc2f941431c563cfabc6b384f9/tools/build_rules/verifier_test/cc_indexer_test.bzl#L152) and update it to the new API. Watch as the rule fails with:

method invocation returned None, please file a bug report: compile(actions for<aspect context for /..., <13 more arguments>)

What operating system are you running Bazel on?

Debian GNU/Linux rodete

What's the output of bazel info release?

release 0.25.0

Have you found anything relevant by searching the web?

Yes, I found the commit which broke things.

[oquenchil]

Hi Shams,

this was under an experimental flag and was never supported. Unfortunately, from what I can see there was a bug in the flag implementation that let you use this in aspects. To fix it, change:

    compile_protos_cc_info = cc_common.compile(
        ctx = ctx,
        cc_toolchain = cc_toolchain,
        srcs = sources,
        hdrs = headers,
        feature_configuration = feature_configuration,
        compilation_contexts = [provider.compilation_context for provider in cc_info_providers],
    )
    linking_info = cc_common.link(
        ctx = ctx,
        cc_toolchain = cc_toolchain,
        cc_compilation_outputs = compile_protos_cc_info.cc_compilation_outputs,
        feature_configuration = feature_configuration,
        linking_contexts = [provider.linking_context for provider in cc_info_providers],
    )

to:

    (compilation_context, compilation_outputs) = cc_common.compile(
        actions = ctx.actions,
        cc_toolchain = cc_toolchain,
        srcs = sources,
        public_hdrs = headers,
        feature_configuration = feature_configuration,
        compilation_contexts = [provider.compilation_context for provider in cc_info_providers],
    )
    (linking_context, linking_outputs) = cc_common.link(
        actions = ctx.actions,
        cc_toolchain = cc_toolchain,
        compilation_outputs = compilation_outputs,
        feature_configuration = feature_configuration,
        linking_contexts = [provider.linking_context for provider in cc_info_providers],
   )

0.25 still has the whitelisting and there are some bugs. But that example in particular should be fine. You will have to pass the flag --experimental_cc_skylark_api_enabled_packages followed by any packages that you want to allow this in.

This is easy to change and we won't be delaying the release because of this.

[shahms]

--experimental_cc_skylark_api_enabled_packages does not help because the functions do not have an implementation any more. It is not a matter of being properly whitelisted or the flag being implemented properly.

[oquenchil]

The functions do exist with a different signature. See my reply above.

[shahms]

Yes, making those changes and passing the command line argument results in:
method invocation returned None, please file a bug report: compile(actions for<aspect context for /..., <13 more arguments>)

[shahms]

Because the body of the function being invoked consists of return null;

[shahms]

(For the record, the changes in your reply had all ready been attempted prior to filing the bug, hence the "method invocation returned None" in the original bug description)

[oquenchil]

I see, you are right. Those API methods were not supposed to be usable in Bazel at all, they were experimental. I'm sorry you were bit by this. I'm afraid you will have to wait for the next release.

[shahms]

Intentional or not, they have been available since at least 0.22 and not documented as requiring any flag for use (or even marked experimental): https://docs.bazel.build/versions/0.22.0/skylark/lib/cc_common.html#compile

While we've come to accept Bazel's inherent instability at this stage of development, the wholesale removal of documented, working (if deprecated) APIs with no warning or compatibility is unprecedented so far and downright hostile.

[oquenchil]

The flag was there. As I said this was due to a bug. You didn't see the error message when trying this from a rule and not an aspect?

[shahms]

I don't doubt that the flag was there but the flag, unlike the API itself, is never mentioned in any documentation. As we only ever had need of the cc_common utilities outside of an aspect we never attempted to use them from a rule.

[dkelmer]

hey @shahms, we're working on creating a patch release to fix this issue, hang tight :)

[kamahen]

It appears that 0.24.1 has been removed from the PPA ... could you at least restore that, please? (My apt-fu isn't good, so I might have missed something; but apt list -a bazel shows only 0.25.0; apt-cache shows something similar.)

[dkelmer]

@kamahen, this is a known issue. Feel free to ping #4947 or #6824

[haberman]

I also got the method invocation returned None error above, and I can't figure out how to work around it.

I tried reducing it to a minimal test case and got the same crash even when using it from a rule instead of an aspect:

#build_defs.bzl
load("@bazel_tools//tools/cpp:toolchain_utils.bzl", "find_cpp_toolchain")

def _foo_rule_impl(ctx):
    src = ctx.new_file(ctx.genfiles_dir, "foo.c")
    hdr = ctx.new_file(ctx.genfiles_dir, "foo.h")
    toolchain = find_cpp_toolchain(ctx)
    feature_configuration = cc_common.configure_features(
        cc_toolchain = toolchain,
        requested_features = ctx.features,
        unsupported_features = ctx.disabled_features,
    )
    (compilation_context, compilation_outputs) = cc_common.compile(
        actions = ctx.actions,
        name = "foo",
        feature_configuration = feature_configuration,
        cc_toolchain = toolchain,
        srcs = [src],
        public_hdrs = [hdr],
        compilation_contexts = [],
    )
    return []

foo_rule = rule(
    output_to_genfiles = True,
    implementation = _foo_rule_impl,
    attrs = {
        "_cc_toolchain": attr.label(default = "@bazel_tools//tools/cpp:current_cc_toolchain"),
        "deps": attr.label_list(),
    },
)


# WORKSPACE
workspace(name = "foo_project")  

# BUILD
load(":build_defs.bzl", "foo_rule")

foo_rule(name = "foo") 

Output:

method invocation returned None, please file a bug report: compile(actions for<rule context for //:..., <13 more arguments>)

Will the patch release fix this issue also?

[kamahen]

Part of the problem is that there are some "experimental" flags that are widely used and unlikely to disappear; for example, ----experimental_action_listener.

[hlopko]

(unrelated to the issue at hand)

@kamahen: Experimental flags that are widely used are a problem. https://docs.bazel.build/versions/master/backward-compatibility.html is very clear that:

...
6) APIs and behavior guarded by an --experimental_* flag can change at any time.
7) Users should never run their production builds with --experimental_* or --incompatible_* flags.

And Bazel team follows this policy, very likely to your disadvantage. Therefore the right procedure is to work with the experimental flag owner to stabilize the behavior and remove the flag.

[hlopko]

It seems this issue can be closed now that 0.25.1 with the fix is already released?

[shahms]

Yeah, I've migrated Kythe for the relevant changes and that part works. Currently fighting some unrelated JDK issues.