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

rules_go 0.41.0 upgrade breaking on missing go_googleapis #3407

Open
leungster opened this issue Jul 18, 2023 · 8 comments · Fixed by #3413
Open

rules_go 0.41.0 upgrade breaking on missing go_googleapis #3407

leungster opened this issue Jul 18, 2023 · 8 comments · Fixed by #3413

Comments

@leungster
Copy link
Contributor

🐛 Bug Report

I upgraded to rules_go to 0.41.0 which removed @go_googleapis as a provided repo. However, grpc-gateway still references @go_googleapis internally so I had to enable build_file_generation to regenerate grpc-gateway's BUILD files as well as add resolve clauses for proto references.

While diagnosing the issue, I noticed that grpc gateway's renovate attempted to upgrade to rules_go 0.41.0 but did so in an inconsistent/invalid manner such that the upgrade doesn't take effect.
#3397 has the 0.41.0 version only on the fallback URL and doesn't apply to the file archive either.

To Reproduce

Upgrade to rules_go 0.41.0
Upgrade to gazelle 0.32.0
Run bazel build @com_github_grpc_ecosystem_grpc_gateway_v2//internal/descriptor/apiconfig:apiconfig_proto

Expected behavior

Bazel build succeeds

Actual Behavior

The bazel build will fail because @go_googleapis is no longer provided and gazelle searches for google/api in the local workspace.

external/com_github_grpc_ecosystem_grpc_gateway_v2/internal/descriptor/apiconfig/BUILD.bazel:7:14: no such package '@com_github_grpc_ecosystem_grpc_gateway_v2//google/api': BUILD file not found in directory 'google/api' of external repository @com_github_grpc_ecosystem_grpc_gateway_v2. Add a BUILD file to a directory to mark it as a package. and referenced by '@com_github_grpc_ecosystem_grpc_gateway_v2//internal/descriptor/apiconfig:apiconfig_proto'

Your Environment

OSX Ventura 13.4.1

@FrankSpitulski
Copy link

FrankSpitulski commented Aug 2, 2023

I don't think this is quite fixed. The fix from #3413 requires "-std=c++14" to be passed in by the end user too since .bazelrc files from external modules are ignored. Ideally the library failing on < c++14 would specify that in its own BUILD.bazel cc_library configs. My build is failing on the @com_google_absl library which I believe is coming in transitively from here.

$ bazel build @com_github_grpc_ecosystem_grpc_gateway_v2//internal/descriptor/apiconfig:apiconfig_proto
Starting local Bazel server and connecting to it...
INFO: Analyzed target @com_github_grpc_ecosystem_grpc_gateway_v2//internal/descriptor/apiconfig:apiconfig_proto (92 packages loaded, 1178 targets configured).
INFO: Found 1 target...
ERROR: /private/var/tmp/.../external/com_google_absl/absl/debugging/BUILD.bazel:183:11: Compiling absl/debugging/internal/address_is_readable.cc [for tool] failed: (Exit 1): cc_wrapper.sh failed: error executing command (from target @com_google_absl//absl/debugging:debugging_internal) external/local_config_cc/cc_wrapper.sh -U_FORTIFY_SOURCE -fstack-protector -Wall -Wthread-safety -Wself-assign -Wunused-but-set-parameter -Wno-free-nonheap-object -fcolor-diagnostics ... (remaining 63 arguments skipped)

@adambabik adambabik reopened this Aug 2, 2023
@adambabik
Copy link
Collaborator

@FrankSpitulski is there anything more to this stack trace? I can't see exactly what error was reported.

Ideally the library failing on < c++14 would specify that in its own BUILD.bazel cc_library configs.

We don't define any cc_library rule. We depend on @com_google_protobuf and @com_google_absl comes from there. You actually might want to consult https://github.com/protocolbuffers/protobuf.

@adambabik
Copy link
Collaborator

I managed to reproduce it in Docker using our image ghcr.io/grpc-ecosystem/grpc-gateway/build-env:1.19 and the only additional info I found was:

gcc: fatal error: Killed signal terminated program cc1plus

which typically indicates a problem with resources AFAIK.

@FrankSpitulski
Copy link

unfortunately that's all the output bazel produced. It's definitely an issue with the transitive proto -> absl dependency though. Ideally it would be fixed by those upstream projects, but users of go get with this library aren't running into that issue, despite the problems upstream.

@adambabik
Copy link
Collaborator

I managed to run bazel build //internal/descriptor/apiconfig:apiconfig_proto successfully in the mentioned Docker image. I used 4 CPUs and 16 GiB of memory. In the CI, we use 2 CPUs and 7 GiB of memory. My recommendation would be to try to build it with more resources.

go get does not rely on Bazel at all.

@FrankSpitulski
Copy link

FrankSpitulski commented Aug 3, 2023

right I brought up go get to show a difference in use between build systems when using this library. go get does not require you to set extra c flags.

I see your build command omits the @library name indicating that you're building in this repo right? that means you are getting settings (c++14) from the bazelrc at the root of this repo. People importing this library will not have that.

@alexbozhenko
Copy link

It is also broken in rules_proto_grpc:
rules-proto-grpc/rules_proto_grpc#275

@adambabik
Copy link
Collaborator

I see your build command omits the @library name indicating that you're building in this repo right? that means you are getting settings (c++14) from the bazelrc at the root of this repo. People importing this library will not have that.

That's correct, I built in the same repo. However, I don't think there is any better way to enforce it. For example, https://github.com/protocolbuffers/protobuf follows the same strategy by setting C++ options in .bazelrc. I believe that anything that depends on protobuf must set those options explicitly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants