cxxopts and conlyopts parameters to cc_library #22041
Labels
P4
This is either out of scope or we don't have bandwidth to review a PR. (No assignee)
team-Rules-CPP
Issues for C++ rules
type: feature request
Comments
comius
added
P4
|
I'm leaning to P3, but I'd need more inputs of what can potentially be affected. |
|
Do you mean that you need examples of projects that need it and use cases? Please see the bug report. BoringSSL has been having to work around this Bazel limitation for some time, as the build system simply does not meet our needs right now. Indeed our testing fidelity is much worse than it could be because of Bazel here; as a library with some C++ internals but a C API, we would ideally have tests where our APIs' immediate callers are C. But this issue means that mixed C/C++ test binaries are so difficult to arrange that we largely don't bother. It also complicates incremental migration of the C portions of the library to C++. |
|
#23460 is another consequence of this bug. What can be done to prioritize this issue? This issue means that BoringSSL, a key dependency of many Google Cloud projects, is unable to effectively use Bazel. |
davidben
added a commit
to google/boringssl
that referenced
this issue
Aug 29, 2024
To work around Bazel's bugs around mixed C/C++ targets (bazelbuild/bazel#22041), we automatically split all of our targets in two. When we did this, we originally pulled the C files into their own target. This had the side effect of building assembly files with the C files instead of with the C++ files. In principle, this does not matter, but Bazel likes to turn targets into shared libraries, and our assembly files still contain a couple references to OPENSSL_ia32cap_P (see https://crbug.com/42290548). Those references rely on OPENSSL_ia32cap_P being a hidden symbol, and statically linked with the assembly files. Pull the C++ out instead, to avoid this. Once https://crbug.com/42290548 is done, either will work, but this is needed for now. Bug: 362664827 Change-Id: Icb929d194ee2311707fe1a0bb27ea0ccaf96a510 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/70690 Reviewed-by: Bob Beck <[email protected]> Commit-Queue: David Benjamin <[email protected]>
|
Bumping this as we're needing to now work around a workaround of a workaround of this bug. |
davidben
added a commit
to google/boringssl
that referenced
this issue
Sep 23, 2024
This fixes an issue where :ssl_test in the Bazel build ran no tests. (The CMake build was fine.) Bazel has no way to handle C and C++ sources need different flags (bazelbuild/bazel#22041), so our Bazel rules transparently split mixed C/C++ targets in two. Bazel silently turns all static libraries into shared libraries for tests. This means that, even if we set linkstatic = True on the split-out library, the two halves don't end up in the same shared object. This is because if A(test) -> B(lib) -> C(lib) and C is linkstatic, C is statically linked into A, not B. This is probably to avoid diamond dependency issues. From what I can tell, there is no clean way to say "B can be made into a shared library but please statically link C into it, because C will never be referenced except by way of B". (This means our use of linkshared is wrong. The next CL will try to redo that.) This Bazel behavior does not recognize that static and shared libraries have very, very different symbol handling. In particular, our assembly files needed to be in the same shared object as OPENSSL_ia32cap_P, so all this required another workaround in https://boringssl-review.googlesource.com/c/boringssl/+/70690 This, in turn, triggered this latest issue: GoogleTest relies on static initializers of individual object files to register tests. This does not work with linker dead code elimination and needs --whole-archive, or alwayslink in Bazel parlance. The most recent Bazel workaround required the C++ sources be the ones that were extracted, so they lost the --whole-archive behavior. As a result, :ssl_test silently ran no tests! Work around this latest Bazel-induced problem by setting alwayslink on cc_test helpers. All this would go away if Bazel just fixed bazelbuild/bazel#22041 (We can probably revert https://boringssl-review.googlesource.com/c/boringssl/+/70690 now, but either way we should probably set alwayslink=True on the helpers so that the build is not sensitive to which were extracted.) Change-Id: I10745e1bcfe91ac929f154e66093b29723efc576 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/71507 Reviewed-by: Bob Beck <[email protected]> Auto-Submit: David Benjamin <[email protected]> Commit-Queue: Bob Beck <[email protected]>
davidben
added a commit
to google/boringssl
that referenced
this issue
Sep 23, 2024
Due to a longstanding Bazel flaw (bazelbuild/bazel#22041), we need to split all our mixed C/C++ targets in two. Ideally this split would behave as if the Bazel flaw were fixed, with the split-out library statically linked with the other source files. Accordingly, the helper macro sets linkstatic = True. It turns out linkstatic = True does not work this way. Bazel interprets linkstatic such that, if A(test, linkshared) -> B(library) -> C(library, linkstatic), C will be statically linked into A, not B. This is probably to avoid diamond dependency problems but means it is not possible for a cc_library split to be transparent, only cc_binary and cc_test. So what is happening is that every cc_test that links libcrypto is getting mlkem.cc statically linked into it, separate from the rest of libcrypto! That means we're getting the worst of both worlds: worse cache hit rate for tests that link libcrypto AND our C/C++ bits are still not contained in the same library. linkstatic = True on the helper is still valuable in cc_test and cc_binary, but otherwise inherit the outer value. Change-Id: I1089c58c6ddaa90c89efd8cdcebd88169b0236c8 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/71508 Reviewed-by: Bob Beck <[email protected]> Commit-Queue: David Benjamin <[email protected]>
keith
added a commit
to keith/bazel
that referenced
this issue
Sep 27, 2024
The inability to pass C or C++ specific compiler flags to targets that contain a mix of those sources is a common sticking point for new users. These mirror the global `--conlyopt` and `--cxxopt` flags but at the target level. Fixes bazelbuild#22041
|
I took a pass at this for cc_library and cc_binary, ptal: #23792 |
keith
added a commit
to keith/bazel
that referenced
this issue
Oct 8, 2024
The inability to pass C or C++ specific compiler flags to targets that contain a mix of those sources is a common sticking point for new users. These mirror the global `--conlyopt` and `--cxxopt` flags but at the target level. Fixes bazelbuild#22041 RELNOTES: Add conlyopts and cxxopts attributes to cc rules Closes bazelbuild#23840 PiperOrigin-RevId: 682144094 Change-Id: I0fe8b728c493652d875ce6a6dd2a9989c36b1f24 (cherry picked from commit 5854788)
github-merge-queue bot
pushed a commit
that referenced
this issue
Oct 9, 2024
The inability to pass C or C++ specific compiler flags to targets that contain a mix of those sources is a common sticking point for new users. These mirror the global `--conlyopt` and `--cxxopt` flags but at the target level. Fixes #22041 RELNOTES: Add conlyopts and cxxopts attributes to cc rules Closes #23840 PiperOrigin-RevId: 682144094 Change-Id: I0fe8b728c493652d875ce6a6dd2a9989c36b1f24 (cherry picked from commit 5854788)
|
A fix for this issue has been included in Bazel 7.4.0 RC1. Please test out the release candidate and report any issues as soon as possible. |
|
Works great. Thank you! Will make a note to upload the build simplification once our minimum Bazel has advanced enough. |
|
Is it possible to specify |
|
in a toolchain you can differentiate by having a feature config that only applies to the C compile or C++ compile actions |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Description of the feature request:
C and C++ compiler options are very related, but not always identical, ranging from variations in language specifiers and even warnings flags. For example GCC uses slightly different names for the same warning between C and C++:
https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wmissing-prototypes
https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wmissing-declarations
Bazel does have --cxxopt and --conlyopt flags, but there is no way to do it from a build file. (A library project cannot control the flags used by upstreams.) It is possible to workaround this by splitting the target in two, but this is messy, particularly in a library project that already needs to support multiple build systems.
Other build tools already have this capability.
https://discourse.cmake.org/t/language-specific-add-compile-options/2643
https://www.gnu.org/software/automake/manual/html_node/Standard-Configuration-Variables.html
https://gn.googlesource.com/gn/+/main/docs/reference.md#var_cflags
Which category does this issue belong to?
C++ Rules
What underlying problem are you trying to solve with this feature?
See above
Which operating system are you running Bazel on?
All
What is the output of
bazel info release?No response
If
bazel info releasereturnsdevelopment versionor(@non-git), tell us how you built Bazel.No response
What's the output of
git remote get-url origin; git rev-parse HEAD?No response
Have you found anything relevant by searching the web?
No response
Any other information, logs, or outputs that you want to share?
No response
The text was updated successfully, but these errors were encountered: