bazelbuild / bazel

a fast, scalable, multi-language and extensible build system
https://bazel.build
Apache License 2.0
23k stars 4.03k forks source link

C++ toolchain callbacks #17237

Open oquenchil opened 1 year ago

oquenchil commented 1 year ago

Description

Add the option to create a Starlark method tied to the C++ toolchain callable before actions

There are many feature requests that involve adding code to the Bazel C++ rules in order to pipe the variables necessary for very specific use cases:

It would scale better if the C++ rules code base didn’t have to add explicit support for each of these use cases.

Work

The community will drive adding support for this feature, it first requires discussion in a design doc and reviewing existing issues labeled team-rules-Cpp in bazelbuild/bazel to make sure that we consider every use case that could benefit from this. Once the design is approved, the author can proceed with implementation.

Issues that could benefit (important to find more to justify the work here):

  1. Custom build variable with short filename: https://github.com/bazelbuild/bazel/issues/15924

  2. AOSP issue for linking which can benefit from this: https://github.com/bazelbuild/bazel/issues/17277

Possible implementation

(This is an idea of how I imagine it roughly, final implementation should be guided by the design.)

Placement

Currently the C++ rules are structured as follows:

Compilation

  1. Rule outer later (cc_library, cc_binary, etc..)

  2. CcCompilationHelper.java (we will start rewriting to Starlark this quarter)

  3. CppCompileActionBuilder

  4. CppCompileAction

Linking

  1. Rule outer later (cc_library, cc_binary, etc..)

  2. CcLinkingHelper

  3. CppLinkActoinBuilder

  4. CppLinkAction

The toolchain callback should probably go between 2 and 3, in other words, it should be called once for every CppCompileAction and every CppLinkAction created. This allows adding the custom output per action.

Possible callback signature for CppCompileAction

def toolchain_callback(ctx, source): # is cc_toolchain and feature config needed  here?
        # Logic specific to each implementation goes here
        # …..
    # Output which would then be read from the C++ rule’s implementation and passed
        # when constructing the CppCompile action
    return {
                 “additional_inputs” : new_inputs,
                 “additional_outputs” : new_outputs,            
                 “extensions” : extensions, # Build variable extensions for the crosstool
            }

The extensions would be a simple dictionary like the one passed here.

For https://github.com/bazelbuild/bazel/issues/15924, the flags can be created in a loop one by one based on the source passed.

There are plans for Q1 and Q2 to starlarkify CcCompilationHelper but meanwhile the part of the C++ rules logic that would invoke the callback is still in Java despite the toolchain callback being in Starlark. Calling Starlark code from Java is possible via Starlark.call (see here, here and here).

The compilation callback would be stored in the CcToolchainProvider and called before creating every CppCompilation action. Similarly for the linking callback. The callback can be passed as an argument here. This feature request only makes sense for projects that are using Bazel's C++ rules and want to add customization on top, a project with custom rules doesn't need this feature.

The suggestions above can be modified completely after experimenting with real use cases. After the design discussion and experimenting contributors may also conclude that this isn't really needed and there are better ways to achieve the same thing.

oquenchil commented 1 year ago

@nikhilkal would you still be interested in driving this?

brentleyjones commented 1 year ago

If this existed, would #15191 have used this capability?

oquenchil commented 1 year ago

So all of this is kind of subjective. First, we should consider how widely useful something is to decide whether to embed logic for something like that in Bazel or have it purely in the toolchain with the feature described here. If it's generally useful, then we shouldn't force everyone to implement it in their own toolchain. Do the same flags work in gcc or MSVC?

I don't really have a sense of how often people are using that diagnostics feature you added. If we could have the code that supports it living in the toolchain in Starlark and not annoy anyone, then why not?

At first glance, it looks like it is indeed the type of flag that the feature requested here could help with. As an exercise the design for this feature could try to satisfy that use case even if the PR is already merged, just to prove that it's flexible enough.

We can consider later whether we should rewrite that PR using this feature.

brentleyjones commented 1 year ago

Do the same flags work in gcc or MSVC?

No, I believe it's limited to clang.

vinhdaitran commented 1 year ago

17277 describes the issue in AOSP for linking.

nikhilkalige commented 1 year ago

@oquenchil I pinged you on slack.. How can I help

oquenchil commented 1 year ago

As much as you can afford in driving this issue from design to implementation. I linked 2 use cases (including yours) in the original description then https://github.com/bazelbuild/bazel/issues/15191 is another issue that would have benefited from this.

If the design satisfies those 3 use cases, that'd show enough flexibility, if you find even more issues related to the C++ rules that would benefit from this, even better. Next week I'm planning to start looking at every issue (about 400) and see if I can find common themes. I will keep an eye for any that could be solved with this.

Meanwhile I think you could start a design doc for the 3 examples that we have. Here you can find many examples of proposals: https://github.com/bazelbuild/proposals

daivinhtran commented 1 year ago

@oquenchil has there been any progress in this work?

oquenchil commented 1 year ago

No, we are looking for contributions here as described in the issue description and my previous comments.