/gcbrun

🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use automerge label. Good luck human!

-- conventional-commit-lint bot
https://conventionalcommits.org/

/gcbrun

We intentionally added this in order to leverage compiler warning squelching of system includes. Why do you want to remove this? And, is there a plan in place to provide the same degree of warning tidiness?

We intentionally added this in order to leverage compiler warning squelching of system includes. Why do you want to remove this? And, is there a plan in place to provide the same degree of warning tidiness?

You already filter warnings from external dependencies with

When building this branch locally, I didn't notice any additional warnings printed compared to building the default branch.

Your patch to @googleapis is blocking anyone to depend on google-cloud-cpp via Bzlmod. google-cloud-cpp has to depend on a BCR version of googleapis, otherwise version resolution doesn't work with Bzlmod.

See #14803 for further info.

The results of

find . -type f -exec sed -i 's%#include <\(google/[^>]\+\)>%#include 1%' {} \;

are hitting more than just the headers generated from the protos in googleapis.

We also need to verify that switching the googleapis produced headers from '<>' to '""' does not compromise our warning suppression when using cmake. We would need to make sure that this verification is done with little to no use of our build cache to ensure the output is accurate.

Getting google-cloud-cpp into BCR is something we would like to eventually achieve. But it may need to wait until we have the cycles to make sure we don't break anything in the process.

The results of

find . -type f -exec sed -i 's%#include <\(google/[^>]\+\)>%#include 1%' {} \;

are hitting more than just the headers generated from the protos in googleapis.

I also saw documentation and comments being modified and it looked desirable to have these changed as well. As an alternative, I also thought about find \( -name '*.cc' -o -name '*.h' \) or similar. Can you give me clearer instructions on which includes you don't want to be replaced=

We also need to verify that switching the googleapis produced headers from '<>' to '""' does not compromise our warning suppression when using cmake. We would need to make sure that this verification is done with little to no use of our build cache to ensure the output is accurate.

IIUC one could continue to use system includes for CMake (target_include_directories(SYSTEM)) as "" includes would still find system headers. See quote from the specification:

A preprocessing directive of the form

# include "q-char-sequence" new-line

causes the replacement of that directive by the entire contents of the source file identified by the specified sequence between the " delimiters. The named source file is searched for in an implementation-defined manner. If this search is not supported, or if the search fails, the directive is reprocessed as if it read

# include <h-char-sequence> new-line

with the identical contained sequence (including > characters, if any) from the original directive.

Getting google-cloud-cpp into BCR is something we would like to eventually achieve. But it may need to wait until we have the cycles to make sure we don't break anything in the process.

Please note that the current state is using huge problems to users who already migrated to Bzlmod. For example in gRPC, we have to resort to non-module dependencies which are still using googleapis from BCR at an arbitrary version. Furthermore, if other dependencies also transitively depend on google-cloud-cpp, one would potentially try to link against two different versions of google-cloud-cpp within the same binary leading to ODR violations. So having this available on BCR would make google-cloud-cpp much more usable from other projects.

This will be tackled for the next major release v3 happening in 2025Q1.