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.

Sign up for GitHub

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

Jump to bottom

Prepare google-cloud-cpp for addition to BCR #14803

Open
dbolduc opened this issue Oct 25, 2024 · 3 comments · May be fixed by #14804
Open

Prepare google-cloud-cpp for addition to BCR #14803

dbolduc opened this issue Oct 25, 2024 · 3 comments · May be fixed by #14804
Labels
type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.

Comments

@dbolduc
Copy link
Member

dbolduc commented Oct 25, 2024

In order to add our repository to the Bazel Central Registry, we cannot have any patches. So says smart Bazel person, @mering.

The one offending patch is for googleapis: https://github.com/googleapis/google-cloud-cpp/blob/main/bazel/googleapis.modules.patch

#14771 removed half of the patch. We balked at accepting the PR though because we did not love that we would have less control over the googleapis SHA update.

However, when we frame the trade-off like this, it seems like the PR is a net positive:
Con:

  • updating googleapis SHA takes up to a day and is out of our control
    Pro:
  • google-cloud-cpp can be added to BCR

Note that we should still be able to pin googleapis to any commit in our repo. We would just have to make sure that commit is on BCR first.

The other half of the patch is the system_includes bit. I don't know exactly what changes. If it is only about suppressing warnings, we were given the following tip:

We are using the following setting in our .bazelrc to hide warnings from external dependencies:

# Avoid warnings from third-party deps that we don't control.
build --per_file_copt=external/.*@-Wno-everything
@dbolduc dbolduc added the type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. label Oct 25, 2024
@mering
Copy link
Contributor

mering commented Oct 25, 2024 •
edited
Loading

Quoting the documentation:

Only the root module's overrides take effect — if a module is used as a dependency, its overrides are ignored.

So it is important that google-cloud-cpp builds without the patch (which is specified in an override).

Edit: It is important that google-cloud-cpp builds with the same patches as on BCR when using overrides (specifically removing the system includes part).

@mering
Copy link
Contributor

mering commented Oct 25, 2024 •
edited
Loading

I noticed you are already ignoring warnings from external dependencies:

google-cloud-cpp/.bazelrc

Lines 56 to 58 in bf3bf1c

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

I tried adding my suggestion to .bazelrc as well and couldn't notice a difference in the shown warnings.

@mering mering linked a pull request Oct 25, 2024 that will close this issue
Open
@mering
Copy link
Contributor

mering commented Oct 25, 2024

The system includes part could be fixed by #14804.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

cleanup(bazel): convert googleapis system includes to normal ones mering/google-cloud-cpp
2 participants
@dbolduc @mering