-
Notifications
You must be signed in to change notification settings - Fork 4.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
C++ toolchain callbacks #17237
Comments
@nikhilkal would you still be interested in driving this? |
If this existed, would #15191 have used this capability? |
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. |
No, I believe it's limited to clang. |
#17277 describes the issue in AOSP for linking. |
@oquenchil I pinged you on slack.. How can I help |
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 #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 |
@oquenchil has there been any progress in this work? |
No, we are looking for contributions here as described in the issue description and my previous comments. |
@oquenchil, I heard that C++ rules is going to be implemented in Starlark instead of Java. Is this true? If so, is this feature dependent on the new implementation? |
@oquenchil, please answer the question above. |
@pzembrod is the new maintainer of C++ rules. |
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:
a new output by the compilation or linking actions not yet contemplated by the rules
flags that have to be generated dynamically and therefore cannot go in copts or linkopts but for which there isn’t an existing build variable
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):
Custom build variable with short filename: Additional
SOURCE_FILENAME
variable in CppCompileVariables #15924AOSP issue for linking which can benefit from this: CppLink actions having access to fdo_profile_path variable #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
Rule outer later (cc_library, cc_binary, etc..)
CcCompilationHelper.java (we will start rewriting to Starlark this quarter)
CppCompileActionBuilder
CppCompileAction
Linking
Rule outer later (cc_library, cc_binary, etc..)
CcLinkingHelper
CppLinkActoinBuilder
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
The extensions would be a simple dictionary like the one passed here.
For #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.
The text was updated successfully, but these errors were encountered: