-
Notifications
You must be signed in to change notification settings - Fork 377
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
base: main
Are you sure you want to change the base?
Conversation
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%' {} \;
/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 -- conventional-commit-lint bot |
Generated via ci/cloudbuild/build.sh -t checkers-pr
/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? |
You already filter warnings from external dependencies with Lines 56 to 58 in a552c93
When building this branch locally, I didn't notice any additional warnings printed compared to building the default branch. Your patch to See #14803 for further info. |
The results of
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. |
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
IIUC one could continue to use system includes for CMake (
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 |
This will be tackled for the next major release v3 happening in 2025Q1. |
Manually removed the
googleapis_system_includes
target fromgoogleapis.modules.patch
andBUILD
dependencies.The remaining changes to C++ files have been generated with the following commands
Fixes #14803
This change is