google-cloud-cpp icon indicating copy to clipboard operation
google-cloud-cpp copied to clipboard

cleanup(bazel): convert googleapis system includes to normal ones

Open mering opened this issue 1 year ago • 8 comments

Manually removed the googleapis_system_includes target from googleapis.modules.patch and BUILD dependencies.

The remaining changes to C++ files have been generated with the following commands

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

Fixes #14803


This change is Reviewable

mering avatar Oct 25 '24 19:10 mering

/gcbrun

dbolduc avatar Oct 25 '24 19:10 dbolduc

🤖 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

dbolduc avatar Oct 25 '24 19:10 dbolduc

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?

scotthart avatar Oct 28 '24 20:10 scotthart

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 https://github.com/googleapis/google-cloud-cpp/blob/a552c933ebfffd1c7e73794b003c84e590bfb82b/.bazelrc#L56-L58

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.

mering avatar Oct 28 '24 20:10 mering

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.

scotthart avatar Nov 05 '24 21:11 scotthart

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.

mering avatar Nov 06 '24 21:11 mering

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

mering avatar Nov 27 '24 08:11 mering

Obsoleted by #15229

mering avatar Jun 27 '25 10:06 mering