Skip to content
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

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mering
Copy link
Contributor

@mering mering commented Oct 25, 2024

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

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

The remaining changes are generated with the following command

    find . -type f \( -name '*.h' -o -name '*.cc' \) -exec sed -i 's%#include <\(google/[^>]\+\)>%#include 1%' {} \;
@dbolduc
Copy link
Member

dbolduc commented Oct 25, 2024

/gcbrun

@dbolduc dbolduc changed the title Convert googleapis system includes to normal ones cleanup(bazel): convert googleapis system includes to normal ones Oct 25, 2024
Copy link

🤖 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/

Generated via

    ci/cloudbuild/build.sh -t checkers-pr
@dbolduc
Copy link
Member

dbolduc commented Oct 25, 2024

/gcbrun

@scotthart scotthart added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Oct 28, 2024
@scotthart
Copy link
Member

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?

@mering
Copy link
Contributor Author

mering commented Oct 28, 2024

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

# Don't show warnings when building external dependencies. This still shows
# warnings when using these dependencies (say in headers).
build --output_filter='^//((?!(external):).)*$'

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.

@scotthart
Copy link
Member

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.

@mering
Copy link
Contributor Author

mering commented Nov 6, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge Indicates a pull request not ready for merge, due to either quality or timing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Prepare google-cloud-cpp for addition to BCR
3 participants